Jonathan Watt [:jwatt] Assignee
	Description • 9 years ago

The implementation of Directory.getContents() and Directory.path can be done as a standalone piece of work separate from the greater directory picker implementation bug, bug 1164310.

Directory objects can be obtained via the device storage API on B2G. This bug is about implementing .getContents() and .path sufficiently completely to be usable on that platform.

	Jonathan Watt [:jwatt] Assignee
	Comment 1 • 9 years ago

Currently when a Blob is created from an nsIFile that is a directory we just treat it as a normal file with zero size. Usually we don't have a direct way to determine whether such a Blob is from an empty file or a directory. This just adds the ability for us to tag such Blobs with the information that they correspond to a directory.

	Andrea Marchesini [:baku]
	Comment 5 • 9 years ago

Comment on attachment 8626508 [details] [diff] [review]
part 1 - Add API and functionality to the BlobImpl classes in order that BlobImpl's that are created from an nsIFile can provide information about whether or not the nsIFile was a directory

Review of attachment 8626508 [details] [diff] [review]:
-----------------------------------------------------------------

I want to check how to propagate isDirectory via IPC for RemoteBlobs with bent.

::: dom/base/File.h
@@ +362,5 @@
>  
>    virtual bool IsDateUnknown() const = 0;
>  
>    virtual bool IsFile() const = 0;
> +  virtual void SetIsDirectory() = 0;

that can go away.

@@ +528,5 @@
>    {
>      return mIsFile;
>    }
>  
> +  virtual void SetIsDirectory() override

Just add:
 mFile->IsDirectory(&mIsDirectory);
in the CTOR of BlobImplFile.

::: dom/ipc/Blob.cpp
@@ +2143,5 @@
>    virtual bool
>    IsFile() const override;
>  
> +  virtual void
> +  SetIsDirectory() override;

we can remove this.

	Andrea Marchesini [:baku]
	Comment 6 • 9 years ago

Comment on attachment 8626509 [details] [diff] [review]
part 2 - Add support to the FileSystem code for obtaining a listing of a Directory's Directory and File contents via a sequence of Blobs

Review of attachment 8626509 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/filesystem/GetDirectoryListingTask.cpp
@@ +58,5 @@
> +already_AddRefed<Promise>
> +GetDirectoryListingTask::GetPromise()
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> +  return nsRefPtr<Promise>(mPromise).forget();

What about:

Promise*
GetDirectoryListingTask::GetPromise() const
{
  MOZ_ASSERT(...);
  return mPromise;
}

@@ +114,5 @@
> +  }
> +
> +  // Whether we want to get the root directory.
> +  bool getRoot = mTargetRealPath.IsEmpty();
> +

All of these code lines are equal to GetFileOrDirectoryTask::Work(). We should try to create an helper or move them to the base class.

@@ +157,5 @@
> +  }
> +
> +  for (;;) {
> +    bool hasMore = false;
> +    if (NS_FAILED(entries->HasMoreElements(&hasMore)) || !hasMore) {

NS_WARN_IF

@@ +161,5 @@
> +    if (NS_FAILED(entries->HasMoreElements(&hasMore)) || !hasMore) {
> +      break;
> +    }
> +    nsCOMPtr<nsISupports> supp;
> +    if (NS_FAILED(entries->GetNext(getter_AddRefs(supp)))) {

NS_WARN_IF

@@ +165,5 @@
> +    if (NS_FAILED(entries->GetNext(getter_AddRefs(supp)))) {
> +      break;
> +    }
> +    nsCOMPtr<nsIFile> currFile = do_QueryInterface(supp);
> +    

extra spaces + MOZ_ASSERT(currFile);

@@ +167,5 @@
> +    }
> +    nsCOMPtr<nsIFile> currFile = do_QueryInterface(supp);
> +    
> +    bool isLink, isSpecial, isFile;
> +    currFile->IsSymlink(&isLink);

In theory all of these methods (IsSymlink, IsSpecial and IsDirectory) should fail, right?

@@ +179,5 @@
> +      continue;
> +    }
> +    BlobImplFile* impl = new BlobImplFile(currFile);
> +    if (isDir) {
> +      impl->SetIsDirectory();

Remove this.

@@ +219,5 @@
> +      nsAutoString name;
> +      mTargetBlobImpls[i]->GetName(name);
> +      nsAutoString path(mTargetRealPath);
> +      path.AppendLiteral(FILESYSTEM_DOM_PATH_SEPARATOR);
> +      path.Append(name);

I would add:

#ifdef DEBUG
  nsCOMPtr<nsIFile> file = mFileSystem->GetLocalFile(path);
  bool exist;
  file->Exists(&exist);
  MOZ_ASSERT(exist);
}
#endif

::: dom/filesystem/GetDirectoryListingTask.h
@@ +7,5 @@
> +#ifndef mozilla_dom_GetDirectoryListing_h
> +#define mozilla_dom_GetDirectoryListing_h
> +
> +#include "mozilla/dom/FileSystemTaskBase.h"
> +#include "nsAutoPtr.h"

alphabetic order of the headers.

@@ +31,5 @@
> +  virtual
> +  ~GetDirectoryListingTask();
> +
> +  already_AddRefed<Promise>
> +  GetPromise();

const ?

	Andrea Marchesini [:baku]
	Comment 7 • 9 years ago

Comment on attachment 8626510 [details] [diff] [review]
part 3 - Implement .getContents() and .path for DOM Directory objects

Review of attachment 8626510 [details] [diff] [review]:
-----------------------------------------------------------------

Follow up for the workers, and maybe check in the spec or in the working group if we have to support Directory in the PostMessages.
Otherwise, without a Constructor() and without postMessages, there are no ways to have a Directory in workers.

::: dom/filesystem/Directory.cpp
@@ +235,5 @@
>    return task->GetPromise();
>  }
>  
> +void
> +Directory::GetPath(nsString& aRetval) const

GetPath(nsAString& aRetval) const

@@ +250,5 @@
> +already_AddRefed<Promise>
> +Directory::GetContents()
> +{
> +  nsresult error = NS_OK;
> +  nsString realPath;

nsAutoString

@@ +251,5 @@
> +Directory::GetContents()
> +{
> +  nsresult error = NS_OK;
> +  nsString realPath;
> +  ErrorResult aRv; // XXXjwatt

1. rv
2. plus... why this comment?

@@ +254,5 @@
> +  nsString realPath;
> +  ErrorResult aRv; // XXXjwatt
> +  nsRefPtr<GetDirectoryListingTask> task =
> +    new GetDirectoryListingTask(mFileSystem, mPath, aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF

::: dom/webidl/Directory.webidl
@@ +7,1 @@
>  /*

Can you add the URL specs for Directory at the top of this file?

@@ +35,5 @@
>     * The 'data' property contains the new file's content.
>     * @return If succeeds, the promise is resolved with the new created
>     * File object. Otherwise, rejected with a DOM error.
>     */
> +  [Pref="device.storage.enabled", NewObject]

If you want to expose it to workers, you have to write Func="Directory::IsEnabled" instead Pref="..". Then you can implement that function similar to BroadcastChannel::IsEnabled, or MessageChannel::IsEnabled.

@@ +47,5 @@
>     * If path exists, createDirectory must fail.
>     * @return If succeeds, the promise is resolved with the new created
>     * Directory object. Otherwise, rejected with a DOM error.
>     */
> +  [Pref="device.storage.enabled", NewObject]

Same here.

	Andrea Marchesini [:baku]
	Comment 8 • 9 years ago

Comment on attachment 8626511 [details] [diff] [review]
part 4 - Add DeviceStorage tests for the new .getContents() and .path API on Directory

Review of attachment 8626511 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/devicestorage/test/test_fs_getContents.html
@@ +27,5 @@
> +// The root directory object.
> +var gRoot = null;
> +
> +function checkContents(contents) {
> +  is(contents.length, 4, "The sub-directory should contain four children");

move this line after 'expected' and do:

is (contents.length, expected.length, ...

@@ +58,5 @@
> +  ok(false, "Should not arrive at handleError! Error: " + e.name);
> +  devicestorage_cleanup();
> +}
> +
> +function createTestFiles(paths, callback) {

This seems == to what we have in test_fs_remove.html. Move it into a helper.js.

@@ +108,5 @@
> +  gStorage.getRoot().then(function(rootDir) {
> +    gRoot = rootDir;
> +    return rootDir.get("sub");
> +  }).then(function(subDir) {
> +    return subDir.getContents();

can you check if you can get data also in the sub directories? 'sub/sub2' for instance?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 9 • 9 years ago

Comment on attachment 8626508 [details] [diff] [review]
part 1 - Add API and functionality to the BlobImpl classes in order that BlobImpl's that are created from an nsIFile can provide information about whether or not the nsIFile was a directory

Review of attachment 8626508 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, looks like you'll need to add an 'isDirectory' member to FileBlobConstructorParams, and then you'll need to set BlobImplBase::mIsDirectory in the BlobChild::RemoteBlobImpl constructor if that arg is set.

	Jonathan Watt [:jwatt] Assignee
	Comment 10 • 9 years ago

Ah, I guess my testcase should also be checking instanceof and/or recursing as suggested by baku.

Ben, do you want to see an updated patch, or is it okay to just make those changes?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 11 • 9 years ago

I'd be happy to look over an interdiff!

	Jonathan Watt [:jwatt] Assignee
	Comment 12 • 9 years ago

Using FileBlobConstructorParams.isDirectory appropriately in all the places that we use FileBlobConstructorParams in Blob.cpp has quite a chain of knock-on effects. Seems like this needs an actual review.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 13 • 9 years ago

Comment on attachment 8626887 [details] [diff] [review]
part 1b

Review of attachment 8626887 [details] [diff] [review]:
-----------------------------------------------------------------

Most of this looks fine, except you're adding quite a performance footgun (similar to another in blob code that we've been trying to stamp out nearly forever).

::: dom/base/File.h
@@ +603,5 @@
>    NS_DECL_ISUPPORTS_INHERITED
>  
>    BlobImplMemory(void* aMemoryBuffer, uint64_t aLength, const nsAString& aName,
>                   const nsAString& aContentType, int64_t aLastModifiedDate)
> +    : BlobImplBase(aName, aContentType, aLength, aLastModifiedDate, false)

Nit: I think it's always nicer to put the arg name in /* */ comments when using bool literals. E.g.:

  BlobImplBase(... aLastModifiedDate, /* aIsDirectory */ false)

@@ +779,5 @@
>  
>    BlobImplFile(const nsAString& aName, const nsAString& aContentType,
>                 uint64_t aLength, nsIFile* aFile,
>                 int64_t aLastModificationDate)
> +    : BlobImplBase(aName, aContentType, aLength, aLastModificationDate, false)

So this will do synchronous I/O on whatever thread happens to call it. The intent here is to have a constructor that doesn't require any synchronous I/O. Can you force the caller to pass in the aIsDirectory arg rather than filling it in with sync I/O?

In fact, can you do that for as many of these as possible? I hope there are only a few places (maybe only like one?) where we need to create a File object and we only have an nsIFile and are forced to hit the disk to fill in the metadata.

	Jonathan Watt [:jwatt] Assignee
	Comment 14 • 9 years ago

Ah, yeah, I pointed this out to baku in Whistler and we agreed to have a tri-state flag to indicate is-dir/isn't-dir/unknown, set the state in the places that matter, and MOZ_ASSERT in IsDirectory() that the state is set. Seems I didn't attach the updated patch after getting back to the UK though. Sorry about that. :(

	Andrea Marchesini [:baku]
	Comment 15 • 9 years ago

Comment on attachment 8628604 [details] [diff] [review]
part 1b

Review of attachment 8628604 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/File.h
@@ +55,5 @@
> + * Used to indicate when a Blob/BlobImpl that was created from an nsIFile
> + * (when IsFile() will return true) was from an nsIFile for which
> + * nsIFile::IsDirectory() returned true.
> + */
> +enum BlobDirState : uint32_t {

can we move this into the Blob class or better in BlobImpl?

@@ +203,5 @@
>  
>    static already_AddRefed<File>
>    Create(nsISupports* aParent, const nsAString& aName,
>           const nsAString& aContentType, uint64_t aLength,
> +         int64_t aLastModifiedDate, BlobDirState aDirState);

What about having a defualt value? ..., BlobDirState aDirState = eUnknownIfDir)

	Jonathan Watt [:jwatt] Assignee
	Comment 16 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #15)
> can we move this into the Blob class or better in BlobImpl?

That's what I was doing initially, but I then had to include File.h into various headers that were otherwise very slim since I couldn't forward declare class-nested enums.

> What about having a defualt value? ..., BlobDirState aDirState =
> eUnknownIfDir)

I'll do that assuming it doesn't seem bad for some reason when I write the code.

	Pulsebot
	Comment 17 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e53abe7d9acc
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e404e27bd8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c050de3408c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/99dcb8342707
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b8f3a6f201c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bc1239c1824

	Carsten Book [:Tomcat]
	Comment 18 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/e53abe7d9acc
https://hg.mozilla.org/mozilla-central/rev/8e404e27bd8a
https://hg.mozilla.org/mozilla-central/rev/c050de3408c8
https://hg.mozilla.org/mozilla-central/rev/99dcb8342707
https://hg.mozilla.org/mozilla-central/rev/2b8f3a6f201c
https://hg.mozilla.org/mozilla-central/rev/7bc1239c1824