1187223 - [CloudStorage] Implement File System Provider API

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect, P1)

Description

[Kershaw Chang [:kershaw]]

This bug is used to track implementation of file system provider api [1].

[1] https://developer.chrome.com/apps/fileSystemProvider

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Is the plan to implement this using the framework in bug 1175770? Or will it be separate somehow? If it's separate, it would be great if there were a way to expose it to the code in bug 1175770 without much difficulty.

Comment 2

[Kershaw Chang [:kershaw]]

(In reply to Bill McCloskey (:billm) from comment #1)
> Is the plan to implement this using the framework in bug 1175770? Or will it
> be separate somehow? If it's separate, it would be great if there were a way
> to expose it to the code in bug 1175770 without much difficulty.

I think it will be separate and I think we can definitely expose it to extensions.

Comment 3

[Kershaw Chang [:kershaw]]

Attachment: WebIDL of file system provider API

This idl is basically copied from [1] except the following changes:
1. We'd like to implement a read-only file system first, so other unrelated operations are removed.
2. Callback function types are changed to interface, since it seems that our callback functions doesn't support calling from js to c++.
3. Return type of some functions is changed to promise.


[1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/extensions/api/file_system_provider.idl

Comment 4

[Kershaw Chang [:kershaw]]

Hi Andrew,

Could please you find somebody to review this new WebIDL?

Thanks.

Comment 5

[Andrew Overholt [:overholt]]

(In reply to Kershaw Chang [:kershaw] from comment #4)
> Could please you find somebody to review this new WebIDL?

baku would be a good person for this.  He gets back from vacation on Tuesday.

smaug told me he took a brief look and noted that at least one |AvailableIn="CertifiedApps"| is missing as we don't want to expose this to the web (only B2G certified apps) .

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8644814 [details] [diff] [review]
WebIDL of file system provider API

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

::: dom/webidl/FileSystemProvider.webidl
@@ +1,4 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* 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/. */

Would be nice to have a URL of the spec written here and in any other webidl file.

::: dom/webidl/FileSystemProviderAbortEvent.webidl
@@ +14,5 @@
> +  // An ID of the request to be aborted.
> +  long operationRequestId;
> +};
> +
> +interface FileSystemProviderAbortEvent : FileSystemProviderEvent {

exposed everywhere?

::: dom/webidl/FileSystemProviderCloseFileEvent.webidl
@@ +14,5 @@
> +  // A request ID used to open the file.
> +  long openRequestId;
> +};
> +
> +interface FileSystemProviderCloseFileEvent : FileSystemProviderEvent {

ditto

::: dom/webidl/FileSystemProviderEvent.webidl
@@ +20,5 @@
> +    "NotEmpty",
> +    "InvalidURL",
> +  };
> +
> +interface FileSystemProviderSuccessCallback {

ditto

@@ +24,5 @@
> +interface FileSystemProviderSuccessCallback {
> +  void onSuccess();
> +};
> +
> +interface FileSystemProviderErrorCallback {

ditto

::: dom/webidl/FileSystemProviderGetMetadataEvent.webidl
@@ +3,5 @@
> + * 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/. */
> +
> +// Options for the FileSystemProviderGetMetadataEvent.
> +interface GetMetadataRequestedOptions {

ditto

@@ +33,5 @@
> +  // Mime type for the entry.
> +  DOMString? mimeType;
> +};
> +
> +interface MetadataCallback {

do we expose this interface everywhere? I don't think so.

@@ +41,5 @@
> +// Raised when metadata of a file or a directory at entryPath
> +// is requested. The metadata must be returned with the
> +// successCallback call. In case of an error,
> +// errorCallback must be called.
> +interface FileSystemProviderGetMetadataEvent : FileSystemProviderEvent {

Same here.

::: dom/webidl/FileSystemProviderOpenFileEvent.webidl
@@ +18,5 @@
> +  // Whether the file will be used for reading or writing.
> +  OpenFileMode mode;
> +};
> +
> +interface FileSystemProviderOpenFileEvent : FileSystemProviderEvent {

ditto

::: dom/webidl/FileSystemProviderReadDirectoryEvent.webidl
@@ +20,5 @@
> +// Success callback for the FileSystemProviderReadDirectoryEvent. If more
> +// entries will be returned, then hasMore must be true, and it
> +// has to be called again with additional entries. If no more entries are
> +// available, then hasMore must be set to false.
> +interface EntriesCallback {

ditto

@@ +28,5 @@
> +// Raised when contents of a directory at directoryPath are
> +// requested. The results must be returned in chunks by calling the
> +// successCallback several times. In case of an error,
> +// errorCallback must be called.
> +interface FileSystemProviderReadDirectoryEvent : FileSystemProviderEvent {

ditto

::: dom/webidl/FileSystemProviderReadFileEvent.webidl
@@ +24,5 @@
> +// Success callback for FileSystemProviderReadFileEvent. If more
> +// data will be returned, then hasMore must be true, and it
> +// has to be called again with additional entries. If no more data is
> +// available, then hasMore must be set to false.
> +interface FileDataCallback {

ditto

@@ +28,5 @@
> +interface FileDataCallback {
> +  void onReadFile(ArrayBuffer data, boolean hasMore);
> +};
> +
> +interface FileSystemProviderReadFileEvent : FileSystemProviderEvent {

ditto

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: 0001-Bug-1187223-WebIDL-v2.patch

Thanks for your previous review, baku.

The comments are addressed and please also note that I move the callback interfaces into event interfaces, so the developer can directly call the callback function. Please let me know if this modification makes sense to you.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8646464 [details] [diff] [review]
0001-Bug-1187223-WebIDL-v2.patch

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

::: dom/webidl/FileSystemProviderAbortEvent.webidl
@@ +1,5 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* 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/. */
> +

* The origin of this IDL file is
* https://developer.chrome.com/apps/fileSystemProvider

::: dom/webidl/FileSystemProviderGetMetadataEvent.webidl
@@ +6,5 @@
> + * The origin of this IDL file is
> + * https://developer.chrome.com/apps/fileSystemProvider
> + */
> +
> +// Options for the FileSystemProviderGetMetadataEvent.

[Pref="device.storage.enabled", CheckAnyPermissions="filesystemprovider", AvailableIn="CertifiedApps"]

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: WebIDL-v3

WebIDL with previous comment addressed.

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: wip

Comment 11

[Gary Chen [:xeonchen]]

Attachment: [v1] nsICloudStorageCallback.idl

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: [v1] nsIFileSystemProvider.idl

Comment 13

[Gary Chen [:xeonchen]]

Attachment: [v2] nsICloudStorageCallback.idl

Comment 14

[Gary Chen [:xeonchen]]

Attachment: [v1] nsICloudStorageCallback classes

WIP

Comment 15

[Eden Chuang[:edenchuang]]

Attachment: [v1] nsICloudStorageRequest.idl

Comment 16

[Eden Chuang[:edenchuang]]

Attachment: [v2]nsICloudStorageRequest.idl

Comment 17

[Eden Chuang[:edenchuang]]

Attachment: [v3]nsICloudStorageRequest.idl and nsICloudStorageRequest classes

Comment 18

[Kershaw Chang [:kershaw]]

Attachment: Part1:FileSystemProvider

Comment 19

[Kershaw Chang [:kershaw]]

Attachment: Part2:nsICloudStorageCallback

Comment 20

[Kershaw Chang [:kershaw]]

Attachment: Part3:nsICloudStorageRequest

Comment 21

[Kershaw Chang [:kershaw]]

Just merge patches. Let's move on based on this.

Comment 22

[Kershaw Chang [:kershaw]]

Attachment: WebIDL-v4

1. Update some data types.
2. Use unsigned long to replace long.

Comment 23

[Kershaw Chang [:kershaw]]

Attachment: Part1:FileSystemProvider

Integrate with nsICloudStorageRequest and nsICloudStorageCallback.

Comment 24

[Kershaw Chang [:kershaw]]

Attachment: Part2:nsICloudStorageCallback

Update some data types.

Comment 25

[Kershaw Chang [:kershaw]]

Attachment: Part3:nsICloudStorageRequest

Update some data types.

Comment 26

[Eden Chuang[:edenchuang]]

Attachment: Part3:nsICloudStorageRequest

Comment 27

[Eden Chuang[:edenchuang]]

Attachment: Part3:nsICloudStorageRequest

Update nsCloudStorageRequest implementation and nsCloudStorageRequestManager implementation

Comment 28

[Eden Chuang[:edenchuang]]

Attachment: Part3:nsICloudStorageRequest

Update nsCloudStorageRequest and nsCloudStorageRequestManager implementation. Remove duplicate attachments.

Comment 29

[Eden Chuang[:edenchuang]]

Attachment: Part1:nsICloudStorageRequest and nsICloudStorageCallback

Integrate nsICloudStorageRequest and nsICloudStorageCallback implementation into one part

Comment 30

[Eden Chuang[:edenchuang]]

Attachment: Part2:FileSystemProvider WebIDL and implementation

Integrate FileSystemProvider WebIDL and implementation into one patch.

Comment 31

[Eden Chuang[:edenchuang]]

Attachment: Part2:FileSystemProvider WebIDL and implementation

Integrate FileSystemProvider WebIDL and implementation into one patch.

Comment 32

[Eden Chuang[:edenchuang]]

Attachment: Part1:nsICloudStorageRequest and nsICloudStorageCallback

update nsICloudStorageCallback function implementation

Comment 33

[Kershaw Chang [:kershaw]]

Attachment: IDLs

Hi baku,

Before asking you to review a huge bunch of code, we would like ask you to take a look of IDLs first. I think it should be helpful for understand the code.

Basically, this bug is aimed to implement Google's file system provider API [1][2]. The architecture of XPCOM components is in [3]. I also drew some sequence diagrams below for demonstrating the interaction between XPCOM components.
[4] The sequence of creating a new file system.
[5] The sequence of creating a read file request.
[6] The sequence of returning the data.

We also have a problem on naming. The prefix of *.webidl files is FileSystemProvider, but the prefix of *.idl files is nsICloudStorage due to historical reasons. We are not sure how to modify the prefix of *.idl files. Maybe you can provide a good suggestion to us.
Please let me know if this is enough for you to understand what we are trying to do. Any suggestions will be greatly appreciated. Thanks.

[1] https://developer.chrome.com/apps/fileSystemProvider
[2] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/extensions/api/file_system_provider.idl
[3] https://docs.google.com/drawings/d/1lkmorRG95pzwE3RIwqB65hhx3RCZNyKKXmUZuaSH2qM/edit?usp=sharing
[4] https://docs.google.com/drawings/d/177wdYEtjaLT9Ic892PCnWUTN4FcV6hMCt4TxRkXJ1qQ/edit?usp=sharing
[5] https://docs.google.com/drawings/d/1m2KERXSf7nZIaiudAQJKLnSOGPKPOnp6raYl-MBjQ8U/edit?usp=sharing
[6] https://docs.google.com/drawings/d/1_wu3Xz1DMj8XgEUGaXEih3a8dE9KlmcjDBxtguRIVy4/edit?usp=sharing

Comment 34

[Andrea Marchesini [:baku]]

Comment on attachment 8671260 [details] [diff] [review]
IDLs

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

why are you implementing XPCOM interfaces when we have WebIDL ones?

::: dom/cloudstorage/interfaces/nsICloudStorage.idl
@@ +7,5 @@
> +interface nsIArray;
> +
> +/**
> + * XPCOM component which represents an opened file.
> + * This is the same as OpenedFile defined in FileSystemProvider.webidl.

why do you need to have the XPCOM component?

Comment 35

[Kershaw Chang [:kershaw]]

(In reply to Andrea Marchesini (:baku) from comment #34)
> Comment on attachment 8671260 [details] [diff] [review]
> IDLs
> 
> Review of attachment 8671260 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> why are you implementing XPCOM interfaces when we have WebIDL ones?
> 
> ::: dom/cloudstorage/interfaces/nsICloudStorage.idl
> @@ +7,5 @@
> > +interface nsIArray;
> > +
> > +/**
> > + * XPCOM component which represents an opened file.
> > + * This is the same as OpenedFile defined in FileSystemProvider.webidl.
> 
> why do you need to have the XPCOM component?

Yes, I understand we have some XPCOM interfaces which are the same as the WebIDL ones. We think it would be nice to define XPCOM interfaces first before starting implementation. For passing information between components, it's difficult to avoid defining the same interface in XPIDL again.
I also found that there are some similar cases in gecko, such as MozIccInfo.webidl and nsIIccInfo.idl.

If you still think this is a bad design, could you please give us some suggestions?
Thanks.

Comment 36

[Eden Chuang[:edenchuang]]

(In reply to Andrea Marchesini (:baku) from comment #34)
> Comment on attachment 8671260 [details] [diff] [review]
> IDLs
> 
> Review of attachment 8671260 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> why are you implementing XPCOM interfaces when we have WebIDL ones?
> 
> ::: dom/cloudstorage/interfaces/nsICloudStorage.idl
> @@ +7,5 @@
> > +interface nsIArray;
> > +
> > +/**
> > + * XPCOM component which represents an opened file.
> > + * This is the same as OpenedFile defined in FileSystemProvider.webidl.
> 
> why do you need to have the XPCOM component?

Hello Baku

Sorry to bother you.

We need this XPCOM component to package and extend the transferring data in Gecko.
There are several data structures are transferred between Gecko. For example, RequestOption, OpenedFileInfo and FileSystemInfo.
However these information saved in Gecko with raw data type, for example, nsString, uint64_t and others, and these data might not saved in the same component. To package these data, we create these XPCOM components.

Of course, we can transfer them with raw data type, but it might make the component be hard to extend in the future. Considering the following case.

If we transfer OpenFileInfo with raw data type, the interface will be
unsigned long GetOpenedFileRequestId(in unsigned long OpenedFileIndex)
DOMString GetOpenedFilePath(in unsigned long OpenedFileIndex)
unsigned short GetOpenedFileMode(in unsigned long OpenedFileIndex)

If the OpenFileInfo needed to be extended in the future. 
we might need to create a new interface in idl to get the extended data. For example
unsigned long GetOpenedFileSize(in unsigned long OpenedFileIndex)

This might involve lots of implementation in many files.

But if we package it as nsOpenedFileInfo, the interface becomes
nsOpenedFileInfo GetOpenedFileInfo(unsigned long OpenedFileIndex)

If we need to extend nsOpenedFileInfo, we can only modify the nsOpenedFileInfo and no other interface will be changed.

Another reason is that interface defined in webidl can not be described in idl.
Although we define these data structure both in XPCOM and webidl, but they can share the implementation.

That's the reason why we need these XPCOM component.

If I misunderstood something or any suggestions, please feel free to tell me. Thanks. :)

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

I think Eden removed the needinfo on the previous comment accidentally. So setting it back to Andrea.

Comment 38

[Andrea Marchesini [:baku]]

Sorry for the delay, I completely missed this NI!

> We need this XPCOM component to package and extend the transferring data in
> Gecko.

Do you mean the Structured Clone algorithm? For this a WebIDL interface is ok.
But maybe I don't understand your point.

> unsigned long GetOpenedFileRequestId(in unsigned long OpenedFileIndex)

Can it be:

nsISupports GetOpenedFileRequestId(in nsISupports OpenedFileIndex)
DOMString GetOpenedFilePath(in nsISupports OpenedFileIndex)
unsigned short GetOpenedFileMode(in nsISupports OpenedFileIndex)

Normally a WebIDL interface is also a nsISupports but they are 'builtinclass', so can just cast it to your real implementation.
Tell me if this makes sense and helps. Maybe we can talk on IRC too.

Comment 39

[Andrew Overholt [:overholt]]

Is this related to https://hacks.mozilla.org/2012/07/why-no-filesystem-api-in-firefox/ at all?

Comment 40

[Andrew Overholt [:overholt]]

(In reply to Andrew Overholt [:overholt] from comment #39)
> Is this related to
> https://hacks.mozilla.org/2012/07/why-no-filesystem-api-in-firefox/ at all?

After further investigation, it appears the answer is no.

Comment 41

[Kershaw Chang [:kershaw]]

Attachment: Part1: WebIDL Implementations

Comment 42

[Kershaw Chang [:kershaw]]

Attachment: Part2: Implementations of XPCOM components

Comment 43

[Kershaw Chang [:kershaw]]

Attachment: Part3: Test cases

Comment 44

[Kershaw Chang [:kershaw]]

Hi baku,

I just uploaded the first version of our implementation and also updated the flow diagrams in comment#33. I understand that you has some disagreements with our IDLs before, but with these c++ implementations, you could be able to understand why we were designing those XPCOM components in this way. So, please take a quick look at these patches and let us know what do you think.

Here are some notes for you:
1. Some low level components are not included in this bug, such as nsIVirtualFileSystemService and nsIVirtialFileSystemCallback. Those components are belong to bug 1188230.
2. We didn't implement IPC part so far. Passing a lot of data between processes should be very slow, so we are wondering if it is possible to make this API to be only used in parent process.

Appreciate for any comments. Thanks.

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Kershaw Chang [:kershaw] from comment #44)
> 2. We didn't implement IPC part so far. Passing a lot of data between
> processes should be very slow, so we are wondering if it is possible to make
> this API to be only used in parent process.
> 

If I am not wrong, this means that only the System app (which might also eventually run oop) will be able to use it. That's not what we want.

Comment 46

[Kershaw Chang [:kershaw]]

> 
> If I am not wrong, this means that only the System app (which might also
> eventually run oop) will be able to use it. That's not what we want.
>
Yes, you are correct. Currently, this API only works in parent process.
However, we will still work on IPC part and try our best to tune the performance.

Comment 47

[Eden Chuang[:edenchuang]]

After discuss with Baku, we are going to do following things
1. remove useless idl design.
2. implement IPC for content process.

Comment 48

[Kershaw Chang [:kershaw]]

Attachment: Part1: WebIDL Implementations

Comment 49

[Kershaw Chang [:kershaw]]

Attachment: Part2: VirtualFileSystem Service and RequestManager

Comment 50

[Kershaw Chang [:kershaw]]

Attachment: Part3: IPC

Comment 51

[Kershaw Chang [:kershaw]]

Attachment: Part4: Tests

Comment 52

[Kershaw Chang [:kershaw]]

Hi baku,

As we discussed at Orlando before, I've removed some redundant XPCOM interfaces and also added IPC support in this version. Please take a look at these patches. Thanks.

Comment 53

[Andrea Marchesini [:baku]]

Comment on attachment 8725051 [details] [diff] [review]
Part1: WebIDL Implementations

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

I would like to see this patch again once the comments are applied. Answer my questions too. Thanks!

::: dom/base/Navigator.cpp
@@ +2985,5 @@
> +    if (!mWindow || !mWindow->GetOuterWindow() || !mWindow->GetDocShell()) {
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return nullptr;
> +    }
> +    mFileSystemProvider = FileSystemProvider::Create(mWindow);

move all this mWindow check into ::Create and pass aRv too.

::: dom/virtualfilesystem/FileSystemProvider.cpp
@@ +4,5 @@
> + * 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 "FileSystemProvider.h"
> +#include "mozilla/dom/FileSystemProviderAbortEvent.h"

you don't need all of these mozilla/dom if these headers are in the same directory.

@@ +11,5 @@
> +#include "mozilla/dom/FileSystemProviderGetMetadataEvent.h"
> +#include "mozilla/dom/FileSystemProviderOpenFileEvent.h"
> +#include "mozilla/dom/FileSystemProviderReadDirectoryEvent.h"
> +#include "mozilla/dom/FileSystemProviderReadFileEvent.h"
> +#include "mozilla/dom/FileSystemProviderUnmountEvent.h"

these too.

@@ +56,5 @@
> +
> +FileSystemProvider::FileSystemProvider(nsPIDOMWindow* aWindow)
> +  : DOMEventTargetHelper(aWindow)
> +  , mEventDispatcher(new FileSystemProviderEventDispatcher(this))
> +{

MOZ_ASSERT(aWindow);

@@ +61,5 @@
> +}
> +
> +FileSystemProvider::~FileSystemProvider()
> +{
> +  mEventDispatcher->Forget();

1. What is this forget about?
2. Whatever it is, it looks like nsRefPtr::forget(). choose a different name.

@@ +70,5 @@
> +FileSystemProvider::Init()
> +{
> +  mVirtualFileSystemService =
> +    VirtualFileSystemServiceFactory::AutoCreateVirtualFileSystemService();
> +  return !!mVirtualFileSystemService;

return some nsresult or take a ErrorResult.

@@ +83,5 @@
> +/* static */ already_AddRefed<FileSystemProvider>
> +FileSystemProvider::Create(nsPIDOMWindow* aWindow)
> +{
> +  RefPtr<FileSystemProvider> provider = new FileSystemProvider(aWindow);
> +  return (provider->Init()) ? provider.forget() : nullptr;

take a ErrorResult obj as param.

@@ +96,5 @@
> +  RefPtr<Promise> promise = Promise::Create(global, aRv);
> +  if (NS_WARN_IF(aRv.Failed())) {
> +    return nullptr;
> +  }
> +  mPendingRequestPromises[++sRequestId] = promise;

Instead doing this, can you pass the promise object to the callback object?
FileSystemProviderMountUmountCallback can just resolve/reject the promise directly.

@@ +99,5 @@
> +  }
> +  mPendingRequestPromises[++sRequestId] = promise;
> +
> +  nsCOMPtr<nsIVirtualFileSystemCallback> callback =
> +    new FileSystemProviderMountUnmountCallback(this);

where is this defined?

@@ +104,5 @@
> +  nsresult rv = mVirtualFileSystemService->Mount(sRequestId,
> +                                                 aOptions,
> +                                                 mEventDispatcher,
> +                                                 callback);
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +105,5 @@
> +                                                 aOptions,
> +                                                 mEventDispatcher,
> +                                                 callback);
> +  if (NS_FAILED(rv)) {
> +    mPendingRequestPromises.erase(sRequestId);

populate the hashtable only if this doesn't fail instead adding and here remove it.

@@ +134,5 @@
> +    aRv.Throw(rv);
> +    return nullptr;
> +  }
> +
> +  return promise.forget();

See the previous comments for this unmount()

@@ +144,5 @@
> +                        ErrorResult& aRv)
> +{
> +  nsresult rv = mVirtualFileSystemService->GetFileSysetmInfoById(aFileSystemId,
> +                                                                 aInfo.SetValue());
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +145,5 @@
> +{
> +  nsresult rv = mVirtualFileSystemService->GetFileSysetmInfoById(aFileSystemId,
> +                                                                 aInfo.SetValue());
> +  if (NS_FAILED(rv)) {
> +    aInfo.SetNull();

this is not needed.

@@ +154,5 @@
> +void
> +FileSystemProvider::GetAll(Nullable<nsTArray<FileSystemInfo>>& aRetVal, ErrorResult& aRv)
> +{
> +  aRetVal.SetValue().TruncateLength(0);
> +  mVirtualFileSystemService->GetAllFileSystemInfo(aRetVal.SetValue());

this method should not receive ErrorResult if it doesn't throw exceptions. Remove [throws] from the webidl.

@@ +165,5 @@
> +  const FileSystemProviderRequestedOptions& aOptions,
> +  virtualfilesystem::BaseVirtualFileSystemRequestManager* aRequestManager)
> +{
> +  if (!aRequestManager) {
> +    return NS_ERROR_FAILURE;

NS_WARN_IF

@@ +192,5 @@
> +    case FileSystemProviderRequestedOptions::Type_UnmountRequestedOptions:
> +      event = new FileSystemProviderUnmountEvent(this, aRequestManager);
> +      break;
> +    default:
> +      MOZ_ASSERT(false);

MOZ_CRASH

@@ +201,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  return DispatchTrustedEvent(event);

rv = ...
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

 return NS_OK;

@@ +205,5 @@
> +  return DispatchTrustedEvent(event);
> +}
> +
> +void
> +FileSystemProvider::NotifyMountUnmountResult(uint32_t aRequestId, bool aSucceeded)

All of this should go away.

::: dom/virtualfilesystem/FileSystemProvider.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_FileSystemProvider_h
> +#define mozilla_dom_FileSystemProvider_h
> +
> +#include <map>

use one of the gecko hashtable.

@@ +13,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +namespace virtualfilesystem {

I don't think we want an additional namespace here.

@@ +61,5 @@
> +  IMPL_EVENT_HANDLER(abortrequested)
> +
> +  static already_AddRefed<FileSystemProvider> Create(nsPIDOMWindow* aWindow);
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

80chars max in all this file.

@@ +87,5 @@
> +  void NotifyMountUnmountResult(uint32_t aRequestId, bool aSucceeded);
> +
> +  RefPtr<FileSystemProviderEventDispatcher> mEventDispatcher;
> +  RefPtr<BaseVirtualFileSystemService> mVirtualFileSystemService;
> +  std::map<uint32_t, RefPtr<Promise>> mPendingRequestPromises;

get rid of this.

::: dom/virtualfilesystem/FileSystemProviderAbortEvent.cpp
@@ +33,5 @@
> +  nsISupports* aParent,
> +  uint32_t aRequestId,
> +  const nsAString& aFileSystemId,
> +  const FileSystemProviderRequestedOptions& aOptions)
> +  : FileSystemProviderRequestedOptionsBase(aParent, aRequestId, aFileSystemId, aOptions)

80chars max everywhere.

@@ +55,5 @@
> +  EventTarget* aOwner,
> +  BaseVirtualFileSystemRequestManager* aManager)
> +  : FileSystemProviderEventWrap(
> +    aOwner, aManager, NS_LITERAL_STRING("abortrequested"))
> +{

MOZ_ASSERT(aOwner)
MOZ_ASSERT(aManager)

::: dom/virtualfilesystem/FileSystemProviderAbortEvent.h
@@ +15,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class AbortRequestedOptions final : public FileSystemProviderRequestedOptionsBase

80chars max.

@@ +36,5 @@
> +  ~AbortRequestedOptions() = default;
> +};
> +
> +class FileSystemProviderAbortEvent final
> +  : public FileSystemProviderEventWrap<

strange indentation

::: dom/virtualfilesystem/FileSystemProviderCloseFileEvent.cpp
@@ +54,5 @@
> +FileSystemProviderCloseFileEvent::FileSystemProviderCloseFileEvent(
> +  EventTarget* aOwner,
> +  BaseVirtualFileSystemRequestManager* aManager)
> +  : FileSystemProviderEventWrap(
> +    aOwner, aManager, NS_LITERAL_STRING("closefilerequested"))

strange indentation here too.

::: dom/virtualfilesystem/FileSystemProviderCloseFileEvent.h
@@ +36,5 @@
> +  virtual ~CloseFileRequestedOptions() = default;
> +};
> +
> +class FileSystemProviderCloseFileEvent final
> +  : public FileSystemProviderEventWrap<

here and elsewhere.

::: dom/virtualfilesystem/FileSystemProviderEvent.cpp
@@ +28,5 @@
> +  : mParent(aParent)
> +  , mRequestId(aRequestId)
> +  , mFileSystemId(aFileSystemId)
> +  , mOptions(aOptions)
> +{

MOZ_ASSERT(aParent);

@@ +76,5 @@
> +  : Event(aOwner, nullptr, nullptr)
> +  , mRequestManager(aManager)
> +  , mOptions(nullptr)
> +  , mEventName(aEventName)
> +{

MOZ_ASSERT(everything, in particular aManager).

::: dom/virtualfilesystem/FileSystemProviderEvent.h
@@ +116,5 @@
> +      MOZ_ASSERT(false, "Invalid options.");
> +      return NS_ERROR_INVALID_ARG;
> +    }
> +
> +    RefPtr<WebIDLOptions> options = new WebIDLOptions(mOwner,

don't use this temporary variable. Use mOptions directly.

::: dom/virtualfilesystem/FileSystemProviderEventOptions.h
@@ +393,5 @@
> +  FileSystemProviderRequestedOptions(const FileSystemProviderRequestedOptions& aOther)
> +  {
> +    switch (aOther.GetType()) {
> +      case Type_AbortRequestedOptions:
> +        new (ptr_FileSystemProviderAbortRequestedOptions())

which syntax is this?

::: dom/virtualfilesystem/FileSystemProviderGetMetadataEvent.h
@@ +22,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(GetMetadataRequestedOptions,
> +                                                         FileSystemProviderRequestedOptionsBase)
> +
> +  explicit GetMetadataRequestedOptions(nsISupports* aParent,

remove this explicit

::: dom/virtualfilesystem/FileSystemProviderOpenFileEvent.cpp
@@ +62,5 @@
> +  BaseVirtualFileSystemRequestManager* aManager)
> +  : FileSystemProviderEventWrap(
> +    aOwner, aManager, NS_LITERAL_STRING("openfilerequested"))
> +{
> +

extra line

::: dom/virtualfilesystem/FileSystemProviderOpenFileEvent.h
@@ +23,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(OpenFileRequestedOptions,
> +                                                         FileSystemProviderRequestedOptionsBase)
> +
> +  explicit OpenFileRequestedOptions(nsISupports* aParent,

ditto

::: dom/virtualfilesystem/FileSystemProviderReadDirectoryEvent.cpp
@@ +78,5 @@
> +    entries.AppendElement(aEntries[i]);
> +  }
> +
> +  nsCOMPtr<nsIVirtualFileSystemReadDirectoryRequestValue> value =
> +    new virtualfilesystem::nsVirtualFileSystemReadDirectoryRequestValue(entries);

can you pass the sequence directly instead converting it to an array? Plus a sequence is a kind of array as well.

::: dom/virtualfilesystem/FileSystemProviderUnmountEvent.cpp
@@ +50,5 @@
> +  BaseVirtualFileSystemRequestManager* aManager)
> +  : FileSystemProviderEventWrap(
> +    aOwner, aManager, NS_LITERAL_STRING("unmountrequested"))
> +{
> +

extra line

Comment 54

[Kershaw Chang [:kershaw]]

(In reply to Andrea Marchesini (:baku) from comment #53)
> Comment on attachment 8725051 [details] [diff] [review]
> Part1: WebIDL Implementations
> 
> Review of attachment 8725051 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would like to see this patch again once the comments are applied. Answer
> my questions too. Thanks!
> 

Thanks for reviewing. I will address these comments soon.
 
> @@ +61,5 @@
> > +}
> > +
> > +FileSystemProvider::~FileSystemProvider()
> > +{
> > +  mEventDispatcher->Forget();
> 
> 1. What is this forget about?

This is for breaking the reference cycle between FileSystemProvider and mEventDispatcher.
mEventDispatcher holds a raw pointer to FileSystemProvider. In order to prevent a dangling pointer, Forget() is used to set this raw pointer to nullptr.

> 2. Whatever it is, it looks like nsRefPtr::forget(). choose a different name.

Maybe rename to ForgetFileSystemProvider?

> @@ +96,5 @@
> > +  RefPtr<Promise> promise = Promise::Create(global, aRv);
> > +  if (NS_WARN_IF(aRv.Failed())) {
> > +    return nullptr;
> > +  }
> > +  mPendingRequestPromises[++sRequestId] = promise;
> 
> Instead doing this, can you pass the promise object to the callback object?
> FileSystemProviderMountUmountCallback can just resolve/reject the promise
> directly.

I think I can do this.

> @@ +99,5 @@
> > +  }
> > +  mPendingRequestPromises[++sRequestId] = promise;
> > +
> > +  nsCOMPtr<nsIVirtualFileSystemCallback> callback =
> > +    new FileSystemProviderMountUnmountCallback(this);
> 
> where is this defined?

FileSystemProviderMountUnmountCallback is defined in FileSystemProvider.h as MountUnmountResultCallback<FileSystemProvider>.
MountUnmountResultCallback is defined in FileSystemProviderCommon.h.

> @@ +105,5 @@
> > +                                                 aOptions,
> > +                                                 mEventDispatcher,
> > +                                                 callback);
> > +  if (NS_FAILED(rv)) {
> > +    mPendingRequestPromises.erase(sRequestId);
> 
> populate the hashtable only if this doesn't fail instead adding and here
> remove it.

OK.

> @@ +154,5 @@
> > +void
> > +FileSystemProvider::GetAll(Nullable<nsTArray<FileSystemInfo>>& aRetVal, ErrorResult& aRv)
> > +{
> > +  aRetVal.SetValue().TruncateLength(0);
> > +  mVirtualFileSystemService->GetAllFileSystemInfo(aRetVal.SetValue());
> 
> this method should not receive ErrorResult if it doesn't throw exceptions.
> Remove [throws] from the webidl.

OK. 

> ::: dom/virtualfilesystem/FileSystemProvider.h
> @@ +6,5 @@
> > +
> > +#ifndef mozilla_dom_FileSystemProvider_h
> > +#define mozilla_dom_FileSystemProvider_h
> > +
> > +#include <map>
> 
> use one of the gecko hashtable.

I've seen someone also use map in gecko. Can I still use map here?

> ::: dom/virtualfilesystem/FileSystemProviderEventOptions.h
> @@ +393,5 @@
> > +  FileSystemProviderRequestedOptions(const FileSystemProviderRequestedOptions& aOther)
> > +  {
> > +    switch (aOther.GetType()) {
> > +      case Type_AbortRequestedOptions:
> > +        new (ptr_FileSystemProviderAbortRequestedOptions())
> 
> which syntax is this?

It's placement new. Basically, this part is copied from IPDL code in https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PDeviceStorageRequest.cpp#610. 

> ::: dom/virtualfilesystem/FileSystemProviderReadDirectoryEvent.cpp
> @@ +78,5 @@
> > +    entries.AppendElement(aEntries[i]);
> > +  }
> > +
> > +  nsCOMPtr<nsIVirtualFileSystemReadDirectoryRequestValue> value =
> > +    new virtualfilesystem::nsVirtualFileSystemReadDirectoryRequestValue(entries);
> 
> can you pass the sequence directly instead converting it to an array? Plus a
> sequence is a kind of array as well.

OK.

Comment 55

[Kershaw Chang [:kershaw]]

Attachment: Part1: WebIDL Implementations - v2

Address last review comments.

Comment 56

[Kershaw Chang [:kershaw]]

Attachment: Part2: VirtualFileSystem Service and RequestManager - v2

Rebase and fix some nits.

Comment 57

[Kershaw Chang [:kershaw]]

Attachment: Part3: IPC - v2

Rebase and fix nits.

Comment 58

[Kershaw Chang [:kershaw]]

Attachment: Part4: Tests - v2

Rebase

Comment 59

[Kershaw Chang [:kershaw]]

Attachment: Interdiff for v1 and v2 of part1.

Comment 60

[Eden Chuang[:edenchuang]]

Attachment: Part5: Write Part Support - v1

FileSystemProvider API implementation: write part support.

Comment 61

[Eden Chuang[:edenchuang]]

Documentation for Cloud Storage Support Framework and FileSystemProvider API usage.
https://wiki.mozilla.org/Firefox_OS/Cloud_Storage

Comment 63

[Kershaw Chang [:kershaw]]

After discussing with baku, we decide cancel the review request.

Thanks for baku's effort.