910412 - Change DeviceStorage API to use FileSystem API spec

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Yuan Xulei [:yxl] (Leave from May 18)]

As the new FileSystem API covers all user cases of the DeviceStorage API and their structure are similar, we will change the DeviceStorage API to use the FileSystem API spec.

The device storage directory may be accessed by the same interface as the sandboxed filesystem: 
partial interface Navigator {
  Promise<Directory> getFilesystem({storage: 'sdcard'});
};

Or use the original interface with Promise<Directory> as return value

partial interface Navigator {
  Promise<Directory> getDeviceStorage(DOMString type);
};

Comment 1

[Doug Turner (:dougt)]

I tend to like the FileSystem API, but understand that Jonas has some other plans.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Doug: Not sure what you are referring to. I definitely think that we should change DeviceStorage to use the more polished FileSystem API than what it's currently using. We'll still need to support the old one in a transition period though.

What you might be referring to is that I think that we should also add a notification system so that apps don't have to rescan the DeviceStorage on each startup. But that's orthogonal to this bug I believe.

Comment 3

[Mounir Lamouri (:mounir)]

Why are we even implementing the DeviceStorage API then?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, to be clear, I don't think we can get rid of the DeviceStorage interface. At least not in the short term. For now we'll have to have something like:

interface nsIDOMDeviceStorage : nsIDOMEventTarget
{
    // This one is new. Promise resolves to a Directory
    Promise getRoot();

    // These we still don't have replacements for
    attribute jsval onchange;

    nsIDOMDOMRequest freeSpace();
    nsIDOMDOMRequest usedSpace();
    nsIDOMDOMRequest available();

    readonly attribute DOMString storageName;

    readonly attribute bool default;


    // These we should phase out. Add warnings when they are used saying that they are going away.
    nsIDOMDOMRequest add(in nsIDOMBlob aBlob);
    nsIDOMDOMRequest addNamed(in nsIDOMBlob aBlob, in DOMString aName);
    nsIDOMDOMRequest get([Null(Stringify)] in DOMString aName);
    nsIDOMDOMRequest getEditable([Null(Stringify)] in DOMString aName);
    nsIDOMDOMRequest delete([Null(Stringify)] in DOMString aName);
};

Comment 5

[Yuan Xulei [:yxl] (Leave from May 18)]

I'm going to implement the Filesystem API for device storage. The following parts will be implemented as a start:

interface DeviceStorage {
  ...
  Promise<Directory> getRoot();
  ...
};

interface Directory {
  ...
  Promise<Directory> createDirectory(DOMString path);
  Promise<(File or Directory)> get(DOMString path);
  ...
};

@sicking, are you the right guy to help and review the patch, or can you recommend someone else? 
Thanks ;-)

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0001-WIP-Part1-v1-WebIDL-and-building.patch

Add WebIDL and corresponding C++ files.

Comment 7

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0002-WIP-Part2-v1-Implement-getRoot-createDirectory-and-g.patch

Implements DeviceStorage.getRoot, Directory.get and Directory.createDirectory.

Comment 8

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0003-WIP-Part3-v1-mochitest.patch

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not really the person to review. Someone on the DOM or WebAPI teams probably could.

Comment 10

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 825844 [details] [diff] [review]
0001-WIP-Part1-v1-WebIDL-and-building.patch

Hi, Ehsan could help to review the patch?

Comment 11

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #9)
> I'm not really the person to review. Someone on the DOM or WebAPI teams
> probably could.

@sicking, thanks and I'll find someone else to help:-)

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0001-WIP-Part1-v2-WebIDL-and-building.patch

Comment 13

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0002-WIP-Part2-v2-Implement-getRoot-createDirectory-and-g.patch

Comment 14

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0003-WIP-Part3-v2-mochitest.patch

Comment 15

[Dave Hylands [:dhylands]]

Comment on attachment 829124 [details] [diff] [review]
0001-WIP-Part1-v2-WebIDL-and-building.patch

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

So I didn't make it all the way through the files, but I think the issue about exposing the root directory is something that needs to be addressed, and this will probably have an impact on the rest of the files.

I'll post the feedback that I've written thus far on the 3 patches (not to be considered complete yet - but I figured the sooner you get this the better).

::: dom/directory/Directory.cpp
@@ +170,5 @@
> +  if (!FilesystemFile::IsValidRelativePath(fs, relativePath)) {
> +    return false;
> +  }
> +
> +  aRealPath = mFile->GetPath() + NS_LITERAL_STRING("/") + relativePath;

Shouldn't this do: mFile->Append(relativePath) (I believe that mFile under windows uses backslash to seperate file elements)

::: dom/webidl/DeviceStorage.webidl
@@ +48,5 @@
>    // for storing new files.
>    readonly attribute boolean default;
> +
> +  [NewObject]
> +  Promise getRoot();

So I have an issue with exposing the root directory to content.

Currently, device storage abstracts away the top level directory by doing:

/storageName/somedir/somefile.txt

So content only sees /storageName. Internally, this gets mapped to the actual file directory (for example /sdcard maps to /storage/emulated/legacy on the Nexus4).

I would really like to continue along this vein and have the top level directory remain abstract, and everything below that can come from the real filesystem.

From a security standpoint, this also makes it easier to ensure that people aren't accesing things that they shouldn't.

From a security standpoint, we definitely don't want to be passing fully qualified paths from the child to the parent (we used to do this in device storage, but now send storageName and relative path - see bug 860934)

::: dom/webidl/Directory.webidl
@@ +15,5 @@
> +  [NewObject]
> +  Promise createDirectory(DOMString path);
> +
> +  [NewObject]
> +  Promise get(DOMString path);

What does this do? Is this like devicestorage's get which retrieves the contents of a file?

@@ +20,5 @@
> +
> +  [NewObject]
> +  AbortableProgressPromise
> +    move((DOMString or File or Directory) path,
> +         optional (DOMString or Directory or DestinationDict) dest);

Is this to rename a directory? What does it mean when you only pass one argument?

@@ +55,5 @@
> +};
> +
> +enum CreateIfExistsMode { "replace", "fail" };
> +enum OpenIfExistsMode { "open", "fail" };
> +enum OpenIfNotExistsMode { "create", "fail" };

So, from the standpoint of using these, if I'm understanding things properly, you'd do:

dir.createFile(someFileName, 'fail');

Looking at this, I can't really tell what 'fail' means. I think that adding the IfExists to the string makes the calling code look really obvious.

dir.createFile(someFileName, 'failIfExists');
dir.createFile(someFileName, 'replaceIfExists');

Comment 16

[Dave Hylands [:dhylands]]

Comment on attachment 829125 [details] [diff] [review]
0002-WIP-Part2-v2-Implement-getRoot-createDirectory-and-g.patch

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

I only made it partway through these files, but figured I'd post what I have so far.

::: dom/ipc/ContentChild.cpp
@@ +905,5 @@
> +
> +bool
> +ContentChild::DeallocPFilesystemRequestChild(PFilesystemRequestChild* aFilesystem)
> +{
> +    mozilla::dom::TaskBase* child = static_cast<mozilla::dom::TaskBase*>(iframe);

So where does iframe come from? I couldn't find any class variables named iframe.

../../../b2g-inbound/dom/ipc/ContentChild.cpp: In member function 'virtual bool mozilla::dom::ContentChild::DeallocPFilesystemRequestChild(mozilla::dom::PContentChild::PFilesystemRequestChild*)':
../../../b2g-inbound/dom/ipc/ContentChild.cpp:909:74: error: 'iframe' was not declared in this scope

@@ +906,5 @@
> +bool
> +ContentChild::DeallocPFilesystemRequestChild(PFilesystemRequestChild* aFilesystem)
> +{
> +    mozilla::dom::TaskBase* child = static_cast<mozilla::dom::TaskBase*>(iframe);
> +    NS_RELEASE(child);

I couldn't find the increment to go with this release.

::: dom/ipc/ContentParent.cpp
@@ +2193,5 @@
> +
> +bool
> +ContentParent::DeallocPFilesystemRequestParent(PFilesystemRequestParent* doomed)
> +{
> +  FilesystemRequestParent *parent = static_cast<FilesystemRequestParent*>(doomed);

nit: FilesystemRequestParent* parent

Comment 17

[Dave Hylands [:dhylands]]

Comment on attachment 829126 [details] [diff] [review]
0003-WIP-Part3-v2-mochitest.patch

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

Presumably this will need to be fleshed out more and have more tests added.

I think this will need to redone once the getRoot issue is resolved.

::: dom/devicestorage/test/test_fs_basic.html
@@ +16,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=910412">Mozilla Bug 910412</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +  

nit: trailing space

Comment 18

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #15)
> Comment on attachment 829124 [details] [diff] [review]
> 0001-WIP-Part1-v2-WebIDL-and-building.patch
> 
> Review of attachment 829124 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I didn't make it all the way through the files, but I think the issue
> about exposing the root directory is something that needs to be addressed,
> and this will probably have an impact on the rest of the files.
> 
> I'll post the feedback that I've written thus far on the 3 patches (not to
> be considered complete yet - but I figured the sooner you get this the
> better).
> 
> ::: dom/directory/Directory.cpp
> @@ +170,5 @@
> > +  if (!FilesystemFile::IsValidRelativePath(fs, relativePath)) {
> > +    return false;
> > +  }
> > +
> > +  aRealPath = mFile->GetPath() + NS_LITERAL_STRING("/") + relativePath;
> 
> Shouldn't this do: mFile->Append(relativePath) (I believe that mFile under
> windows uses backslash to seperate file elements)

This should append a relative path to the given path.

I'll always use "/" as separator to pass "path" around before creating local file.
I call this kind of internal path by the name "absolute DOM path". When creating
local file, |FilesystemUtils::CetLocalFile| will be called to convert the DOM path
to the right OS dependent path. Below is the implementation of that utility
function:

// static
already_AddRefed<nsIFile>
FilesystemUtils::CetLocalFile(const nsAString& aRealPath)
{
  nsString localPath;
  localPath = aRealPath;
#if defined(XP_WIN)
  PRUnichar* cur = localPath.BeginWriting();
  PRUnichar* end = localPath.EndWriting();
  for (; cur < end; ++cur) {
    if (PRUnichar('\\') == *cur)
      *cur = PRUnichar('/');
  }
#endif
  nsCOMPtr<nsIFile> file;
  nsresult rv = NS_NewLocalFile(localPath, false, getter_AddRefs(file));
  NS_ENSURE_SUCCESS(rv, nullptr);
  return file.forget();
}

The reason I introduce the absolute DOM path is that we will use virtual
path for the sandboxed filesystem, which is going to be implemented later. "The
sandboxed filesystem uses a database to store things like directory structures
and file metadata. Whereas the actual files stored in the filesystem
should create real OS files so that we can do quick reading and
writing." For the sandboxed filesystem, it will use an OS independent path
without limitation on file names or paths.

> 
> ::: dom/webidl/DeviceStorage.webidl
> @@ +48,5 @@
> >    // for storing new files.
> >    readonly attribute boolean default;
> > +
> > +  [NewObject]
> > +  Promise getRoot();
> 
> So I have an issue with exposing the root directory to content.
> 
> Currently, device storage abstracts away the top level directory by doing:
> 
> /storageName/somedir/somefile.txt
> 
> So content only sees /storageName. Internally, this gets mapped to the
> actual file directory (for example /sdcard maps to /storage/emulated/legacy
> on the Nexus4).
> 
> I would really like to continue along this vein and have the top level
> directory remain abstract, and everything below that can come from the real
> filesystem.
> 
> From a security standpoint, this also makes it easier to ensure that people
> aren't accesing things that they shouldn't.
> 
> From a security standpoint, we definitely don't want to be passing fully
> qualified paths from the child to the parent (we used to do this in device
> storage, but now send storageName and relative path - see bug 860934)
Filesystem API uses the similar mechanism to ensure the path security. Here is
a short explanation about the path:
 http://lists.w3.org/Archives/Public/public-script-coord/2013JulSep/0379.html

The path string passed to directory is relative and the content can only
access the descendent files of a given directory. There are two constraints to
enforce the security. 
1) The functions on Directory that accept DOMString arguments for filenames allow
names like "path/to/file.txt". Such paths are always interpreted as relative to the
directory itself, never relative to the root. And the paths aren't allowd to walk
up the directory tree. So paths like "../foo", "..", "/foo/bar" or "foo/../bar"
are not allowed.
2) Passing a File/Directory object to an operation of Directory where the
File/Directory object isn't contained in that directory or its descendents also
results in an error.

These two constraints prevent content from modifying the root directory or getting its parent.
> 
> ::: dom/webidl/Directory.webidl
> @@ +15,5 @@
> > +  [NewObject]
> > +  Promise createDirectory(DOMString path);
> > +
> > +  [NewObject]
> > +  Promise get(DOMString path);
> 
> What does this do? Is this like devicestorage's get which retrieves the
> contents of a file?

Actually, this should be 
 Promise<(File or Directory)> get(DOMString path);

It gets a descendent file or directory with the given relative path. For
example there is a file and a directory under the root:
  root +--- x.txt
       |
       ---- subdirectory

|root.get('x.txt')| returns a File object(http://dev.w3.org/2006/webapi/FileAPI/),
while |root.get('subdirectory') returns a Directory object.
     

> 
> @@ +20,5 @@
> > +
> > +  [NewObject]
> > +  AbortableProgressPromise
> > +    move((DOMString or File or Directory) path,
> > +         optional (DOMString or Directory or DestinationDict) dest);
> 
> Is this to rename a directory? What does it mean when you only pass one
> argument?
Yes, this is to rename a directory. The second argument is required as the spec(http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface) and should not be optional.
But the WebIDL compiler complains about the second argument and says 
"Dictionary argument or union argument containing a dictionary not
followed by a required argument must be optional". So I have to make
it optional. I should leave a comment about the second argument.

Comment 19

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #15)
> @@ +55,5 @@
> > +};
> > +
> > +enum CreateIfExistsMode { "replace", "fail" };
> > +enum OpenIfExistsMode { "open", "fail" };
> > +enum OpenIfNotExistsMode { "create", "fail" };
> 
> So, from the standpoint of using these, if I'm understanding things
> properly, you'd do:
> 
> dir.createFile(someFileName, 'fail');
> 
> Looking at this, I can't really tell what 'fail' means. I think that adding
> the IfExists to the string makes the calling code look really obvious.
> 
> dir.createFile(someFileName, 'failIfExists');
> dir.createFile(someFileName, 'replaceIfExists');

Looks like a good suggestion. I don't have much experience about WebAPI
designing and the implementation will follow the API proposal doc
(http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface).

Jonas is responsible for API proposal. Let's cc him and let him drive
the API designing.

Comment 20

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #16)
> 
> ::: dom/ipc/ContentChild.cpp
> @@ +905,5 @@
> > +
> > +bool
> > +ContentChild::DeallocPFilesystemRequestChild(PFilesystemRequestChild* aFilesystem)
> > +{
> > +    mozilla::dom::TaskBase* child = static_cast<mozilla::dom::TaskBase*>(iframe);
> 
> So where does iframe come from? I couldn't find any class variables named
> iframe.

There is a mistake. |iframe| should be |aFilesystem|.

> 
> @@ +906,5 @@
> > +bool
> > +ContentChild::DeallocPFilesystemRequestChild(PFilesystemRequestChild* aFilesystem)
> > +{
> > +    mozilla::dom::TaskBase* child = static_cast<mozilla::dom::TaskBase*>(iframe);
> > +    NS_RELEASE(child);
> 
> I couldn't find the increment to go with this release.
It is increased in TaskBase::Start of TaskBase.cpp as following:

  void
  TaskBase::Start()
  {
    ...
    } else {
      // Run in child process.
  
      // Retain a reference so the task object isn't deleted without IPDL's
      // knowledge. The reference will be released by
      // mozilla::dom::ContentChild::DeallocPFilesystemRequestChild.
      AddRef();
      ContentChild::GetSingleton()->SendPFilesystemRequestConstructor(this,
        GetRequestParams());
    }
  }

Comment 21

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #17)
> Comment on attachment 829126 [details] [diff] [review]
> 0003-WIP-Part3-v2-mochitest.patch
> 
> Review of attachment 829126 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Presumably this will need to be fleshed out more and have more tests added.
> 
> I think this will need to redone once the getRoot issue is resolved.

Yes, I'll add more tests.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So, to help people read the WebIDL:

createFile is defined as:

interface Directory {
  createFile(DOMString path, optional CreateFileOptions options);
  ...
}

I.e. the second argument is of type 'CreateFileOptions'.

CreateFileOptions is defined in CreateFileOptions.webidl as

dictionary CreateFileOptions {
  CreateIfExistsMode ifExists = "fail";
  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;
};

A 'dictionary' is an object with a number of optional properties, with our without default values. Properties in dictionaries are by definition *always* optional. In this case there are two properties, 'ifExists' and 'data'. Again, even properties without default values are optional.

That means that you call createFile using something like any of the following:

dir.createFile('filename.txt', { ifExists: ..., data: ... });
dir.createFile('filename.txt', { ifExists: ... });
dir.createFile('filename.txt', { data: ... });
dir.createFile('filename.txt', { });
dir.createFile('filename.txt');

You can actually even do

dir.createFile('filename.txt', { ifExists: ..., data: ..., flubber: ..., hello: ... });

in which case the 'flubber' and 'hello' properties will simply be ignored.

Oo, lets look at what values you can pass for the 'ifExists' and 'data' properties.

'ifExists' is defined as a CreateIfExistsMode. CreateIfExistsMode is defined as

enum CreateIfExistsMode { "replace", "fail" };

this means that 'ifExists' can be set to either the string "replace" or the string "fail". Since CreateFileOptions defines 'fail' as default value for ifExists, if no value is provided, the value 'fail' is used.

'data' is defined as (DOMString or Blob or ArrayBuffer or ArrayBufferView). So you can pass any object of those types.

So here are some full examples of calls to createFile

A) dir.createFile('filename.txt', { ifExists: "fail", data: myBlob });
B) dir.createFile('filename.txt', { ifExists: "replace", data: "filecontents" });
C) dir.createFile('filename.txt', { ifExists: "fail" });
D) dir.createFile('filename.txt', { ifExists: "replace" });
E) dir.createFile('filename.txt', { data: myBlob });
F) dir.createFile('filename.txt', { });
G) dir.createFile('filename.txt');


A) If the file already exists, produce an error. Otherwise create the file and write the contents of myBlob into the file.

B) Unconditionally create the file, overwrite the file contents if the file already exists. Write the string "filecontents" into the file.

C) If the file already exists, produce an error. Otherwise create the file and leave it empty.

D) Unconditionally create the file, overwrite the file contents if the file already exists. Leave the newly created file empty.

E) Same as A) (since 'fail' is used as default for ifExists)

F) Same as C) (since 'fail' is used as default for ifExists)

G) Same as C) (since 'fail' is used as default for ifExists)

Hope that explains things?

Note that implementation-wise the WebIDL generator should help with the handling of dictionaries.

Comment 23

[Dave Hylands [:dhylands]]

(In reply to Yuan Xulei [:yxl] from comment #18)
> (In reply to Dave Hylands [:dhylands] from comment #15)
...snip...
> > So I have an issue with exposing the root directory to content.
> > 
> > Currently, device storage abstracts away the top level directory by doing:
> > 
> > /storageName/somedir/somefile.txt
> > 
> > So content only sees /storageName. Internally, this gets mapped to the
> > actual file directory (for example /sdcard maps to /storage/emulated/legacy
> > on the Nexus4).
> > 
> > I would really like to continue along this vein and have the top level
> > directory remain abstract, and everything below that can come from the real
> > filesystem.
> > 
> > From a security standpoint, this also makes it easier to ensure that people
> > aren't accesing things that they shouldn't.
> > 
> > From a security standpoint, we definitely don't want to be passing fully
> > qualified paths from the child to the parent (we used to do this in device
> > storage, but now send storageName and relative path - see bug 860934)
> Filesystem API uses the similar mechanism to ensure the path security. Here
> is
> a short explanation about the path:
>  http://lists.w3.org/Archives/Public/public-script-coord/2013JulSep/0379.html
> 
> The path string passed to directory is relative and the content can only
> access the descendent files of a given directory. There are two constraints
> to
> enforce the security. 

So what are the paths relative to?

For DeviceStorage we can have multiple storage areas. How do you choose the storage area?

I can't really provide much more feedback on the webapi related stuff. I'm more than happy to review any of the implementation details underneath the webapi stuff.

So I'm going to remove the r? for me from the attachments for now.

Comment 24

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #23)
> (In reply to Yuan Xulei [:yxl] from comment #18)
> > (In reply to Dave Hylands [:dhylands] from comment #15)
> ...snip...
> > > So I have an issue with exposing the root directory to content.
> > > 
> > > Currently, device storage abstracts away the top level directory by doing:
> > > 
> > > /storageName/somedir/somefile.txt
> > > 
> > > So content only sees /storageName. Internally, this gets mapped to the
> > > actual file directory (for example /sdcard maps to /storage/emulated/legacy
> > > on the Nexus4).
> > > 
> > > I would really like to continue along this vein and have the top level
> > > directory remain abstract, and everything below that can come from the real
> > > filesystem.
> > > 
> > > From a security standpoint, this also makes it easier to ensure that people
> > > aren't accesing things that they shouldn't.
> > > 
> > > From a security standpoint, we definitely don't want to be passing fully
> > > qualified paths from the child to the parent (we used to do this in device
> > > storage, but now send storageName and relative path - see bug 860934)
> > Filesystem API uses the similar mechanism to ensure the path security. Here
> > is
> > a short explanation about the path:
> >  http://lists.w3.org/Archives/Public/public-script-coord/2013JulSep/0379.html
> > 
> > The path string passed to directory is relative and the content can only
> > access the descendent files of a given directory. There are two constraints
> > to
> > enforce the security. 
> 
> So what are the paths relative to?
The patchs are relative to the Directory that accept it as aguments.

> 
> For DeviceStorage we can have multiple storage areas. How do you choose the
> storage area?
>

We keep the entry of the original DeviceStorage and choose the storage area with
the original API:
  var storage = navigator.getDeviceStorage('storagearea');

The root directory is not the top level directory for all storage areas, but
has the same level with the corresponding storage area. We have multiple root
directories. Each storage area is mapped to a Directory object and the content
gets the mapped Directory by calling |storage.getRoot|. 

So for the device storage
path:
  /storageName/somedir/somefile.txt
The root is "/storageName", but not "/". To access the "somefile.txt", we should do:
  var storage = navigator.getDeviceStorage('storagearea');
  storage.getRoot(function(root) {
    root.get('somedir/somefile.txt');
  });

Will this resolve the root directory issue?

> I can't really provide much more feedback on the webapi related stuff. I'm
> more than happy to review any of the implementation details underneath the
> webapi stuff.
> 
> So I'm going to remove the r? for me from the attachments for now.

I'm sorry. Please focus on the implementation detail. I didn't mean to discuss the
WebAPI stuff here, but just wanted to provide the API details. I should have given
enough explanation about API details before asking for review. I'll write comments
in the WebIDL file and explain each function in as much detail as possible. Would
you like to continue to review?

Comment 25

[Dave Hylands [:dhylands]]

(In reply to Yuan Xulei [:yxl] from comment #24)
> (In reply to Dave Hylands [:dhylands] from comment #23)
> > (In reply to Yuan Xulei [:yxl] from comment #18)
> > > (In reply to Dave Hylands [:dhylands] from comment #15)
> > ...snip...

> > For DeviceStorage we can have multiple storage areas. How do you choose the
> > storage area?
> >
> 
> We keep the entry of the original DeviceStorage and choose the storage area
> with
> the original API:
>   var storage = navigator.getDeviceStorage('storagearea');
> 
> The root directory is not the top level directory for all storage areas, but
> has the same level with the corresponding storage area. We have multiple root
> directories. Each storage area is mapped to a Directory object and the
> content
> gets the mapped Directory by calling |storage.getRoot|. 
> 
> So for the device storage
> path:
>   /storageName/somedir/somefile.txt
> The root is "/storageName", but not "/". To access the "somefile.txt", we
> should do:
>   var storage = navigator.getDeviceStorage('storagearea');
>   storage.getRoot(function(root) {
>     root.get('somedir/somefile.txt');
>   });
> 
> Will this resolve the root directory issue?

So as long as querying root (i.e. asking for the path of root returns /storageName and not the underlying real directory name), then that addresses any security concerns I have.

> > I can't really provide much more feedback on the webapi related stuff. I'm
> > more than happy to review any of the implementation details underneath the
> > webapi stuff.
> > 
> > So I'm going to remove the r? for me from the attachments for now.
> 
> I'm sorry. Please focus on the implementation detail. I didn't mean to
> discuss the
> WebAPI stuff here, but just wanted to provide the API details. I should have
> given
> enough explanation about API details before asking for review. I'll write
> comments
> in the WebIDL file and explain each function in as much detail as possible.
> Would
> you like to continue to review?

Sure, I just figured I'd let you know that you'll want to line up somebody else for looking at the webapi aspects of this.

Thanks very much for the explanation above, it helps to provide context.

Comment 26

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #25)
> So as long as querying root (i.e. asking for the path of root returns
> /storageName and not the underlying real directory name), then that
> addresses any security concerns I have.
I'll make a fix about the root directory name.
> 
> > > I can't really provide much more feedback on the webapi related stuff. I'm
> > > more than happy to review any of the implementation details underneath the
> > > webapi stuff.
> > > 
> > > So I'm going to remove the r? for me from the attachments for now.
> > 
> > I'm sorry. Please focus on the implementation detail. I didn't mean to
> > discuss the
> > WebAPI stuff here, but just wanted to provide the API details. I should have
> > given
> > enough explanation about API details before asking for review. I'll write
> > comments
> > in the WebIDL file and explain each function in as much detail as possible.
> > Would
> > you like to continue to review?
> 
> Sure, I just figured I'd let you know that you'll want to line up somebody
> else for looking at the webapi aspects of this.
> 
Thanks for you help and reminding. I may ask Jonas or someone else to review
the webapi stuff. ;-)

Comment 27

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0001-WIP-Part1-v3-WebIDL-and-building.patch

Add full comments to dom/webidl/Directory.webidl about the WebAPI details.

@Jonas Could you review the comments of Directory.webid to make sure I understand the API spec correctly.

Comment 28

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0002-WIP-Part2-v3-Implement-getRoot-createDirectory-and-g.patch

The patch addresses comment 6.

Comment 29

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Diff-Part2-v2-v3

This is a rough diff for part2 between v2 and v3. It shows the main changes of part2 v3, though not all changes. 

Hope it helps.

Comment 30

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0003-WIP-Part3-v3-mochitest.patch

Add a mochitest file to test |createDirectory| function.

Comment 31

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Yuan Xulei [:yxl] from comment #28)
> Created attachment 831327 [details] [diff] [review]
> 0002-WIP-Part2-v3-Implement-getRoot-createDirectory-and-g.patch
> 
> The patch addresses comment 6.

Also querying the root directory name will return the device storage name and not the underlying real directory name.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 831324 [details] [diff] [review]
0001-WIP-Part1-v3-WebIDL-and-building.patch

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

Just looked over the webidl files. Looks good with the below nits.

You should probably have Baku review the Promise implementation. I'm a bit surprised that you don't need to inherit the existing Promise class?

::: dom/webidl/CreateFileOptions.webidl
@@ +5,5 @@
> + */
> +
> +dictionary CreateFileOptions {
> +  CreateIfExistsMode ifExists = "fail";
> +  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;

we should probably change this to

(DOMString or Blob or ArrayBuffer or ArrayBufferView)? data = null;

so that passing null is allowed.

@@ +6,5 @@
> +
> +dictionary CreateFileOptions {
> +  CreateIfExistsMode ifExists = "fail";
> +  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;
> +};

Move this into Directory.webidl with the other dictionaries.

::: dom/webidl/Directory.webidl
@@ +29,5 @@
> +   * If 'ifExists' is 'fail' and the path already exists, createFile must fail;
> +   * If 'ifExists' is 'replace', the path already exists, and is a file, create
> +   * a new file to replace the existing one;
> +   * If 'ifExists' is 'replace', the path already exists, but is a directory,
> +   * createFile must fail.

It's a good question what to do if the path exists but is a directory. I think failing is the right behavior, but we should get the spec clarified.

@@ +37,5 @@
> +   * File object. Otherwise, rejected with a DOM error.
> +   *
> +   * It should be
> +   *   Promise<File> createFile(DOMString path, CreateFileOptions options),
> +   * but gecko currently requires the last dictionary argument to be optional.

The second argument should be optional, sounds like the spec is wrong here.

@@ +70,5 @@
> +  Promise get(DOMString path);
> +
> +  /*
> +   * Moves (renames) a file or directory. The source and destination path
> +   * must be contained in current directory or its descendents.

Actually, the destination doesn't need to be a descendent. However a destination string can't contain '..' or start with '/'. So the following should all be forbidden:

move("filename", "../foo");
move("filename", "/foo");
move("filename", { name: "../foo", dir: myDir });
move("filename", { name: "/foo", dir: myDir });

@@ +77,5 @@
> +   * Directory object.
> +   * @param dest The destination. If a DOM string is passed, it is the relative
> +   * path to the destination file. If a Directory object is passed the source
> +   * will be moved to that directory. If a DestinationDict is passed, the parent
> +   * directory and the new name of the destination will be specified.

Change "will" to "must" here. Though maybe we should allow passing a dictionary with either a name or a dictionary specified too. Good question for the spec.

@@ +81,5 @@
> +   * directory and the new name of the destination will be specified.
> +   * @return If the source doesn't exist or the destination already exists, move
> +   * should fail and the returning AbortableProgressPromise is rejected with a
> +   * DOM error. During the operation, the current file being moved will be sent
> +   * as progress value of the AbortableProgressPromise.

Hmm.. It's a good question if moves should always fail if the destination exists. I could see allowing overwriting the destination if the destination is a file and not a directory.

Though always failing sounds like a safe start. Either way we should get this clarified in the spec.

@@ +95,5 @@
> +    move((DOMString or File or Directory) path,
> +         optional (DOMString or Directory or DestinationDict) dest);
> +
> +  /*
> +   * Deletes a file or an empty directory. The target should be a descendent of

Change "should" to "must"

@@ +102,5 @@
> +   * target. Otherwise, the File or Directory object of the target should be
> +   * passed.
> +   * @return If the target exists, the promise is resolved with boolean true or
> +   * false to indicate success. If the target is a non-empty directory or other
> +   * error occurrs, rejected with a DOM error.

The description here is a bit unclear. If the target is a non-empty directory, or if deleting the target fails, the promise is rejected with a DOM error. If the target did not exist, the promise is resolved with boolean false. If the target did exist and was successfully deleted, the promise is resolved with boolean true.

@@ +115,5 @@
> +   * @param path If a DOM string is passed, it is the relative path of the
> +   * target. Otherwise, the File or Directory object of the target should be
> +   * passed.
> +   * @return If the target exists, the promise is resolved with boolean true or
> +   * false to indicate success. Otherwise, rejected with a DOM error.

Same here. Resolved true means that it did exist and was deleted. Resolved false means that it did not exist. Rejected error means that an error occurred.

@@ +152,5 @@
> +   *
> +   * It should be
> +   *   Promise<FileHandleWritable> openWrite((DOMString or File) path,
> +   *                                         OpenWriteOptions options),
> +   * but gecko currently requires the last dictionary argument to be optional.

This should be optional in spec. So gecko is right and spec needs updating.

@@ +187,5 @@
> +callback VoidAnyCallback = void (optional any value);
> +interface AbortableProgressPromise : Promise {
> +  void abort();
> +  AbortableProgressPromise progress(VoidAnyCallback callback);
> +};

Move this into its own .webidl file since it's not specific to Directory.

Comment 33

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #32)
> Comment on attachment 831324 [details] [diff] [review]
> 0001-WIP-Part1-v3-WebIDL-and-building.patch
> 
> Review of attachment 831324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just looked over the webidl files. Looks good with the below nits.
> 
> You should probably have Baku review the Promise implementation. I'm a bit
> surprised that you don't need to inherit the existing Promise class?

OK, I will. I haven't implemented AbortableProgressPromise in this patch.
The corresponding class implementation are for passing the compiling, so don't inherit
Promise class yet.

> 
> @@ +6,5 @@
> > +
> > +dictionary CreateFileOptions {
> > +  CreateIfExistsMode ifExists = "fail";
> > +  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;
> > +};
> 
> Move this into Directory.webidl with the other dictionaries.

If moving this into Directory.webidl, I get a problem that two generated WebIDL binding
headers will include each others. So I have to leave CreateFileOptions in a sperate 
WebIDL file.

Comment 34

[Yuan Xulei [:yxl] (Leave from May 18)]

@Jonas, thanks for reviewing the WebIDL. I'll update the WebIDL later to address other parts of comment 32.

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > Move this into Directory.webidl with the other dictionaries.
> 
> If moving this into Directory.webidl, I get a problem that two generated
> WebIDL binding headers will include each others.

That sounds strange since that problem doesn't seem to affect any of the other dictionaries that are currently in Directory.webidl. What's different about CreateFileOptions?

Which two headers end up including each other?

Not a big deal, just was surprised.

Comment 36

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #35)
> > > Move this into Directory.webidl with the other dictionaries.
> > 
> > If moving this into Directory.webidl, I get a problem that two generated
> > WebIDL binding headers will include each others.
> 
> That sounds strange since that problem doesn't seem to affect any of the
> other dictionaries that are currently in Directory.webidl. What's different
> about CreateFileOptions?
> 
> Which two headers end up including each other?
If moving CreateFileOptions into Directory.webidl, the DirectoryBinding.h
(generated from Directory.webidl) and mozilla/dom/UnionTypes.h will include
 each others. 

Because 'CreateFileOptions' is declared in DirectoryBinding.h,
and requires including UnionTypes.h to declare the |data| property of union type
OwningStringOrBlobOrArrayBufferOrArrayBufferView, DirectoryBinding.h will
include UnionTypes.h.

The member function of Directory, such as 
 Promise<boolean> remove((DOMString or File or Directory) path),
has union type as argument and one memeber of the union is Directory, so declaring
these kinds of union type in UnionTypes.h needs to include UnionTypes.h.

Comment 37

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #32)
> ::: dom/webidl/CreateFileOptions.webidl
> @@ +5,5 @@
> > + */
> > +
> > +dictionary CreateFileOptions {
> > +  CreateIfExistsMode ifExists = "fail";
> > +  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;
> 
> we should probably change this to
> 
> (DOMString or Blob or ArrayBuffer or ArrayBufferView)? data = null;
> 
> so that passing null is allowed.
It seems the gecko doesn't allow to set a dictionary property as nullable.
If user don't use the data property, he/she can leave that propety unassigned.

Comment 38

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0001-WIP-Part1-v4-WebIDL-and-building.patch

The patch Addresses comment 32 of Jonas:
1. Fix the nits in the WebIDL comments.
2. Create the AbortableProgressPromise.webidl for AbortableProgressPromise and move its dummy implementation for dom/Directory to dom/Promise.

@Arun, please help to verify the spec description in dom/webidl/Directory.webidl. Thanks in advance.

Comment 39

[Dave Hylands [:dhylands]]

Comment on attachment 831327 [details] [diff] [review]
0002-WIP-Part2-v3-Implement-getRoot-createDirectory-and-g.patch

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

So I managed to get a chance to read some more of this. I really like the documentation in the TaskBase.h

I figured I'd post what comments I have so far.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3128,5 @@
> +  static const nsString kInvalidChars = NS_LITERAL_STRING(":\0");
> +#elif defined (XP_UNIX)
> +  static const nsString kInvalidChars = NS_LITERAL_STRING("\0");
> +#endif
> +  return kInvalidChars;

So I'm not sure that this is sufficient.

You've made the characters in a filename be OS dependent. But don't they also need to be filesystem dependent?

For example, under linux, the valid characters are different when storing into a file on the ext4 file system versus storing into a file on the VFAT file system. Real sdcards (on those phones that use them) are almost always VFAT. However, internal filesystems (like the Nexus 4) are typically done through a fuse-based filesystem. I'll need to check the list of valid characters available there.

You get the same problem on desktop, where you can have many different types of filesystems available.

@@ +3152,5 @@
> +  for (; cur < end; ++cur) {
> +    if (PRUnichar('\\') == *cur)
> +      *cur = PRUnichar('/');
> +  }
> +#endif

I see this duplicated in DeviceStorageFile::NormalizeFilePath

Perhaps this should be factored out into a standalone function?

::: dom/directory/CreateDirectoryTask.cpp
@@ +75,5 @@
> +void
> +CreateDirectoryTask::Work()
> +{
> +  // Resolve nsIFile from mTargetRealPath.
> +  nsCOMPtr<nsIFile> file = FilesystemUtils::CetLocalFile(mTargetRealPath);

typo: Was this supposed to be GetLocalFile?

It looks like we're passing a fully qualified filename from the child to the parent and that the parent is then just blindly using that path. This is fairly significant security problem. We have to assume that a child can be compromised.

We need to make sure that we only pass a relative path from the child to the parent and that the parent fills in the root portion of the pathname. We also need to ensure that relative path doesn't include any ..'s

::: dom/directory/FilesystemBase.h
@@ +15,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class FilesystemBase : public nsSupportsWeakReference

Could you explain (with a comment) why you're using a weak reference?

::: dom/directory/TaskBase.cpp
@@ +36,5 @@
> +
> +  if (IsParentProcess()) {
> +    // Run in parent process.
> +    if (!mWorkerThread) {
> +      nsresult rv = NS_NewThread(getter_AddRefs(mWorkerThread));

It seems to me that this will wind up launching alot of worker threads (or it may just be that since I don't really understand the lifetime of this particular object there may not be that many threads).

Does it make sense to use the LazyIdleThread (which will go away from time to time and need to respawned, but if it already exists, then it would just use that)
Spawning lots of threads is fairly expensive in terms of memory used for the stack, which is where my concern stems from.

@@ +46,5 @@
> +        return;
> +      }
> +      // Start worker thread
> +      mWorkerThread->Dispatch(this, NS_DISPATCH_NORMAL);
> +    }

nit: Add return and unindent the rest of this function

@@ +66,5 @@
> +  if (!NS_IsMainThread()) {
> +    // Run worker thread tasks
> +    Work();
> +    // Dispatch itself to main thread
> +    NS_DispatchToMainThread(this);

nit: return NS_OK here and you can unindent the rest of this function.

@@ +97,5 @@
> +{
> +  MOZ_ASSERT(IsParentProcess(), "Only call from parent process!");
> +  if (HasError()) {
> +    return FilesystemErrorResponse(mErrorName);
> +  } else {

Drop the else and unindent the next line

Comment 40

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #39)
> Comment on attachment 831327 [details] [diff] [review]
> 0002-WIP-Part2-v3-Implement-getRoot-createDirectory-and-g.patch
> 
> Review of attachment 831327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I managed to get a chance to read some more of this. I really like the
> documentation in the TaskBase.h
> 
> I figured I'd post what comments I have so far.
> 
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +3128,5 @@
> > +  static const nsString kInvalidChars = NS_LITERAL_STRING(":\0");
> > +#elif defined (XP_UNIX)
> > +  static const nsString kInvalidChars = NS_LITERAL_STRING("\0");
> > +#endif
> > +  return kInvalidChars;
> 
> So I'm not sure that this is sufficient.
> 
> You've made the characters in a filename be OS dependent. But don't they
> also need to be filesystem dependent?

It is a problem and it needs to be filesystem dependent. I should reconsider
a way to do this check.

> 
> For example, under linux, the valid characters are different when storing
> into a file on the ext4 file system versus storing into a file on the VFAT
> file system. Real sdcards (on those phones that use them) are almost always
> VFAT. However, internal filesystems (like the Nexus 4) are typically done
> through a fuse-based filesystem. I'll need to check the list of valid
> characters available there.
> 
> You get the same problem on desktop, where you can have many different types
> of filesystems available.
> 
> @@ +3152,5 @@
> > +  for (; cur < end; ++cur) {
> > +    if (PRUnichar('\\') == *cur)
> > +      *cur = PRUnichar('/');
> > +  }
> > +#endif
> 
> I see this duplicated in DeviceStorageFile::NormalizeFilePath
> 
> Perhaps this should be factored out into a standalone function?

I'll add functions to deal with local path conversion.

> 
> ::: dom/directory/CreateDirectoryTask.cpp
> @@ +75,5 @@
> > +void
> > +CreateDirectoryTask::Work()
> > +{
> > +  // Resolve nsIFile from mTargetRealPath.
> > +  nsCOMPtr<nsIFile> file = FilesystemUtils::CetLocalFile(mTargetRealPath);
> 
> typo: Was this supposed to be GetLocalFile?

Yes, a typo.

> 
> It looks like we're passing a fully qualified filename from the child to the
> parent and that the parent is then just blindly using that path. This is
> fairly significant security problem. We have to assume that a child can be
> compromised.
> 
> We need to make sure that we only pass a relative path from the child to the
> parent and that the parent fills in the root portion of the pathname. We
> also need to ensure that relative path doesn't include any ..'s
> 

OK, I'll make change to enforce the security.

> ::: dom/directory/FilesystemBase.h
> @@ +15,5 @@
> > +
> > +namespace mozilla {
> > +namespace dom {
> > +
> > +class FilesystemBase : public nsSupportsWeakReference
> 
> Could you explain (with a comment) why you're using a weak reference?

The comment will be added to the updated path. The reason I use a weak reference
is that when the child window is closed, the child doesn't need to cancel all
background tasks and wait until the they finish. If a background task find
FilesystemBase is null, it should exit silently.
> 
> ::: dom/directory/TaskBase.cpp
> @@ +36,5 @@
> > +
> > +  if (IsParentProcess()) {
> > +    // Run in parent process.
> > +    if (!mWorkerThread) {
> > +      nsresult rv = NS_NewThread(getter_AddRefs(mWorkerThread));
> 
> It seems to me that this will wind up launching alot of worker threads (or
> it may just be that since I don't really understand the lifetime of this
> particular object there may not be that many threads).

A task will launch only one worker thread. Everytime we calling |Directory.createDirectory 
(or |Directory.get|) creates a new task and hence lauches its worker thread.

> 
> Does it make sense to use the LazyIdleThread (which will go away from time
> to time and need to respawned, but if it already exists, then it would just
> use that)
> Spawning lots of threads is fairly expensive in terms of memory used for the
> stack, which is where my concern stems from.

I should use LazyIdleThread.

Comment 41

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: 0002-WIP-Part2-v4-Implement-getRoot-createDirectory-and-get.patch

This patch addresses all parts of Comment 39 except that passing relative path  instead of full path from the child to the parent. I'm going to make another patch to address the passing relative path problem.

The patch fixes the nits and makes following changes:

1. Introduces two functions to help convert the path separators between local file path and dom file path.

2. Uses thread pool to manager threads without creating and destroying threads frequently.

Comment 42

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Diff-Part2-v3-v4.patch

The diff of part2 between v4 and v3 to help review.

Comment 43

[Jan Varga [:janv]]

It would be interesting to hear how you want to integrate with FileService once you get to implementing FileHandle parts of the FileSystem API.

I'm asking because I quickly went through the patches and it seems you are creating a new thread pool.
I would also suggest to use a better name for new dom/ subdirectory. Maybe dom/filesystem ?

Comment 44

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Varga [:janv] from comment #43)
> It would be interesting to hear how you want to integrate with FileService
> once you get to implementing FileHandle parts of the FileSystem API.
I haven't considered about the implementing that parts?
> 
> I'm asking because I quickly went through the patches and it seems you are
> creating a new thread pool.
Yes, I create a new thread pool. Maybe we can share the same thread pool with FileService?
Any suggestion?
> I would also suggest to use a better name for new dom/ subdirectory. Maybe
> dom/filesystem ?
It seems dom/filesystem is better than dom/directory. If I changed the directory to dom/filesystem, should I create the namespace dom::mozilla:filesystem?

Comment 45

[Jan Varga [:janv]]

(In reply to Yuan Xulei [:yxl] from comment #44)
> Yes, I create a new thread pool. Maybe we can share the same thread pool
> with FileService?
> Any suggestion?

Well, this will require much more thinking once we get to the FileHandle stuff, I just wanted to be sure you were aware of it.

> It seems dom/filesystem is better than dom/directory. If I changed the
> directory to dom/filesystem, should I create the namespace
> dom::mozilla:filesystem?

Yeah, I think dom/filesystem is better, I would also use mozilla::dom::filesystem as a C++ namespace for it.

Comment 46

[Dave Hylands [:dhylands]]

Comment on attachment 8336609 [details] [diff] [review]
0002-WIP-Part2-v4-Implement-getRoot-createDirectory-and-get.patch

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

So I went over everything again, and this looks good to me.

I think it will be important to test everything on the Nexus 4 (i.e. JB) to make sure that everything works as expected.

::: dom/directory/CreateDirectoryTask.cpp
@@ +92,5 @@
> +    SetError(FilesystemUtils::DOM_ERROR_PATH_EXISTS);
> +    return;
> +  }
> +
> +  rv = file->Create(nsIFile::DIRECTORY_TYPE, 0700);

Using 0700 could leave the file unaccessible. Files created on sdcard should have at least group privileges, since sdcard_r and sdcard_rw are groups.

This will also fail (under JB) if executed in the child The child processes have no privileges to open/create files on the sdcard - so all file opens/directory/file creation/deletion has to take place in the parent.
Bug 910498 adds support for the one place in DeviceStorage where we used to open a file in the child.

Comment 47

[Dave Hylands [:dhylands]]

Comment on attachment 8333645 [details] [diff] [review]
0001-WIP-Part1-v4-WebIDL-and-building.patch

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

I don't think that I have enough experience with this code to give it r+ for review. It looks fine to me, so I'm comfortable giving it feedback+.

Comment 48

[Dave Hylands [:dhylands]]

Comment on attachment 831330 [details] [diff] [review]
0003-WIP-Part3-v3-mochitest.patch

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

Looks reasonable to me.

Comment 49

[Dave Hylands [:dhylands]]

Comment on attachment 8333645 [details] [diff] [review]
0001-WIP-Part1-v4-WebIDL-and-building.patch

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

Whoops. Meant to do feedback+

Comment 50

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v1 WebIDL and DOM binding for filesystem API

Fix the implementation of AbortableProgressPromise to inherit Promise.

@Jan, could you help to review the patch?

Comment 51

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v1 Implement getRoot createDirectory and get for Directory.

The patch fixes:

1. Create directory with 0777 mode bits instead of 0700.
2. Use virtual DOM path instead of the full local path to pass file path between child and parent.
3. Share the same thread pool with stream transport service.
4. Move the files to dom/filesystem directory. But I still use the namespace mozilla::dom, as it is the default namespace for IPC and dom binding related code and changing it to mozilla::dom::filesystem causes troubles.

Comment 52

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part3 v1 Basic tests for filesystem API

Rebased mochitest. Nothing has been changed with the previous patch.

Comment 53

[Dave Hylands [:dhylands]]

Comment on attachment 8354982 [details] [diff] [review]
Part1 v1 WebIDL and DOM binding for filesystem API

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

I can't really provide any useful feedabck on the AbortableProgressPromise stuff, so I'm just going to clear the feedback flag.

Comment 54

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8354982 [details] [diff] [review]
Part1 v1 WebIDL and DOM binding for filesystem API

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

@baku, I want to implement the AbortableProgressPromise which inherits Promise. Could you help to review the related WebIDL and dom binding? Thank you.

Comment 55

[Andrea Marchesini [:baku]]

Comment on attachment 8354982 [details] [diff] [review]
Part1 v1 WebIDL and DOM binding for filesystem API

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

The WebIDL part looks good but I want to have a comment from annevk about AbortProgressPromise.
There was a discussion about such kind of Promise so probably he can give good advices.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3220,5 @@
>  }
>  
> +already_AddRefed<Promise>
> +nsDOMDeviceStorage::GetRoot()
> +{

why this method?

::: dom/filesystem/Directory.cpp
@@ +10,5 @@
> +#include "nsStringGlue.h"
> +
> +#include "mozilla/dom/AbortableProgressPromise.h"
> +#include "mozilla/dom/DirectoryBinding.h"
> +#include "mozilla/dom/Promise.h"

remove this.

@@ +35,5 @@
> +
> +nsPIDOMWindow*
> +Directory::GetParentObject() const
> +{
> +  return nullptr;

can we have a parent for this object?

@@ +46,5 @@
> +}
> +
> +void
> +Directory::GetName(nsString& retval) const
> +{

aRetval.Truncate();
// TODO

@@ +51,5 @@
> +}
> +
> +already_AddRefed<Promise>
> +Directory::CreateFile(const nsAString& path, const CreateFileOptions& options)
> +{

// TODO

@@ +57,5 @@
> +}
> +
> +already_AddRefed<Promise>
> +Directory::CreateDirectory(const nsAString& aPath)
> +{

// TODO

::: dom/filesystem/Directory.h
@@ +38,5 @@
> +  ~Directory();
> +
> +  // ========= Begin WebIDL bindings. ===========
> +
> +  nsPIDOMWindow* GetParentObject() const;

returnType
method();

here and everywhere in the .h files

@@ +43,5 @@
> +
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  void GetName(nsString& retval) const;

aRetval (and the same pattern everywhere)

@@ +45,5 @@
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  void GetName(nsString& retval) const;
> +
> +  already_AddRefed<Promise> CreateFile(const nsAString& path, const CreateFileOptions& options);

80chars?

@@ +51,5 @@
> +  already_AddRefed<Promise> CreateDirectory(const nsAString& path);
> +
> +  already_AddRefed<Promise> Get(const nsAString& aPath);
> +
> +  already_AddRefed<AbortableProgressPromise> Move(const StringOrFileOrDirectory& path, const StringOrDirectoryOrDestinationDict& dest);

this is definitely not 80chars :)

@@ +67,5 @@
> +
> +  // Mark this as resultNotAddRefed to return raw pointers
> +  already_AddRefed<EventStream> EnumerateDeep(const Optional<nsAString >& path);
> +
> +  // =========== End WebIDL bindings.============

remove this... nothing is after this

::: dom/filesystem/EventStream.cpp
@@ +30,5 @@
> +
> +EventStream*
> +EventStream::GetParentObject() const
> +{
> +  return nullptr;

something here.

@@ +40,5 @@
> +  return EventStreamBinding::Wrap(aCx, aScope, this);
> +}
> +
> +already_AddRefed<Promise>
> +EventStream::ForEach(ForEachCallback& callback)

aCallback + TODO

::: dom/filesystem/EventStream.h
@@ +20,5 @@
> +
> +class Promise;
> +class ForEachCallback;
> +
> +class EventStream MOZ_FINAL : public nsISupports /* Change nativeOwnership in the binding configuration if you don't want this */,

remove these comments

@@ +32,5 @@
> +  EventStream();
> +
> +  ~EventStream();
> +
> +  // TODO: return something sensible here, and change the return type

this comment should go away.

@@ +35,5 @@
> +
> +  // TODO: return something sensible here, and change the return type
> +  EventStream* GetParentObject() const;
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;

In any .h file:

returnType
Method();

@@ +38,5 @@
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  // Mark this as resultNotAddRefed to return raw pointers
> +  already_AddRefed<Promise> ForEach(ForEachCallback& callback);

already_AddRefed<Promise>
ForEach(ForEachCallback& aCallback);

::: dom/promise/AbortableProgressPromise.cpp
@@ +9,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +#define NS_IMPL_CYCLE_COLLECTION_INHERITED_0(_class, _base)                    \

move this in nsCycleCollectionParticipant.h

@@ +46,5 @@
> +}
> +
> +// Mark this as resultNotAddRefed to return raw pointers
> +already_AddRefed<AbortableProgressPromise>
> +AbortableProgressPromise::Progress(VoidAnyCallback& callback)

aCallback

::: dom/webidl/Directory.webidl
@@ +87,5 @@
> +   * but gecko currently requires the last union argument with Dictionary
> +   * memeber to be optional.
> +   */
> +  [NewObject]
> +  AbortableProgressPromise

You should talk with annevk about this new type of promise.

Comment 56

[Anne (:annevk)]

See https://twitter.com/jaffathecake/status/413240852333359105 for some discussion on promises that can be terminated.

Comment 57

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Andrea Marchesini (:baku) from comment #55)
> Comment on attachment 8354982 [details] [diff] [review]
> Part1 v1 WebIDL and DOM binding for filesystem API
> 
> Review of attachment 8354982 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The WebIDL part looks good but I want to have a comment from annevk about
> AbortProgressPromise.
> There was a discussion about such kind of Promise so probably he can give
> good advices.
> 
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +3220,5 @@
> >  }
> >  
> > +already_AddRefed<Promise>
> > +nsDOMDeviceStorage::GetRoot()
> > +{
> 
> why this method?

We want to expose the filesystem API thought DeviceStorage interface. See comment 4 for details (https://bugzilla.mozilla.org/show_bug.cgi?id=910412#c4).

> 
> ::: dom/filesystem/Directory.cpp
> @@ +10,5 @@
> > +#include "nsStringGlue.h"
> > +
> > +#include "mozilla/dom/AbortableProgressPromise.h"
> > +#include "mozilla/dom/DirectoryBinding.h"
> > +#include "mozilla/dom/Promise.h"
> 
> remove this.
> 
> @@ +35,5 @@
> > +
> > +nsPIDOMWindow*
> > +Directory::GetParentObject() const
> > +{
> > +  return nullptr;
> 
> can we have a parent for this object?

Yes, but not in this patch. The next patch () will return a window as parent.

> @@ +67,5 @@
> > +
> > +  // Mark this as resultNotAddRefed to return raw pointers
> > +  already_AddRefed<EventStream> EnumerateDeep(const Optional<nsAString >& path);
> > +
> > +  // =========== End WebIDL bindings.============
> 
> remove this... nothing is after this
The next patch will add something after this :-)
> 
> ::: dom/filesystem/EventStream.cpp
> @@ +30,5 @@
> > +
> > +EventStream*
> > +EventStream::GetParentObject() const
> > +{
> > +  return nullptr;
> 
> something here.
> 
I'm going to implement EventStream in another bug (bug 935883), not in this bug.
So I have nothing to return. I'll add a TOTO comment here to remind me to return
something as parent.
 
> ::: dom/webidl/Directory.webidl
> @@ +87,5 @@
> > +   * but gecko currently requires the last union argument with Dictionary
> > +   * memeber to be optional.
> > +   */
> > +  [NewObject]
> > +  AbortableProgressPromise
> 
> You should talk with annevk about this new type of promise.
Thanks. I'm going to discuss and implement AbortableProgressPromise in bug 935883.
In this bug, I'll add a dummy interface with basic interface members to pass the compiling.

Comment 58

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Anne (:annevk) from comment #56)
> See https://twitter.com/jaffathecake/status/413240852333359105 for some
> discussion on promises that can be terminated.

Thanks for link.

Comment 59

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v2 WebIDL and DOM binding for filesystem API

@baku, thank you.

The patch addresses all of comment 55 except a few that I explained in comment 57.

Comment 60

[Dave Hylands [:dhylands]]

Comment on attachment 8354985 [details] [diff] [review]
Part2 v1 Implement getRoot createDirectory and get for Directory.

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

Looks good.

Overall comment. All file operations can only be done from the parent (getting file size, checking existence of a file, etc), so I'd like to see asserts to verify that we're in the parent for all of these. The file operations may work in the child on pre-JB phones, but they won't work on JB or newer (i.e. Nexus 4 ot 5).

So for now, I'm just clearing the review flag.

I also had some concerns about only seeming to use the default storage area? (or maybe I'm just missing something)

::: dom/base/domerr.msg
@@ +69,5 @@
>  
>  DOM_MSG_DEF(NS_ERROR_DOM_INDEXEDDB_RECOVERABLE_ERR, "The operation failed because the database was prevented from taking an action. The operation might be able to succeed if the application performs some recovery steps and retries the entire transaction. For example, there was not enough remaining storage space, or the storage quota was reached and the user declined to give more space to the database.")
>  
> +/* Filesystem DOM errors. */
> +DOM4_MSG_DEF(InvalidAccessError, "Invalid file system path.", NS_ERROR_DOM_FILESYSTEM_INVALID_PATH_ERR)  

nit: trailing space

::: dom/devicestorage/DeviceStorage.h
@@ +326,5 @@
>        DEVICE_STORAGE_TYPE_SHARED,
>        DEVICE_STORAGE_TYPE_EXTERNAL
>    };
> +
> +  nsCOMPtr<DeviceStorageFilesystem> mFilesystem;

nit: My understanding was that you should use nsCOMPtr for nsIXxx classes, and use nsRefPtr for concrete classes (DeviceStorageFileSystem seems to be a concrete class)

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3205,5 @@
>  {
> +  if (!mFilesystem) {
> +    mFilesystem = new DeviceStorageFilesystem(mStorageType, GetOwner());
> +  }
> +  return mozilla::dom::Directory::GetRoot(mFilesystem);

Each nsDOMDeviceStorage object is for a particular storageName.

But this creates a DeviceStorageFileSystem only for the default storage area, which doesn't seem right to me.

Each named storage area will have a different root.

::: dom/filesystem/DeviceStorageFilesystem.cpp
@@ +26,5 @@
> +  if (!DeviceStorageTypeChecker::IsVolumeBased(mStorageType)) {
> +    // The storage name will be the empty string
> +    mStorageName.Truncate();
> +  } else {
> +    nsDOMDeviceStorage::GetDefaultStorageName(mStorageType, mStorageName);

So why is this using the default storage name?

The default storage name is controlled by the user (through the settings app) and can change over time.

If there are multiple storage areas, will this allow them to all be used? Or just the default one?

::: dom/filesystem/FilesystemFile.cpp
@@ +88,5 @@
> +    nsCOMPtr<nsIFile> file = aFilesystem->GetLocalFile(mPath);
> +    if (NS_WARN_IF(!file)) {
> +      return JSVAL_NULL;
> +    }
> +    nsCOMPtr<nsIDOMFile> domFile = new nsDOMFileFile(file);

File operations are only guaranteed to work in the parent (they happen to work on some phones in the child, but in general they only work in the parent). Can we assert that we're in the parent?

If you're only planning on doing filename manipulations in the child then you're ok, but you won't be able to do things like domFile->GetSize() or domFile->GetLastModifiedDate() from the child on JB or newer.

::: dom/filesystem/FilesystemFile.h
@@ +34,5 @@
> +  void GetName(nsAString& aName) const;
> +
> +  JS::Value ToJsValue(JSContext* cx, FilesystemBase* aFilesystem) const;
> +private:
> +  static const PRUnichar kSeparatorChar = PRUnichar('/');

nit: Use char16_t rather than PRUnichar

::: dom/filesystem/FilesystemRequestParent.cpp
@@ +33,5 @@
> +    case FilesystemParams::TFilesystemCreateDirectoryParams: {
> +      FilesystemCreateDirectoryParams& p = mParams;
> +      mFilesystem = FilesystemBase::FromString(p.filesystem());
> +      MOZ_ASSERT(mFilesystem, "Filesystem should not be null.");
> +      task = new CreateDirectoryTask(mFilesystem, p, this);

It seems kind of weird to me to create a new object just to start an operation.

Wouldn't it make more sense to just have a helper function?

This looks weird because you create the task object and don't do anything with it except to destruct it.

Even pulling the Start out of the CreateDirectoryTask constructor would make this "look" better. Then at least you'd be doing:

task = new CreateDirectoryTask(...)
task->Start();

You're probably getting compiler warning about not using the task variable.

@@ +41,5 @@
> +    case FilesystemParams::TFilesystemGetFileOrDirectoryParams: {
> +      FilesystemGetFileOrDirectoryParams& p = mParams;
> +      mFilesystem = FilesystemBase::FromString(p.filesystem());
> +      MOZ_ASSERT(mFilesystem, "Filesystem should not be null.");
> +      task = new GetFileOrDirectoryTask(mFilesystem, p, this);

Similar comment here

::: dom/filesystem/FilesystemUtils.cpp
@@ +25,5 @@
> +  if (NS_WARN_IF(!globalObject)) {
> +    return JSVAL_NULL;
> +  }
> +
> +  JS::Rooted<JSObject*> global(cx, globalObject->GetGlobalJSObject());

Should you assert cx to be non-NULL?

@@ +27,5 @@
> +  }
> +
> +  JS::Rooted<JSObject*> global(cx, globalObject->GetGlobalJSObject());
> +
> +  return OBJECT_TO_JSVAL(aObject->WrapObject(cx, global));

Should you assert aObject to be non-NULL?

@@ +66,5 @@
> +  nsString result;
> +  result = aLocal;
> +#if defined(XP_WIN)
> +  PRUnichar* cur = result.BeginWriting();
> +  PRUnichar* end = result.EndWriting();

nit: PRUnichar needs to be changed to char16_t

See: https://groups.google.com/forum/#!topic/mozilla.dev.platform/HMIS6Wb45sY

::: dom/filesystem/GetFileOrDirectoryTask.cpp
@@ +90,5 @@
> +    return;
> +  }
> +
> +  bool ret;
> +  nsresult rv = file->Exists(&ret);

This only works in the parent, so lets assert that we're in the parent.

Comment 61

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #60)
> Comment on attachment 8354985 [details] [diff] [review]
> Part2 v1 Implement getRoot createDirectory and get for Directory.
> 
> Review of attachment 8354985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> Overall comment. All file operations can only be done from the parent
> (getting file size, checking existence of a file, etc), so I'd like to see
> asserts to verify that we're in the parent for all of these. The file
> operations may work in the child on pre-JB phones, but they won't work on JB
> or newer (i.e. Nexus 4 ot 5).
> 
> So for now, I'm just clearing the review flag.
> 
> I also had some concerns about only seeming to use the default storage area?
> (or maybe I'm just missing something)
Dave, thank you:-) I'm quite happy to get your reply.

That's true. Is it a problem that the patch only uses the default storage? I will correct 
it soon as well as other parts.

Comment 62

[Dave Hylands [:dhylands]]

(In reply to Yuan Xulei [:yxl] from comment #61)
> (In reply to Dave Hylands [:dhylands] from comment #60)
> > Comment on attachment 8354985 [details] [diff] [review]
> > Part2 v1 Implement getRoot createDirectory and get for Directory.
> > 
> > Review of attachment 8354985 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good.
> > 
> > Overall comment. All file operations can only be done from the parent
> > (getting file size, checking existence of a file, etc), so I'd like to see
> > asserts to verify that we're in the parent for all of these. The file
> > operations may work in the child on pre-JB phones, but they won't work on JB
> > or newer (i.e. Nexus 4 ot 5).
> > 
> > So for now, I'm just clearing the review flag.
> > 
> > I also had some concerns about only seeming to use the default storage area?
> > (or maybe I'm just missing something)
> Dave, thank you:-) I'm quite happy to get your reply.
> 
> That's true. Is it a problem that the patch only uses the default storage? I
> will correct 
> it soon as well as other parts.

Since DeviceStorage allows access to all of the storage areas, not just the default one, I assumed that the FileSystem API should allow access to all of the storage areas as well.

Especially, since the location of the default storage area is changable by the user, so when you start your app using the FileSystem API, the default storage area might be in one place, and then the user changes it in the settings app, and goes back to the app using the FileSystem API, things will probably be very confused.

The Gallery, Music, and Video apps scan all of the storage areas not just the default one. The default storage area is primarily used as the location to put new files, since the user doesn't have any other way of specifying that.

But the user is probably interested in seeing all of their files.

Comment 63

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v2 WebIDL and DOM binding for filesystem API

Almost the same with the previous version and just fix a few nits about the code format.

Comment 64

[Jan Varga [:janv]]

I'll take a look once I finish other reviews.

Comment 65

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v2 Implement getRoot createDirectory and get for Directory.

Fix all items of comment 60:

1. Format code, remove trailing spaces and add asserts.

2. Use nsRefPtr for concrete classes

3. Support multiple storage areas

4. All file operations are performed in the parent. DOMFile will be created in the parent instead of the child and returned to child as blob by IPC.

5. Use char16_t rather than PRUnichar

6. Add static helper functions on TaskBase for creating TaskBase subclass instances.

7. Merge FilesystemFile into Directory and remove FilesystemFile class.

8. Create root the directory if not exits. See GetFileOrDirectoryTask::Work.

Comment 66

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part3 v2 Basic tests for filesystem API

Add test_file_get.html to test Directory#get.

Comment 67

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Varga [:janv] from comment #64)
> I'll take a look once I finish other reviews.

Thank you :)

Comment 68

[Dave Hylands [:dhylands]]

Comment on attachment 8358260 [details] [diff] [review]
Part2 v2 Implement getRoot createDirectory and get for Directory.

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

::: dom/filesystem/FilesystemBase.cpp
@@ +17,5 @@
> +already_AddRefed<FilesystemBase>
> +FilesystemBase::FromString(const nsAString& aString)
> +{
> +  if (StringBeginsWith(aString, NS_LITERAL_STRING("devicestorage-"))) {
> +    // The string representation of devicestroage file system is of the format:

nit: typo (devicestroage should be devicestorage)

Comment 69

[Dave Hylands [:dhylands]]

Comment on attachment 8358261 [details] [diff] [review]
Part3 v2 Basic tests for filesystem API

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

::: dom/devicestorage/test/test_fs_createDirectory.html
@@ +74,5 @@
> +      d.createDirectory('..').then(function(what) {
> +        ok(false, "Should not overwrite an existing directory.");
> +        devicestorage_cleanup();
> +      }, function(e) {
> +        ok(true, "Accessubg parent directory with '..' is not allowed.");

nit: typo

Comment 70

[Jan Varga [:janv]]

Ok, I have some time now, where does the spec live these days, http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface ?

Comment 71

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Varga [:janv] from comment #70)
> Ok, I have some time now, where does the spec live these days,
> http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface ?
Just in the comments of the Directory.webidl file of first patch (Part1 v2 WebIDL and DOM binding for filesystem API).
It is a draft version based on my understanding and reviewed by Jonas in this bug (see comment 22 and comment 32).

AFAIK, we don't have a more detailed one than http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface.

Comment 72

[Dave Hylands [:dhylands]]

Comment on attachment 8358253 [details] [diff] [review]
Part1 v2 WebIDL and DOM binding for filesystem API

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

::: dom/webidl/Directory.webidl
@@ +89,5 @@
> +   */
> +  [NewObject]
> +  AbortableProgressPromise
> +    move((DOMString or File or Directory) path,
> +         optional (DOMString or Directory or DestinationDict) dest);

Question: If the source and dest are in different storage areas, then this needs to be implemented as a copy/delete. If the source and destination are in the same storage area then this can be a rename or move. So I just thought I'd ask if this had been considered?

Comment 73

[Jan Varga [:janv]]

(In reply to Dave Hylands [:dhylands] from comment #72)
> Comment on attachment 8358253 [details] [diff] [review]
> Part1 v2 WebIDL and DOM binding for filesystem API
> 
> Review of attachment 8358253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Directory.webidl
> @@ +89,5 @@
> > +   */
> > +  [NewObject]
> > +  AbortableProgressPromise
> > +    move((DOMString or File or Directory) path,
> > +         optional (DOMString or Directory or DestinationDict) dest);
> 
> Question: If the source and dest are in different storage areas, then this
> needs to be implemented as a copy/delete. If the source and destination are
> in the same storage area then this can be a rename or move. So I just
> thought I'd ask if this had been considered?

we need to document this, since the former isn't atomic

Comment 74

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #72)
> Comment on attachment 8358253 [details] [diff] [review]
> Part1 v2 WebIDL and DOM binding for filesystem API
> 
> Review of attachment 8358253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Directory.webidl
> @@ +89,5 @@
> > +   */
> > +  [NewObject]
> > +  AbortableProgressPromise
> > +    move((DOMString or File or Directory) path,
> > +         optional (DOMString or Directory or DestinationDict) dest);
> 
> Question: If the source and dest are in different storage areas, then this
> needs to be implemented as a copy/delete. If the source and destination are
> in the same storage area then this can be a rename or move. So I just
> thought I'd ask if this had been considered?

Yes, I had a rough thought about that in bug935883. See https://bugzilla.mozilla.org/show_bug.cgi?id=935883#c1

Comment 75

[Yuan Xulei [:yxl] (Leave from May 18)]

@baku, I want to land this bug with the following parts implemented first:

interface DeviceStorage {
  Promise<Directory> getRoot();
};

interface Directory {
  Promise<Directory> createDirectory(DOMString path);
  Promise<(File or Directory)> get(DOMString path);
};

Other parts including the AbortableProgressPromise are going to be discussed and implemented in follow up bugs. I marked them as TODOs in this bug's patch and made dummy implementation for them to pass compiling.

dhylands has reviewed the last two patches. Could you help to review and verify the WebIDL and dom binding implementation of of the first patch (Part1 v2 WebIDL and DOM binding for filesystem API)?

Comment 76

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That sounds like an ok start to me. Though please don't land functions with dummy implementations. Just remove the functions from the WebIDL instead. That way applications can more easily test which functions are available and which ones aren't.

Comment 77

[Yuan Xulei [:yxl] (Leave from May 18)]

OK, I'll remove the dummy implementations and update the patches.

Comment 78

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v3 WebIDL and DOM binding for filesystem API

Remove unimplemented functions from WebIDL.

Comment 79

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v3 Implement getRoot createDirectory and get for Directory.

Fix nits

Comment 80

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part3 v3 Basic tests for filesystem API

Fix nits.

Comment 81

[Andrea Marchesini [:baku]]

Comment on attachment 8373944 [details] [diff] [review]
Part1 v3 WebIDL and DOM binding for filesystem API

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

It's unfinished. I cannot give you a r+. Can I see the rest of the code when written? Thanks!

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3475,5 @@
>  }
>  
> +already_AddRefed<Promise>
> +nsDOMDeviceStorage::GetRoot()
> +{

TODO

::: dom/filesystem/Directory.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "Directory.h"

mozilla/dom/Directory.h

@@ +7,5 @@
> +#include "Directory.h"
> +
> +#include "nsStringGlue.h"
> +
> +#include "mozilla/dom/DirectoryBinding.h"

move it to line 8

@@ +52,5 @@
> +
> +already_AddRefed<Promise>
> +Directory::CreateDirectory(const nsAString& aPath)
> +{
> +  // TODO

This will fail. You cannot return nullptr without an error. But I think it's enough for now. New patches will come, I guess.

::: dom/filesystem/Directory.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_Directory_h__

remove __

Comment 82

[Andrea Marchesini [:baku]]

Comment on attachment 8373944 [details] [diff] [review]
Part1 v3 WebIDL and DOM binding for filesystem API

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

Ok I see. there are other patches :)

Comment 83

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Andrea Marchesini (:baku) from comment #81)
...
> 
> ::: dom/filesystem/Directory.h
> @@ +3,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#ifndef mozilla_dom_Directory_h__
> 
> remove __

I found some macros of the header files under the dom directory have the "__"(2 underscores) suffix, or even "___"(3 underscores), but others don't.
When should I add the "__" suffix? I checked the code style guide on MDN (https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style) and didn't find any explanation.

Comment 84

[Dave Hylands [:dhylands]]

Look in this section: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#C.2FC.2B.2B_practices

The 11th bullet item details include guards, and following that guideine neither trailing nor leading underscores should be used.

Comment 85

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v4 WebIDL and DOM binding for filesystem API

Fix issues in Comment 81.

Comment 86

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v4 Implement getRoot createDirectory and get for Directory.

Fix include guards.

Comment 87

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part4 v1 Fix DOM API test.

Filesystem API introduces a new WebIDL interface - Directory. Add the new interface to test_interfaces.html.

Comment 88

[Boris Zbarsky [:bzbarsky]]

Can you please link me to the spec in question (and add that link to Directory.webidl as well)?  Is it stable enough that we want to be exposing this to web content?

Comment 89

[Boris Zbarsky [:bzbarsky]]

Also, if this is _not_ stable and only needed for DeviceStorage stuff, we should either make it NoInterfaceObject or at least hide it when DeviceStorage is not available.  See bug 971732.

Comment 90

[Yuan Xulei [:yxl] (Leave from May 18)]

@bz, this is not stable and we only have an editor's draft spec:
http://w3c.github.io/filesystem-api/Overview.html#the-directory-interface

Comment 91

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8374677 [details] [diff] [review]
Part2 v4 Implement getRoot createDirectory and get for Directory.

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

::: dom/filesystem/GetFileOrDirectoryTask.cpp
@@ +190,5 @@
> +        mTargetFile, &NS_GET_IID(nsIDOMFile));
> +    }
> +    NS_WARN_IF(JSVAL_IS_NULL(value));
> +    JS::Rooted<JS::Value> val(cx, value);
> +    mPromise->MaybeResolve(cx, val);

I get a problem with |mPromise->MaybeResolve(cx, val)|. |mPromise->MaybeResolve| will try to get the js |then| property from |val| parameter with the following code:
 JS::Rooted<JS::Value> then(cx);
 JS_GetProperty(cx, val, "then", &then)

The js compartments of cx and val are not the same. It makes debug build crash. How can I resolve this issue?

Comment 92

[Yuan Xulei [:yxl] (Leave from May 18)]

@dhylands, I got the above issue when rebasing the patch. Any idea to solve it?

Comment 93

[Boris Zbarsky [:bzbarsky]]

> How can I resolve this issue?

You need to enter the correct compartment.  In general, any time you call a function with a cx and a value the compartments need to match.

Is "val" guaranteed to be an object in this case?

Looking at the code more carefully, WrapperCacheObjectToJsval is completely wrong.  You should be using WrapNewBindingObject instead; that won't clobber existing values.  And you should be entering the compartment of the correct global first; possibly as soon as you put that AutoSafeJSContext on the stack.

InterfaceToJsval is also reinventing some wheels... and should again expect the caller to enter the right compartment.

What you may want a helper for is "get the global JSObject from this nsPIDOMWindow".

Comment 94

[Boris Zbarsky [:bzbarsky]]

On general, I strongly suggest getting a review for this patch from someone familiar with jsapi and bindings...

Comment 95

[Yuan Xulei [:yxl] (Leave from May 18)]

@bz, thanks. Can you help to review this patch? or recommend someone else for me?

Comment 96

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Boris Zbarsky [:bz] from comment #93)
> > How can I resolve this issue?
> 
> You need to enter the correct compartment.  In general, any time you call a
> function with a cx and a value the compartments need to match.
> 
> Is "val" guaranteed to be an object in this case?
Yes, "val" is an object.

Comment 97

[Boris Zbarsky [:bzbarsky]]

I can try, but likely not until Monday.  Olli, would you be able to take a look before then, and if so, are you happy doing it?

Comment 98

[Dave Hylands [:dhylands]]

(In reply to Yuan Xulei [:yxl] from comment #92)
> @dhylands, I got the above issue when rebasing the patch. Any idea to solve
> it?

Sorry - this a bit out of my depth.

Comment 99

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v5 Implement getRoot createDirectory and get for Directory.

Update patch based on comment 93.
@bz and @smaug, if either of you can take a look, please help. Thank you ;-)

Comment 100

[Yuan Xulei [:yxl] (Leave from May 18)]

The above patch fixes GetFileOrDirectoryTask::HandlerCallback of dom/filesystem/GetFileOrDirectoryTask.cpp and CreateDirectoryTask::HandlerCallback of dom/filesystem/CreateDirectoryTask.cpp. It also adds a helper function FilesystemUtils::GetGetGlobalJSObject to dom/filesystem/FilesystemUtils.h for "getting the global JSObject from this nsPIDOMWindow" and removes unused functions of FilesystemUtils. Other parts remain unchanged.

Comment 101

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8377026 [details] [diff] [review]
Part2 v5 Implement getRoot createDirectory and get for Directory.

All the non-main-thread handling requires plenty of MOZ_ASSERTs.
Like, those nsIFile methods which may do sync IO must not be run in the main thread of parent process.

In case you have mPromise member variable in classes inheriting TaskBase (which is thread safe, and used in several threads), please
document why mPromise is ever used only in one thread. Please add assertions.
Especially in case of parent process only I don't quite understand what guarantees mPromise to be addrefed or released only in the
thread it was created. (Promise objects are cycle collectable, so one must not addref or release them in the wrong thread.)

>+DOM4_MSG_DEF(AbortError, "File already exits.", NS_ERROR_DOM_FILESYSTEM_PATH_EXISTS_ERR)
exists

>+  nsCOMPtr<nsIFile> file = filesystem->GetLocalFile(mTargetRealPath);
>+  if (!file) {
>+    SetError(NS_ERROR_DOM_FILESYSTEM_INVALID_PATH_ERR);
>+    return;
>+  }
>+
>+   bool ret;
>+   nsresult rv = file->Exists(&ret);
some odd indentation here.


>+  // The local path of the root. Only available in the parent process.
>+  // In the child process, we don't use it and its value should be empty.
>+  nsString mLocalRootPath;
>+  nsPIDOMWindow* mWindow;
This is super scary. Nothing guarantees mWindow never points to a dead object.

>+Directory::IsValidRelativePath(const nsString& aPath)
>+{
>+  if (aPath.IsEmpty()) {
>+    return true;
>+  }
>+
>+  // Absolute path is not allowed.
>+  if (aPath.First() == FilesystemUtils::kSeparatorChar) {
>+    return false;
>+  }
>+
>+  static const nsString kCurrentDir = NS_LITERAL_STRING(".");
>+  static const nsString kParentDir = NS_LITERAL_STRING("..");
NS_NAMED_LITERAL_STRING

>+/*
>+ * Helper class for holding and accessing a weak reference of FilesystemBase.
>+ */
>+class FilesystemWeakRef
>+{
>+public:
>+  FilesystemWeakRef(FilesystemBase* aFilesystem);
>+  ~FilesystemWeakRef();
>+
>+  already_AddRefed<FilesystemBase>
>+  Get();
>+private:
>+  nsWeakPtr mFilesystem;
>+};
This is odd. What is wrong with normal do_QueryReferent?

>+class FilesystemRequestParent
>+  : public PFilesystemRequestParent
>+{
>+public:
>+  FilesystemRequestParent(const FilesystemParams& aParams);
>+  ~FilesystemRequestParent();
>+
>+  NS_IMETHOD_(nsrefcnt)
>+  AddRef();
>+
>+  NS_IMETHOD_(nsrefcnt)
>+  Release();
>+
What is wrong with normal thread safe refcounting macros?


>+class TaskBase
TaskBase is way too generic name dealing filesystem stuff, yet live in mozilla::dom.
(HTML spec is all about Tasks, but TaskBase has nothing to do with those tasks.)
So perhaps just FSTaskBase

Comment 102

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Olli Pettay [:smaug] from comment #101)
> Comment on attachment 8377026 [details] [diff] [review]
> Part2 v5 Implement getRoot createDirectory and get for Directory.
...
> >+class FilesystemRequestParent
> >+  : public PFilesystemRequestParent
> >+{
> >+public:
> >+  FilesystemRequestParent(const FilesystemParams& aParams);
> >+  ~FilesystemRequestParent();
> >+
> >+  NS_IMETHOD_(nsrefcnt)
> >+  AddRef();
> >+
> >+  NS_IMETHOD_(nsrefcnt)
> >+  Release();
> >+
> What is wrong with normal thread safe refcounting macros?
Do you mean the macro of NS_DECL_THREADSAFE_ISUPPORTS? FilesystemRequestParent 
doesn't inherit nsISupports, and we only need to add the refcounting functions 
"AddRef" and "Release". But NS_DECL_THREADSAFE_ISUPPORTS will add an extra
function "QueryInterface" besides the refcouting ones. That's why I didn't use 
NS_DECL_THREADSAFE_ISUPPORTS. Is there a macro for this case?

Comment 103

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v6 Implement getRoot createDirectory and get for Directory.

The patch addresses all the items of Comment 101 except the "thread safe refcounting macros" issue.

TaskBase is renamed to FilesystemTaskBase to keep consistent with other class names, such as FilesystemBase, FilesystemUtils.

Comment 104

[Jan Varga [:janv]]

maybe NS_INLINE_DECL_THREADSAFE_REFCOUNTING(class) ?

Comment 105

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: interdiff of comment 101

This is the interdiff to help review the above patch.

Comment 106

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Varga [:janv] from comment #104)
> maybe NS_INLINE_DECL_THREADSAFE_REFCOUNTING(class) ?

Thank you. It works :)

Comment 107

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v7 Implement getRoot createDirectory and get for Directory.

Fix "thread safe refcounting macros" issue.

The interdiff:

diff --git a/dom/filesystem/FilesystemRequestParent.cpp b/dom/filesystem/FilesystemRequestParent.cpp
--- a/dom/filesystem/FilesystemRequestParent.cpp
+++ b/dom/filesystem/FilesystemRequestParent.cpp
@@ -7,19 +7,16 @@
 
 #include "mozilla/dom/FilesystemBase.h"
 #include "mozilla/dom/FilesystemTaskBase.h"
 #include "mozilla/dom/FilesystemUtils.h"
 
 namespace mozilla {
 namespace dom {
 
-NS_IMPL_ADDREF(FilesystemRequestParent)
-NS_IMPL_RELEASE(FilesystemRequestParent)
-
 FilesystemRequestParent::FilesystemRequestParent(
   const FilesystemParams& aParams)
   : mParams(aParams)
 {
 }
 
 FilesystemRequestParent::~FilesystemRequestParent()
 {
diff --git a/dom/filesystem/FilesystemRequestParent.h b/dom/filesystem/FilesystemRequestParent.h
--- a/dom/filesystem/FilesystemRequestParent.h
+++ b/dom/filesystem/FilesystemRequestParent.h
@@ -14,38 +14,30 @@
 namespace mozilla {
 namespace dom {
 
 class FilesystemBase;
 
 class FilesystemRequestParent
   : public PFilesystemRequestParent
 {
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(FilesystemRequestParent)
 public:
   FilesystemRequestParent(const FilesystemParams& aParams);
   ~FilesystemRequestParent();
 
-  NS_IMETHOD_(nsrefcnt)
-  AddRef();
-
-  NS_IMETHOD_(nsrefcnt)
-  Release();
-
   bool
   IsRunning()
   {
     return state() == PFilesystemRequest::__Start;
   }
 
   void
   Dispatch();
 private:
-  ThreadSafeAutoRefCnt mRefCnt;
-  NS_DECL_OWNINGTHREAD
-
   FilesystemParams mParams;
   nsCOMPtr<FilesystemBase> mFilesystem;
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_FilesystemRequestParent_h

Comment 108

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part3 v4 Basic tests for filesystem API

Update test. Fix the test_fs_get.html failure on emulator.

Comment 109

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8377400 [details] [diff] [review]
Part2 v7 Implement getRoot createDirectory and get for Directory.

>+  nsDOMDeviceStorage* mDeviceStorage;
This another super scary member variable.
I don't see anything making sure mDeviceStorage can't point to a deleted object.
Especially, nothing sets it to null.
So, who is supposed to own what and how long?
I'd like to get answer to that and some fixes to the patch before reviewing the rest.

Comment 110

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Olli Pettay [:smaug] from comment #109)
> Comment on attachment 8377400 [details] [diff] [review]
> Part2 v7 Implement getRoot createDirectory and get for Directory.
> 
> >+  nsDOMDeviceStorage* mDeviceStorage;
> This another super scary member variable.
> I don't see anything making sure mDeviceStorage can't point to a deleted
> object.
> Especially, nothing sets it to null.
> So, who is supposed to own what and how long?
> I'd like to get answer to that and some fixes to the patch before reviewing
> the rest.

nsDOMDeviceStorage owns DeviceStorageFilesystem. It creates DeviceStorageFilesystem with nsDOMDeviceStorage#GetRoot and releases DeviceStorageFilesystem when it destructs.

Comment 111

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v8 Implement getRoot createDirectory and get for Directory.

(In reply to Olli Pettay [:smaug] from comment #109)
> Comment on attachment 8377400 [details] [diff] [review]
> Part2 v7 Implement getRoot createDirectory and get for Directory.
> 
> >+  nsDOMDeviceStorage* mDeviceStorage;
> This another super scary member variable.
> I don't see anything making sure mDeviceStorage can't point to a deleted
> object.
> Especially, nothing sets it to null.
> So, who is supposed to own what and how long?
> I'd like to get answer to that and some fixes to the patch before reviewing
> the rest.
This patch adds the |SetDeviceStorage| function to DeviceStorageFilesystem class to set mDeviceStorage to null when mDeviceStorage is deleted.

Also I refactored TaskBase class to FilesystemTaskBase and removed its static StartXXXTask functions to make it thinner.

Comment 112

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v8 Implement getRoot createDirectory and get for Directory.

Sorry, this is the updated patch.

Comment 113

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: interdiff for Part2 between v8 and v6

The interdiff of part2 between the above patch and the one before the previous.

Comment 114

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part4 v1 permission request and check

Add permission request and check logic, which is similar to the original DeviceStorageRequest.

Comment 115

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part5 v1 Test for checking permission

Most of the code is copied from test_app_permissions.html of devicestorage.

Comment 116

[Dave Hylands [:dhylands]]

Comment on attachment 8379023 [details] [diff] [review]
Part4 v1 permission request and check

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

You patches aren't including much context. If you're using git to create the patches, please use -U8. If you're using mercurial, please see https://developer.mozilla.org/en/docs/Installing_Mercurial (Basic Configuration section).

Otherwise, looks good.

::: dom/filesystem/DeviceStorageFilesystem.cpp
@@ +32,5 @@
>    mString.AppendLiteral("-");
>    mString.Append(mStorageName);
>  
> +  mIsTesting =
> +    mozilla::Preferences::GetBool("device.storage.prompt.testing", false);

nit: Preferences only work on the main thread, so we should assert that we're on the main thread (you might be doing this already, I just can't tell)

::: dom/filesystem/FilesystemPermissionRequest.cpp
@@ +84,5 @@
> +  return CreatePermissionArray(mPermissionType, mPermissionAccess, aTypes);
> +}
> +
> +NS_IMETHODIMP
> +FilesystemPermissionRequest::GetPrincipal(nsIPrincipal * *aRequestingPrincipal)

nit: *'s to the left (also a few places below)

::: dom/filesystem/FilesystemPermissionRequest.h
@@ +42,5 @@
> +  NS_DECL_NSICONTENTPERMISSIONREQUEST
> +  NS_DECL_NSIRUNNABLE
> +private:
> +  FilesystemPermissionRequest(FilesystemTaskBase* aTask);
> +  ~FilesystemPermissionRequest();

nit: should be declared virtual (it inherits the virtualness from the base, but it also acts as a form of documentaiton)

Comment 117

[Olli Pettay [:smaug][[e-mail]]]

(trying to review this tomorrow.)

Comment 118

[Dave Hylands [:dhylands]]

Comment on attachment 8379026 [details] [diff] [review]
Part5 v1 Test for checking permission

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

r=me with a few more test cases added

::: dom/devicestorage/test/test_fs_app_permissions.html
@@ +109,5 @@
> +    type: 'music',
> +    shouldPass: false,
> +    fileExtension: '.ogg',
> +    test: TestGet
> +  },

So I can think of a few more test cases to add:

.ogg and .mp4 can both be either video or music, so they should get picked up by either one.

We should make sure that wrong file types are ignored (so creating a .png and then reading using music shouldn't pick up the png file).

Comment 119

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #116)
> Comment on attachment 8379023 [details] [diff] [review]
> Part4 v1 permission request and check
> 
> Review of attachment 8379023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You patches aren't including much context. If you're using git to create the
> patches, please use -U8. If you're using mercurial, please see
> https://developer.mozilla.org/en/docs/Installing_Mercurial (Basic
> Configuration section).
Thanks. I'll provide hg patch next time:)

> 
> Otherwise, looks good.
> 
> ::: dom/filesystem/DeviceStorageFilesystem.cpp
> @@ +32,5 @@
> >    mString.AppendLiteral("-");
> >    mString.Append(mStorageName);
> >  
> > +  mIsTesting =
> > +    mozilla::Preferences::GetBool("device.storage.prompt.testing", false);
> 
> nit: Preferences only work on the main thread, so we should assert that
> we're on the main thread (you might be doing this already, I just can't tell)
I'll fix it.
> 
> ::: dom/filesystem/FilesystemPermissionRequest.cpp
> @@ +84,5 @@
> > +  return CreatePermissionArray(mPermissionType, mPermissionAccess, aTypes);
> > +}
> > +
> > +NS_IMETHODIMP
> > +FilesystemPermissionRequest::GetPrincipal(nsIPrincipal * *aRequestingPrincipal)
> 
> nit: *'s to the left (also a few places below)
Should be fixed:)
> 
> ::: dom/filesystem/FilesystemPermissionRequest.h
> @@ +42,5 @@
> > +  NS_DECL_NSICONTENTPERMISSIONREQUEST
> > +  NS_DECL_NSIRUNNABLE
> > +private:
> > +  FilesystemPermissionRequest(FilesystemTaskBase* aTask);
> > +  ~FilesystemPermissionRequest();
> 
> nit: should be declared virtual (it inherits the virtualness from the base,
> but it also acts as a form of documentaiton)
FilesystemPermissionRequest is a final class and we don't delete it with a base pointer. Should we still declare virtual base for it?

Comment 120

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #118)
> Comment on attachment 8379026 [details] [diff] [review]
> Part5 v1 Test for checking permission
> 
> Review of attachment 8379026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with a few more test cases added
> 
> ::: dom/devicestorage/test/test_fs_app_permissions.html
> @@ +109,5 @@
> > +    type: 'music',
> > +    shouldPass: false,
> > +    fileExtension: '.ogg',
> > +    test: TestGet
> > +  },
> 
> So I can think of a few more test cases to add:
> 
> .ogg and .mp4 can both be either video or music, so they should get picked
> up by either one.
> 
> We should make sure that wrong file types are ignored (so creating a .png
> and then reading using music shouldn't pick up the png file).
This Filesystem API doesn't do such kind of type check. We don't prevent reading 
a .png file under the music device storage. And it seems the original DeviceStorage 
API doesn't prevent adding or reading a .png file under the music device storage.

If we need to enforce this, I'll implement it.

Comment 121

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Yuan Xulei [:yxl] from comment #120)
> (In reply to Dave Hylands [:dhylands] from comment #118)
> > Comment on attachment 8379026 [details] [diff] [review]
> > Part5 v1 Test for checking permission
> > 
> > Review of attachment 8379026 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with a few more test cases added
> > 
> > ::: dom/devicestorage/test/test_fs_app_permissions.html
> > @@ +109,5 @@
> > > +    type: 'music',
> > > +    shouldPass: false,
> > > +    fileExtension: '.ogg',
> > > +    test: TestGet
> > > +  },
> > 
> > So I can think of a few more test cases to add:
> > 
> > .ogg and .mp4 can both be either video or music, so they should get picked
> > up by either one.
> > 
> > We should make sure that wrong file types are ignored (so creating a .png
> > and then reading using music shouldn't pick up the png file).
> This Filesystem API doesn't do such kind of type check. We don't prevent
> reading 
> a .png file under the music device storage. And it seems the original
> DeviceStorage 
> API doesn't prevent adding or reading a .png file under the music device
> storage.
> 
> If we need to enforce this, I'll implement it.
I made a mistake. The original DeviceStorage API does checks the file type when adding or reading files. So I'll update my patch to add similar function.

Comment 122

[Dave Hylands [:dhylands]]

(In reply to Yuan Xulei [:yxl] from comment #120)
> (In reply to Dave Hylands [:dhylands] from comment #118)
> > Comment on attachment 8379026 [details] [diff] [review]
...snip...
> a .png file under the music device storage. And it seems the original
> DeviceStorage 
> API doesn't prevent adding or reading a .png file under the music device
> storage.
> 
> If we need to enforce this, I'll implement it.

On the phone, sdcard, music, video, and pictures are all stored in the same directory tree (the fact that there happens to be a Photos folder is just a user convenience. For music, cover art might be stored in the same directory as the music files.

So any file you store in music can be enumerated by sdcard or vice-versa.

Hence the issue with .ogg and .mp4 files which could contain music or video.

Comment 123

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Interdiff for Comment 116

Comment 124

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Interdiff for Comment 118

Comment 125

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part4 v2 permission request and check

Fix items of comment 116 and make sure the file type is correct.

Comment 126

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8381352 [details] [diff] [review]
Part4 v2 permission request and check

The interdiff with the previous version:
https://bugzilla.mozilla.org/attachment.cgi?id=8381349&action=diff

Comment 127

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part5 v2 Test for checking permission

Add more test cases as comment 118 mentioned.
The interdiff with previous version:
https://bugzilla.mozilla.org/attachment.cgi?id=8381352&action=diff

Comment 128

[Dave Hylands [:dhylands]]

Comment on attachment 8381352 [details] [diff] [review]
Part4 v2 permission request and check

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

Just a minor potential issue with the descendant logic.

::: dom/filesystem/FilesystemUtils.cpp
@@ +70,5 @@
> +  prefix = aPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR);
> +
> +  // Check the sub-directory path to see if it has the parent path as prefix.
> +  if (aDescendantPath.Length() < prefix.Length() ||
> +      !StringBeginsWith(aDescendantPath, prefix)) {

If aDescendantPath is greater in length than prefix, it might not be a descendant if the prefix matches.

If prefix ends with a directory separator, then this logic is fine.

Put if prefix was /sdcard/someDir
and aDescendant was /sdcard/someDirElsewhere/

then descendant passes your test but isn't a descendant.

Comment 129

[Dave Hylands [:dhylands]]

(In reply to Yuan Xulei [:yxl] from comment #120)
> (In reply to Dave Hylands [:dhylands] from comment #118)
> > Comment on attachment 8379026 [details] [diff] [review]
> > Part5 v1 Test for checking permission
> > 
> > Review of attachment 8379026 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with a few more test cases added
> > 
> > ::: dom/devicestorage/test/test_fs_app_permissions.html
> > @@ +109,5 @@
> > > +    type: 'music',
> > > +    shouldPass: false,
> > > +    fileExtension: '.ogg',
> > > +    test: TestGet
> > > +  },
> > 
> > So I can think of a few more test cases to add:
> > 
> > .ogg and .mp4 can both be either video or music, so they should get picked
> > up by either one.
> > 
> > We should make sure that wrong file types are ignored (so creating a .png
> > and then reading using music shouldn't pick up the png file).
> This Filesystem API doesn't do such kind of type check. We don't prevent
> reading 
> a .png file under the music device storage. And it seems the original
> DeviceStorage 
> API doesn't prevent adding or reading a .png file under the music device
> storage.
> 
> If we need to enforce this, I'll implement it.

device storage does that checking here:
http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#2622

Comment 130

[Dave Hylands [:dhylands]]

(In reply to Yuan Xulei [:yxl] from comment #119)
> (In reply to Dave Hylands [:dhylands] from comment #116)
> > ::: dom/filesystem/FilesystemPermissionRequest.h
> > @@ +42,5 @@
> > > +  NS_DECL_NSICONTENTPERMISSIONREQUEST
> > > +  NS_DECL_NSIRUNNABLE
> > > +private:
> > > +  FilesystemPermissionRequest(FilesystemTaskBase* aTask);
> > > +  ~FilesystemPermissionRequest();
> > 
> > nit: should be declared virtual (it inherits the virtualness from the base,
> > but it also acts as a form of documentaiton)
> FilesystemPermissionRequest is a final class and we don't delete it with a
> base pointer. Should we still declare virtual base for it?

I think that the "rule" or "guideline" is that: if a class contains virtual methods then it should have a virtual destructor. Whether you destruct through a base pointer or not shouldn't matter, especially since somebody could change the code down the road and then get weird behaviour.

If a base class has a virtual destructor then the derived destructor will be virtual regardless of whether its declared virtual or not.

So since it is, in fact, virtual, we should mention this as a form of documentation. Otherwise people looking at this may think that you forgot to declare it virtual.

Comment 131

[Dave Hylands [:dhylands]]

Comment on attachment 8381356 [details] [diff] [review]
Part5 v2 Test for checking permission

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

r=me with issue addressed.

::: dom/devicestorage/test/test_fs_app_permissions.html
@@ +153,5 @@
> +    type: 'videos',
> +    shouldPass: true,
> +    fileExtension: '.ogg',
> +
> +    permissions: ["device-storage:music"],

Shouldn't the permission match the type? Otherwise, this should fail, not pass.

@@ +212,5 @@
> +    shouldPass: true,
> +    fileExtension: '.ogg',
> +
> +    app: "https://example.com/manifest_cert.webapp",
> +    permissions: ["device-storage:music"],

Similary here.

Comment 132

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8378854 [details] [diff] [review]
Part2 v8 Implement getRoot createDirectory and get for Directory.


>+  rv = file->Create(nsIFile::DIRECTORY_TYPE, 0777);
Do we really want 0777 here? I guess so, and DeviceStorage etc do that too.


>+CreateDirectoryTask::HandlerCallback()
>+{
>+  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>+  nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
>+  if (!filesystem) {
>+    return;
>+  }
>+
>+  AutoSafeJSContext cx;
>+  JSObject* global =
>+    FilesystemUtils::GetGetGlobalJSObject(filesystem->GetWindow());
>+  if (!global) {
>+    return;
>+  }
>+  JSAutoCompartment ac(cx, global);
>+  JS::Rooted<JSObject*> scopeObj(cx, global);
>+
>+  JS::Rooted<JS::Value> val(cx);
>+  if (!HasError()) {
>+    nsRefPtr<Directory> dir = new Directory(filesystem, mTargetRealPath);
>+    if (!dom::WrapNewBindingObject(cx, scopeObj, dir, &val)) {
>+      return;
>+    }
>+    mPromise->MaybeResolve(cx, val);
>+  } else {
>+    nsRefPtr<DOMError> domError = new DOMError(filesystem->GetWindow(),
>+      mErrorValue);
>+    if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
>+      return;
>+    }
>+    mPromise->MaybeReject(cx, val);
>+  }
>+}
This all will become a lot simpler once Bug 974120 lands.
Do you think you could base your patch on top of that.


>+GetFileOrDirectoryTask::HandlerCallback()
>+{
>+  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>+  nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
>+  if (!filesystem) {
>+    return;
>+  }
>+
>+  AutoSafeJSContext cx;
>+  JSObject* global =
>+    FilesystemUtils::GetGetGlobalJSObject(filesystem->GetWindow());
>+  if (!global) {
>+    return;
>+  }
>+  JSAutoCompartment ac(cx, global);
>+  JS::Rooted<JSObject*> scopeObj(cx, global);
>+
>+  JS::Rooted<JS::Value> val(cx);
>+  if (!HasError()) {
>+    if (mIsDirectory) {
>+      nsRefPtr<Directory> dir = new Directory(filesystem, mTargetRealPath);
>+      if (!dom::WrapNewBindingObject(cx, scopeObj, dir, &val)) {
>+        return;
>+      }
>+    } else {
>+      if (NS_FAILED(nsContentUtils::WrapNative(cx, scopeObj, mTargetFile,
>+          &NS_GET_IID(nsIDOMFile), &val))) {
>+        return;
>+      }
>+    }
>+    mPromise->MaybeResolve(cx, val);
>+  } else {
>+    nsRefPtr<DOMError> domError = new DOMError(filesystem->GetWindow(),
>+      mErrorValue);
>+    if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
>+      return;
>+    }
>+    mPromise->MaybeReject(cx, val);
>+  }
Also this would be simpler on top of bug 974120.

So, I'd like to see a new patch on top of bug 974120, since that way there should be less
error prone JS API and binding API usage.

Comment 133

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #128)
> Comment on attachment 8381352 [details] [diff] [review]
> Part4 v2 permission request and check
> 
> Review of attachment 8381352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a minor potential issue with the descendant logic.
> 
> ::: dom/filesystem/FilesystemUtils.cpp
> @@ +70,5 @@
> > +  prefix = aPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR);
> > +
> > +  // Check the sub-directory path to see if it has the parent path as prefix.
> > +  if (aDescendantPath.Length() < prefix.Length() ||
> > +      !StringBeginsWith(aDescendantPath, prefix)) {
> 
> If aDescendantPath is greater in length than prefix, it might not be a
> descendant if the prefix matches.
> 
> If prefix ends with a directory separator, then this logic is fine.
> 
> Put if prefix was /sdcard/someDir
> and aDescendant was /sdcard/someDirElsewhere/
> 
> then descendant passes your test but isn't a descendant.
That's what the patch has done. The prefix will always ends with a directory separator:
prefix = aPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR);

Comment 134

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #130)
> (In reply to Yuan Xulei [:yxl] from comment #119)
> > (In reply to Dave Hylands [:dhylands] from comment #116)
> > > ::: dom/filesystem/FilesystemPermissionRequest.h
> > > @@ +42,5 @@
> > > > +  NS_DECL_NSICONTENTPERMISSIONREQUEST
> > > > +  NS_DECL_NSIRUNNABLE
> > > > +private:
> > > > +  FilesystemPermissionRequest(FilesystemTaskBase* aTask);
> > > > +  ~FilesystemPermissionRequest();
> > > 
> > > nit: should be declared virtual (it inherits the virtualness from the base,
> > > but it also acts as a form of documentaiton)
> > FilesystemPermissionRequest is a final class and we don't delete it with a
> > base pointer. Should we still declare virtual base for it?
> 
> I think that the "rule" or "guideline" is that: if a class contains virtual
> methods then it should have a virtual destructor. Whether you destruct
> through a base pointer or not shouldn't matter, especially since somebody
> could change the code down the road and then get weird behaviour.
> 
> If a base class has a virtual destructor then the derived destructor will be
> virtual regardless of whether its declared virtual or not.
> 
> So since it is, in fact, virtual, we should mention this as a form of
> documentation. Otherwise people looking at this may think that you forgot to
> declare it virtual.

Thanks for clarification. I'll fix them all in my patch:)

Comment 135

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Olli Pettay [:smaug] from comment #132)
> Comment on attachment 8378854 [details] [diff] [review]
> Part2 v8 Implement getRoot createDirectory and get for Directory.
> 
> 
> >+  rv = file->Create(nsIFile::DIRECTORY_TYPE, 0777);
> Do we really want 0777 here? I guess so, and DeviceStorage etc do that too.
It just follows the DeviceStorage and see comment 46 (https://bugzilla.mozilla.org/show_bug.cgi?id=910412#c46) for details. It might be restricted to 0770, but I 
haven't test it.
> 
> 
> >+CreateDirectoryTask::HandlerCallback()
> >+{
> >+  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> >+  nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
> >+  if (!filesystem) {
> >+    return;
> >+  }
> >+
> >+  AutoSafeJSContext cx;
> >+  JSObject* global =
> >+    FilesystemUtils::GetGetGlobalJSObject(filesystem->GetWindow());
> >+  if (!global) {
> >+    return;
> >+  }
> >+  JSAutoCompartment ac(cx, global);
> >+  JS::Rooted<JSObject*> scopeObj(cx, global);
> >+
> >+  JS::Rooted<JS::Value> val(cx);
> >+  if (!HasError()) {
> >+    nsRefPtr<Directory> dir = new Directory(filesystem, mTargetRealPath);
> >+    if (!dom::WrapNewBindingObject(cx, scopeObj, dir, &val)) {
> >+      return;
> >+    }
> >+    mPromise->MaybeResolve(cx, val);
> >+  } else {
> >+    nsRefPtr<DOMError> domError = new DOMError(filesystem->GetWindow(),
> >+      mErrorValue);
> >+    if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
> >+      return;
> >+    }
> >+    mPromise->MaybeReject(cx, val);
> >+  }
> >+}
> This all will become a lot simpler once Bug 974120 lands.
> Do you think you could base your patch on top of that.
> 
> 
> >+GetFileOrDirectoryTask::HandlerCallback()
> >+{
> >+  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> >+  nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
> >+  if (!filesystem) {
> >+    return;
> >+  }
> >+
> >+  AutoSafeJSContext cx;
> >+  JSObject* global =
> >+    FilesystemUtils::GetGetGlobalJSObject(filesystem->GetWindow());
> >+  if (!global) {
> >+    return;
> >+  }
> >+  JSAutoCompartment ac(cx, global);
> >+  JS::Rooted<JSObject*> scopeObj(cx, global);
> >+
> >+  JS::Rooted<JS::Value> val(cx);
> >+  if (!HasError()) {
> >+    if (mIsDirectory) {
> >+      nsRefPtr<Directory> dir = new Directory(filesystem, mTargetRealPath);
> >+      if (!dom::WrapNewBindingObject(cx, scopeObj, dir, &val)) {
> >+        return;
> >+      }
> >+    } else {
> >+      if (NS_FAILED(nsContentUtils::WrapNative(cx, scopeObj, mTargetFile,
> >+          &NS_GET_IID(nsIDOMFile), &val))) {
> >+        return;
> >+      }
> >+    }
> >+    mPromise->MaybeResolve(cx, val);
> >+  } else {
> >+    nsRefPtr<DOMError> domError = new DOMError(filesystem->GetWindow(),
> >+      mErrorValue);
> >+    if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
> >+      return;
> >+    }
> >+    mPromise->MaybeReject(cx, val);
> >+  }
> Also this would be simpler on top of bug 974120.
> 
> So, I'd like to see a new patch on top of bug 974120, since that way there
> should be less
> error prone JS API and binding API usage.
OK, I'll provide the patch soon:)

Comment 136

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: interdiff for Comment 132

Comment 137

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v4 WebIDL and DOM binding for filesystem API

rebased.

Comment 138

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v9 Implement getRoot createDirectory and get for Directory

Fix the items of comment 132. The interdiff:
https://bugzilla.mozilla.org/attachment.cgi?id=8381995&action=diff

Comment 139

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part3 v4 Basic tests for filesystem API

rebased.

Comment 140

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part4 v3 permission request and check

Rebase and address Comment 130.

Comment 141

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part5 v3 Test for checking permission

Address Comment 131.

Comment 142

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8382011 [details] [diff] [review]
Part2 v9 Implement getRoot createDirectory and get for Directory

Shouldn't everything named *Filesystem* be actually *FileSystem*

>+Directory::DOMPathToRealPath(const nsAString& aPath, nsAString& aRealPath) const
>+{
>+  aRealPath.Truncate();
>+
>+  nsString relativePath;
>+  relativePath = aPath;
>+
>+  // Trim white spaces.
>+  static const char kWhitespace[] = "\b\t\r\n ";
>+  relativePath.Trim(kWhitespace);
>+
>+  // Remove the leading "./".
>+  if (StringBeginsWith(relativePath, NS_LITERAL_STRING("./"))) {
What if relativePath is something like "./././foobar"?
In other words, I don't quite understand what this leading "./"
removal is for.
IsValidRelativePath seems to then later reject paths with ./ but why
why ./ is ok but two isn't?


>+  // Remove trailing "/".
>+  relativePath.Trim(FILESYSTEM_DOM_PATH_SEPARATOR, false, true);
And do we need to care about if relativePath ends with several "/" ?
>+FilesystemTaskBase::FilesystemTaskBase(FilesystemBase* aFilesystem,
>+                   const FilesystemParams& aParam,
>+                                       FilesystemRequestParent* aParent)
Align the latter two params under the first one


>+FilesystemTaskBase::Start()
>+{
>+  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
>+
>+  if (HasError()) {
>+    HandlerCallback();
>+    return;
>+  }
>+
>+  if (FilesystemUtils::IsParentProcess()) {
>+    // Run in parent process.
>+    // Start worker thread.
>+    nsCOMPtr<nsIEventTarget> target
>+      = do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
>+    NS_ASSERTION(target, "Must have stream transport service.");
>+    target->Dispatch(this, NS_DISPATCH_NORMAL);
>+    return;
>+  }
>+
>+  // Run in child process.
>+  nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
>+  if (!filesystem) {
>+    return;
>+  }
>+
>+  // Retain a reference so the task object isn't deleted without IPDL's
>+  // knowledge. The reference will be released by
>+  // mozilla::dom::ContentChild::DeallocPFilesystemRequestChild.
>+  AddRef();
NS_ADDREF_THIS();

>+  /*
>+   * To create a parent process task delivered from the child process through
>+   * IPC.
>+   */
>+  FilesystemTaskBase(FilesystemBase* aFilesystem,
>+           const FilesystemParams& aParam,
>+           FilesystemRequestParent* aParent);
Align params



No need to ask another review for coding style nits or renames, just fix before landing,
but I'd like to get some explanation why path string handling is what it is.
So conditional r+, waiting for explanation and possible another patch on top of this
changing the path handling.

Comment 143

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Olli Pettay [:smaug] from comment #142)
> Comment on attachment 8382011 [details] [diff] [review]
> Part2 v9 Implement getRoot createDirectory and get for Directory
--snip--
> 
> No need to ask another review for coding style nits or renames, just fix
> before landing,
> but I'd like to get some explanation why path string handling is what it is.
> So conditional r+, waiting for explanation and possible another patch on top
> of this
> changing the path handling.
The relative path should not contain any segment of ".." or ".", and not start 
or end with "/". This patch tried to correct the path.

I'd like to change patch to make strict checks on the path and don't allow the 
following invalid paths:
"../foo", "..", "foo/../bar", "/foo/bar", "./", "foo/./bar", "bar/", ""bar/."

Comment 144

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v10 Implement getRoot createDirectory and get for Directory

Fix path string handling issue based on the above two comments (comment 142 and 143). I'll rename all *Filesystem*s to *FileSystem*s after this gets reviewed.

The interdiff:

diff --git a/dom/filesystem/Directory.cpp b/dom/filesystem/Directory.cpp
--- a/dom/filesystem/Directory.cpp
+++ b/dom/filesystem/Directory.cpp
@@ -129,51 +129,38 @@ Directory::DOMPathToRealPath(const nsASt
 
   nsString relativePath;
   relativePath = aPath;
 
   // Trim white spaces.
   static const char kWhitespace[] = "\b\t\r\n ";
   relativePath.Trim(kWhitespace);
 
-  // Remove the leading "./".
-  if (StringBeginsWith(relativePath, NS_LITERAL_STRING("./"))) {
-    relativePath = Substring(relativePath, 2);
-  } else {
-    relativePath = relativePath;
-  }
-
-  // Remove trailing "/".
-  relativePath.Trim(FILESYSTEM_DOM_PATH_SEPARATOR, false, true);
-
-  // We don't allow empty relative path to access the root.
-  if (relativePath.IsEmpty()) {
-    return false;
-  }
-
   if (!IsValidRelativePath(relativePath)) {
     return false;
   }
 
   aRealPath = mPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR) +
     relativePath;
 
   return true;
 }
 
 // static
 bool
 Directory::IsValidRelativePath(const nsString& aPath)
 {
+  // We don't allow empty relative path to access the root.
   if (aPath.IsEmpty()) {
-    return true;
+    return false;
   }
 
-  // Absolute path is not allowed.
-  if (aPath.First() == FilesystemUtils::kSeparatorChar) {
+  // Leading and trailing "/" are not allowed.
+  if (aPath.First() == FilesystemUtils::kSeparatorChar ||
+      aPath.Last() == FilesystemUtils::kSeparatorChar) {
     return false;
   }
 
   NS_NAMED_LITERAL_STRING(kCurrentDir, ".");
   NS_NAMED_LITERAL_STRING(kParentDir, "..");
 
   // Split path and check each path component.
   nsCharSeparatedTokenizer tokenizer(aPath, FilesystemUtils::kSeparatorChar);
diff --git a/dom/filesystem/FilesystemTaskBase.cpp b/dom/filesystem/FilesystemTaskBase.cpp
--- a/dom/filesystem/FilesystemTaskBase.cpp
+++ b/dom/filesystem/FilesystemTaskBase.cpp
@@ -22,18 +22,18 @@ FilesystemTaskBase::FilesystemTaskBase(F
   : mErrorValue(NS_OK)
 {
   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
   MOZ_ASSERT(aFilesystem, "aFilesystem should not be null.");
   mFilesystem = do_GetWeakReference(aFilesystem);
 }
 
 FilesystemTaskBase::FilesystemTaskBase(FilesystemBase* aFilesystem,
-                   const FilesystemParams& aParam,
-                   FilesystemRequestParent* aParent)
+                                       const FilesystemParams& aParam,
+                                       FilesystemRequestParent* aParent)
   : mErrorValue(NS_OK)
   , mRequestParent(aParent)
 {
   MOZ_ASSERT(FilesystemUtils::IsParentProcess(),
              "Only call from parent process!");
   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
   MOZ_ASSERT(aFilesystem, "aFilesystem should not be null.");
   mFilesystem = do_GetWeakReference(aFilesystem);
@@ -68,17 +68,17 @@ FilesystemTaskBase::Start()
   nsRefPtr<FilesystemBase> filesystem = do_QueryReferent(mFilesystem);
   if (!filesystem) {
     return;
   }
 
   // Retain a reference so the task object isn't deleted without IPDL's
   // knowledge. The reference will be released by
   // mozilla::dom::ContentChild::DeallocPFilesystemRequestChild.
-  AddRef();
+  NS_ADDREF_THIS();
   ContentChild::GetSingleton()->SendPFilesystemRequestConstructor(this,
     GetRequestParams(filesystem->ToString()));
 }
 
 NS_IMETHODIMP
 FilesystemTaskBase::Run()
 {
   if (!NS_IsMainThread()) {
diff --git a/dom/filesystem/FilesystemTaskBase.h b/dom/filesystem/FilesystemTaskBase.h
--- a/dom/filesystem/FilesystemTaskBase.h
+++ b/dom/filesystem/FilesystemTaskBase.h
@@ -134,18 +134,18 @@ protected:
    */
   FilesystemTaskBase(FilesystemBase* aFilesystem);
 
   /*
    * To create a parent process task delivered from the child process through
    * IPC.
    */
   FilesystemTaskBase(FilesystemBase* aFilesystem,
-           const FilesystemParams& aParam,
-           FilesystemRequestParent* aParent);
+                     const FilesystemParams& aParam,
+                     FilesystemRequestParent* aParent);
 
   virtual
   ~FilesystemTaskBase();
 
   /*
    * The function to perform task operation. It will be run on the worker
    * thread of the parent process.
    * Overrides this function to define the task operation for individual task.

Comment 145

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8384414 [details] [diff] [review]
Part2 v10 Implement getRoot createDirectory and get for Directory

Ok, makes more sense, since ./ handling is now at least more consistent.

We may want to think whether ./ should be allowed, but not in this bug.
(and whoever designed the API should make the decision.)

Comment 146

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We should never allow filenames that contain a path segment that is "." or "..". So all of the following should be treated as illegal directory or file names "./foo", "./", "foo/./bar", "../foo", "../", "foo/../bar".

All of those should result in an asynchronous error. I.e. the returned promise from the operation should be rejected.

Comment 147

[Olli Pettay [:smaug][[e-mail]]]

OK, thanks.

yxl, could you add some more tests for invalid path names.

Comment 148

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, also "." and ".." are forbidden directory or file names.

Comment 149

[Yuan Xulei [:yxl] (Leave from May 18)]

@smaug and @sicking, really thanks for your help :-)
I'll add more tests for invalid path names before landing this bug.

Comment 150

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v5 WebIDL and DOM binding for filesystem API

rebase.

Comment 151

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v11 Implement getRoot createDirectory and get for Directory

rebase.

Comment 152

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part3 v5 Basic tests for filesystem API

I'll add more tests for invalid path names

Comment 153

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part4 v3 permission request and check

rebase.

Comment 154

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part5 v3 Test for checking permission

rebase

Comment 155

[Yuan Xulei [:yxl] (Leave from May 18)]

Let's wait for the try server result.

https://tbpl.mozilla.org/?tree=Try&rev=345dee55fbfc

Comment 156

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v6 WebIDL and DOM binding for filesystem API

Rename all Filesystem to FileSystem.

Comment 157

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v12 Implement getRoot createDirectory and get for Directory

Rename all Filesystem to FileSystem.

Comment 158

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part3 v6 Basic tests for filesystem API

Rename all Filesystem to FileSystem.

Comment 159

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part4 v4 permission request and check

Rename all Filesystem to FileSystem.

Comment 160

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part5 v4 Test for checking permission

Rename all Filesystem to FileSystem.

Comment 161

[Yuan Xulei [:yxl] (Leave from May 18)]

https://tbpl.mozilla.org/?tree=Try&rev=5bb91483c0b7

Comment 162

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b1c0310cdac1
https://hg.mozilla.org/integration/b2g-inbound/rev/3431d59d752e
https://hg.mozilla.org/integration/b2g-inbound/rev/422b66d4b1ca
https://hg.mozilla.org/integration/b2g-inbound/rev/d5863d302bde
https://hg.mozilla.org/integration/b2g-inbound/rev/e3eb9463b3e1

Comment 163

[Jan Varga [:janv]]

Comment on attachment 8385837 [details] [diff] [review]
Part1 v6 WebIDL and DOM binding for filesystem API

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

::: dom/webidl/Directory.webidl
@@ +9,5 @@
> +/*
> + * All functions on Directory that accept DOMString arguments for file or
> + * directory names only allow relative path to current directory itself. The
> + * path should be a descendent path like "path/to/file.txt" and not contain a
> + * segment of ".." or ".". So the paths aren't allowd to walk up the directory

Nit: s/allowd/allowed

Comment 164

[(no longer active)]

Why did you put this code in gklayout and not xul?  <https://hg.mozilla.org/integration/b2g-inbound/rev/b1c0310cdac1#l5.19>  That's not going to work on Windows if you call any of the libxul internal symbols.

(That should be FINAL_LIBRARY = 'xul').

Comment 165

[(no longer active)]

Hmm, it seems like the code in dom/devicestorage is also linked into gklayout.  That is a very bizarre choice.

Comment 166

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Varga [:janv] from comment #163)
> Comment on attachment 8385837 [details] [diff] [review]
> Part1 v6 WebIDL and DOM binding for filesystem API
> 
> Review of attachment 8385837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Directory.webidl
> @@ +9,5 @@
> > +/*
> > + * All functions on Directory that accept DOMString arguments for file or
> > + * directory names only allow relative path to current directory itself. The
> > + * path should be a descendent path like "path/to/file.txt" and not contain a
> > + * segment of ".." or ".". So the paths aren't allowd to walk up the directory
> 
> Nit: s/allowd/allowed

Thanks, I'll fix it in bug 934368.

Comment 167

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #165)
> Hmm, it seems like the code in dom/devicestorage is also linked into
> gklayout.  That is a very bizarre choice.

It was linked into xul, but bug 935881 changed it to gklayout as well as most dom modules. So I followed them.

Comment 168

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b1c0310cdac1
https://hg.mozilla.org/mozilla-central/rev/3431d59d752e
https://hg.mozilla.org/mozilla-central/rev/422b66d4b1ca
https://hg.mozilla.org/mozilla-central/rev/d5863d302bde
https://hg.mozilla.org/mozilla-central/rev/e3eb9463b3e1

Comment 169

[Ryan VanderMeulen [:RyanVM]]

Another new crash with bug 981145 landed on inbound:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35943511&tree=Mozilla-Inbound

It seems we've been stuck in a week-long game of whack-a-mole fixing crashes from this bug. I've backed this out for now until it can be stabilized and re-landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/428e5462fe29

Comment 170

[Yuan Xulei [:yxl] (Leave from May 18)]

FileSystemBase is required to be thread-safe, but its base class nsSupportsWeakReference depends on the non-thread-safe class nsWeakReference. This non-thread-safe class causes the above crash.

I'll fix the thread-safe issue by removing nsSupportsWeakReference dependence FileSystemBase.

Comment 171

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part1 v7 WebIDL and DOM binding for filesystem API

Fix nits of Comment 163.

Comment 172

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part2 v13 Implement getRoot createDirectory and get for Directory

Comment 173

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part3 v7 Basic tests for filesystem API

Comment 174

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8390364 [details] [diff] [review]
Part3 v7 Basic tests for filesystem API

rebase

Comment 175

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part4 v5 permission request and check

Comment 176

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part5 v5 Test for checking permission

rebase

Comment 177

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: fs_base_interdiff.patch

This is the interdiff for all patches with the previous version. I made the following changes:
1. As weak pointer isn't thread-safe, I had to make FileSystemBase a strong pointer. 

2. Incorporate improvement from bug 934368.

The try server result:
https://tbpl.mozilla.org/?tree=Try&rev=89cd63b7a522

Comment 178

[Dave Hylands [:dhylands]]

I retriggered a bunch more M2's on Debug build of various platforms. Lets wait for those results to come in.

Comment 179

[Dave Hylands [:dhylands]]

Comment on attachment 8390365 [details] [diff] [review]
Part4 v5 permission request and check

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

::: dom/filesystem/FileSystemPermissionRequest.cpp
@@ +37,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  mTask->GetPermissionAccessType(mPermissionAccess);
> +
> +  FileSystemBase* filesystem = mTask->GetFileSystem();

You were using an nsRefPtr here before. Why did you change to a bare pointer?

I'm seeing other places that are still using nsRefPtr (like in the FileSystemPermissionRequest::Run below.

I'd much rather see the use of nsRefPtr

Comment 180

[Dave Hylands [:dhylands]]

Comment on attachment 8390363 [details] [diff] [review]
Part2 v13 Implement getRoot createDirectory and get for Directory

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

Looks good to me.

::: dom/filesystem/FileSystemTaskBase.cpp
@@ +49,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> +
> +  if (HasError()) {
> +    NS_DispatchToMainThread(this);

nit: Since you know you're already on the main thread, you can use NS_DispatchToCurrentThread(this) (which I was told was slightly more efficient).

Comment 181

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Dave Hylands [:dhylands] from comment #179)
> Comment on attachment 8390365 [details] [diff] [review]
> Part4 v5 permission request and check
> 
> Review of attachment 8390365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/filesystem/FileSystemPermissionRequest.cpp
> @@ +37,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  mTask->GetPermissionAccessType(mPermissionAccess);
> > +
> > +  FileSystemBase* filesystem = mTask->GetFileSystem();
> 
> You were using an nsRefPtr here before. Why did you change to a bare pointer?
mTask holds an owning reference to FileSystemBase, and we holds an owning reference to mTask.
So we holds an indirect owning reference to FileSystemBase. That's why I changed to a bare pointer.
Do I stil need to use nsRefPtr here?
> 
> I'm seeing other places that are still using nsRefPtr (like in the
> FileSystemPermissionRequest::Run below.
> 
> I'd much rather see the use of nsRefPtr

Comment 182

[Dave Hylands [:dhylands]]

It isn't so much about whether its needed now, its more about good coding practice.

Its more of a maintenance thing.

Somebody comes along and decides that they need to get the filesystem, and sees the example there and just copies it and winds up with weird problems because they perhaps should have used an nsRefPtr.

Or makes a change in the code in the future that then violates one of the assumptions.

So if you're returning a bare pointer, then why do you use nsRefPtr in FileSystemPermissionRequest::Run

I guess I'd have expected to see a bare pointer everywhere (if its ok) or using nsRefPtr everywhere, and not a mixture of the 2.

Comment 183

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Part4 v6 permission request and check

Dave, really thanks. I learned a lot from you.

Update the patch to use nsRefPtr instead of raw FileSystemBase pointer.

I checked the source code of NS_DispatchToMainThread and NS_DispatchToCurrentThread. It seems NS_DispatchToMainThread will
be a little more efficient.

Both gets the main thread instance of type nsIThread and then dispatches the runnable (http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.cpp#161).

NS_DispatchToMainThread gets the main thread instance by one function call from nsThreadManager (http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThreadManager.cpp#266), while NS_DispatchToCurrentThread needs two function 
calls (http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThreadManager.cpp#276).

Comment 184

[Dave Hylands [:dhylands]]

I agree with your analysis. NS_DispatchToMainThread is more efficient than NS_DispatchToCurrentThread (I didn't actually look it - so you learn something new every day :)

The try run looks good.

Comment 185

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ed100a12c75f
https://hg.mozilla.org/integration/b2g-inbound/rev/7e61acb41c3f
https://hg.mozilla.org/integration/b2g-inbound/rev/b1886a252ed6
https://hg.mozilla.org/integration/b2g-inbound/rev/1b02211faa78
https://hg.mozilla.org/integration/b2g-inbound/rev/b83bb5aa1b25

Comment 186

[[e-mail]]

Comment on attachment 8390370 [details] [diff] [review]
fs_base_interdiff.patch

>-  nsRefPtr<FileSystemBase> fs = do_QueryReferent(mFileSystem);

>-  nsRefPtr<FileSystemBase> fs = do_QueryReferent(mFileSystem);

>-  nsRefPtr<FileSystemBase> fs = do_QueryReferent(mFileSystem);

>-    nsCOMPtr<DeviceStorageFileSystem> f =
>+    nsRefPtr<DeviceStorageFileSystem> f =
I couldn't find the patch that introduced these lines but getting rid of this bogus code is a good move; the nsCOMPtr/nsRefPtr is a bit of a correctness improvement although bug 514280 is hoping to make that error fail to compile while it's pure luck if do_QueryReferent is returning anything sane here.

Comment 187

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed100a12c75f
https://hg.mozilla.org/mozilla-central/rev/7e61acb41c3f
https://hg.mozilla.org/mozilla-central/rev/b1886a252ed6
https://hg.mozilla.org/mozilla-central/rev/1b02211faa78
https://hg.mozilla.org/mozilla-central/rev/b83bb5aa1b25