Closed Bug 1187223 Opened 5 years ago Closed 4 years ago

[CloudStorage] Implement File System Provider API

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED WONTFIX
2.6 S7 - 2/12
Tracking Status
firefox42 --- affected

People

(Reporter: kershaw, Assigned: kershaw)

References

Details

Attachments

(6 files, 33 obsolete files)

114.22 KB, patch
Details | Diff | Splinter Review
61.15 KB, patch
Details | Diff | Splinter Review
55.29 KB, patch
Details | Diff | Splinter Review
31.97 KB, patch
Details | Diff | Splinter Review
65.64 KB, patch
Details | Diff | Splinter Review
99.24 KB, patch
Details | Diff | Splinter Review
This bug is used to track implementation of file system provider api [1].

[1] https://developer.chrome.com/apps/fileSystemProvider
Assignee: nobody → kechang
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.
(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.
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
Hi Andrew,

Could please you find somebody to review this new WebIDL?

Thanks.
Flags: needinfo?(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) .
Flags: needinfo?(overholt)
Attachment #8644814 - Flags: review?(amarchesini)
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
Attachment #8644814 - Flags: review?(amarchesini) → review-
Attached patch 0001-Bug-1187223-WebIDL-v2.patch (obsolete) — Splinter Review
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.
Attachment #8644814 - Attachment is obsolete: true
Attachment #8646464 - Flags: review?(amarchesini)
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"]
Attachment #8646464 - Flags: review?(amarchesini) → review+
Attached patch WebIDL-v3 (obsolete) — Splinter Review
WebIDL with previous comment addressed.
Attachment #8646464 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attached file [v1] nsIFileSystemProvider.idl (obsolete) —
Summary: Implement File System Provider API → [CloudStorage] Implement File System Provider API
Blocks: 1164750
Attached patch [v2] nsICloudStorageCallback.idl (obsolete) — Splinter Review
Attachment #8648657 - Attachment is obsolete: true
Attachment #8648604 - Attachment is obsolete: true
Attached patch [v2]nsICloudStorageRequest.idl (obsolete) — Splinter Review
Attachment #8649653 - Attachment is obsolete: true
Target Milestone: --- → FxOS-S8 (02Oct)
Attached patch Part1:FileSystemProvider (obsolete) — Splinter Review
Attachment #8647907 - Attachment is obsolete: true
Attached patch Part2:nsICloudStorageCallback (obsolete) — Splinter Review
Attachment #8649647 - Attachment is obsolete: true
Attachment #8649649 - Attachment is obsolete: true
Attached patch Part3:nsICloudStorageRequest (obsolete) — Splinter Review
Attachment #8652155 - Attachment is obsolete: true
Just merge patches. Let's move on based on this.
Attached patch WebIDL-v4 (obsolete) — Splinter Review
1. Update some data types.
2. Use unsigned long to replace long.
Attachment #8647906 - Attachment is obsolete: true
Attached patch Part1:FileSystemProvider (obsolete) — Splinter Review
Integrate with nsICloudStorageRequest and nsICloudStorageCallback.
Attachment #8653920 - Attachment is obsolete: true
Attached patch Part2:nsICloudStorageCallback (obsolete) — Splinter Review
Update some data types.
Attachment #8653921 - Attachment is obsolete: true
Attached patch Part3:nsICloudStorageRequest (obsolete) — Splinter Review
Update some data types.
Attachment #8653922 - Attachment is obsolete: true
Attached patch Part3:nsICloudStorageRequest (obsolete) — Splinter Review
Attachment #8655895 - Attachment is obsolete: true
Attached patch Part3:nsICloudStorageRequest (obsolete) — Splinter Review
Update nsCloudStorageRequest implementation and nsCloudStorageRequestManager implementation
Attached patch Part3:nsICloudStorageRequest (obsolete) — Splinter Review
Update nsCloudStorageRequest and nsCloudStorageRequestManager implementation. Remove duplicate attachments.
Attachment #8656483 - Attachment is obsolete: true
Attachment #8656649 - Attachment is obsolete: true
Integrate nsICloudStorageRequest and nsICloudStorageCallback implementation into one part
Attachment #8655894 - Attachment is obsolete: true
Attachment #8656652 - Attachment is obsolete: true
Integrate FileSystemProvider WebIDL and implementation into one patch.
Integrate FileSystemProvider WebIDL and implementation into one patch.
Attachment #8655890 - Attachment is obsolete: true
Attachment #8655892 - Attachment is obsolete: true
Attachment #8659009 - Attachment is obsolete: true
update nsICloudStorageCallback function implementation
Attachment #8659008 - Attachment is obsolete: true
Attached patch IDLs (obsolete) — Splinter Review
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
Attachment #8659010 - Attachment is obsolete: true
Attachment #8659119 - Attachment is obsolete: true
Attachment #8671260 - Flags: feedback?(amarchesini)
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?
Attachment #8671260 - Flags: feedback?(amarchesini) → feedback-
(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.
Flags: needinfo?(amarchesini)
(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. :)
Flags: needinfo?(amarchesini)
I think Eden removed the needinfo on the previous comment accidentally. So setting it back to Andrea.
Flags: needinfo?(amarchesini)
Target Milestone: FxOS-S8 (02Oct) → FxOS-S11 (13Nov)
Priority: -- → P1
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.
Flags: needinfo?(amarchesini) → needinfo?(echuang)
Is this related to https://hacks.mozilla.org/2012/07/why-no-filesystem-api-in-firefox/ at all?
Flags: needinfo?(kechang)
(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.
Flags: needinfo?(kechang)
Attached patch Part1: WebIDL Implementations (obsolete) — Splinter Review
Attachment #8671260 - Attachment is obsolete: true
Attachment #8692527 - Flags: feedback?(amarchesini)
Attachment #8692528 - Flags: feedback?(amarchesini)
Attached patch Part3: Test cases (obsolete) — Splinter Review
Attachment #8692529 - Flags: feedback?(amarchesini)
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.
Flags: needinfo?(echuang)
(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.
> 
> 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.
After discuss with Baku, we are going to do following things
1. remove useless idl design.
2. implement IPC for content process.
Attachment #8692527 - Flags: feedback?(amarchesini)
Attachment #8692528 - Flags: feedback?(amarchesini)
Attachment #8692529 - Flags: feedback?(amarchesini)
Depends on: 772528
Blocks: 1210336
Blocks: 1210338
Blocks: 1235960
Target Milestone: FxOS-S11 (13Nov) → 2.6 S7 - 2/12
Attached patch Part1: WebIDL Implementations (obsolete) — Splinter Review
Attachment #8692527 - Attachment is obsolete: true
Attachment #8692528 - Attachment is obsolete: true
Attachment #8692529 - Attachment is obsolete: true
Attachment #8725051 - Flags: review?(amarchesini)
Attachment #8725052 - Flags: review?(amarchesini)
Attached patch Part3: IPC (obsolete) — Splinter Review
Attachment #8725053 - Flags: review?(amarchesini)
Attached patch Part4: Tests (obsolete) — Splinter Review
Attachment #8725054 - Flags: review?(amarchesini)
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 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
Attachment #8725051 - Flags: review?(amarchesini) → review-
(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.
Address last review comments.
Attachment #8725051 - Attachment is obsolete: true
Attachment #8725052 - Attachment is obsolete: true
Attachment #8725053 - Attachment is obsolete: true
Attachment #8725054 - Attachment is obsolete: true
Attachment #8725052 - Flags: review?(amarchesini)
Attachment #8725053 - Flags: review?(amarchesini)
Attachment #8725054 - Flags: review?(amarchesini)
Attachment #8732804 - Flags: review?(amarchesini)
Rebase and fix some nits.
Attachment #8732805 - Flags: review?(amarchesini)
Attached patch Part3: IPC - v2Splinter Review
Rebase and fix nits.
Attachment #8732806 - Flags: review?(amarchesini)
Rebase
Attachment #8732807 - Flags: review?(amarchesini)
FileSystemProvider API implementation: write part support.
Attachment #8738861 - Flags: review?(amarchesini)
Documentation for Cloud Storage Support Framework and FileSystemProvider API usage.
https://wiki.mozilla.org/Firefox_OS/Cloud_Storage
Duplicate of this bug: 1235960
Attachment #8732804 - Flags: review?(amarchesini)
Attachment #8732805 - Flags: review?(amarchesini)
Attachment #8732806 - Flags: review?(amarchesini)
Attachment #8732807 - Flags: review?(amarchesini)
Attachment #8738861 - Flags: review?(amarchesini)
After discussing with baku, we decide cancel the review request.

Thanks for baku's effort.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.