[CloudStorage] Implement File System Provider API

RESOLVED WONTFIX

Status

()

P1
normal
RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: kershaw, Assigned: kershaw)

Tracking

Trunk
2.6 S7 - 2/12
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected)

Details

Attachments

(6 attachments, 33 obsolete attachments)

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
(Assignee)

Description

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

[1] https://developer.chrome.com/apps/fileSystemProvider
Keywords: dev-doc-needed
(Assignee)

Updated

3 years ago
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.
(Assignee)

Comment 2

3 years ago
(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.
(Assignee)

Comment 3

3 years ago
Created attachment 8644814 [details] [diff] [review]
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
(Assignee)

Comment 4

3 years ago
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-
(Assignee)

Comment 7

3 years ago
Created attachment 8646464 [details] [diff] [review]
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.
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+
(Assignee)

Comment 9

3 years ago
Created attachment 8647906 [details] [diff] [review]
WebIDL-v3

WebIDL with previous comment addressed.
Attachment #8646464 - Attachment is obsolete: true
Created attachment 8648604 [details] [diff] [review]
[v1] nsICloudStorageCallback.idl
(Assignee)

Comment 12

3 years ago
Created attachment 8648657 [details]
[v1] nsIFileSystemProvider.idl
Summary: Implement File System Provider API → [CloudStorage] Implement File System Provider API
Blocks: 1164750
Created attachment 8649647 [details] [diff] [review]
[v2] nsICloudStorageCallback.idl
Attachment #8648657 - Attachment is obsolete: true
Created attachment 8649649 [details] [diff] [review]
[v1] nsICloudStorageCallback classes

WIP
Attachment #8648604 - Attachment is obsolete: true
Created attachment 8649653 [details] [diff] [review]
[v1] nsICloudStorageRequest.idl
Created attachment 8649749 [details] [diff] [review]
[v2]nsICloudStorageRequest.idl
Attachment #8649653 - Attachment is obsolete: true
Created attachment 8652155 [details] [diff] [review]
[v3]nsICloudStorageRequest.idl and nsICloudStorageRequest classes
Attachment #8649749 - Attachment is obsolete: true
Target Milestone: --- → FxOS-S8 (02Oct)
(Assignee)

Comment 18

3 years ago
Created attachment 8653920 [details] [diff] [review]
Part1:FileSystemProvider
Attachment #8647907 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Created attachment 8653921 [details] [diff] [review]
Part2:nsICloudStorageCallback
Attachment #8649647 - Attachment is obsolete: true
Attachment #8649649 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
Created attachment 8653922 [details] [diff] [review]
Part3:nsICloudStorageRequest
Attachment #8652155 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Just merge patches. Let's move on based on this.
(Assignee)

Comment 22

3 years ago
Created attachment 8655890 [details] [diff] [review]
WebIDL-v4

1. Update some data types.
2. Use unsigned long to replace long.
Attachment #8647906 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Created attachment 8655892 [details] [diff] [review]
Part1:FileSystemProvider

Integrate with nsICloudStorageRequest and nsICloudStorageCallback.
Attachment #8653920 - Attachment is obsolete: true
(Assignee)

Comment 24

3 years ago
Created attachment 8655894 [details] [diff] [review]
Part2:nsICloudStorageCallback

Update some data types.
Attachment #8653921 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
Created attachment 8655895 [details] [diff] [review]
Part3:nsICloudStorageRequest

Update some data types.
Attachment #8653922 - Attachment is obsolete: true
Created attachment 8656483 [details] [diff] [review]
Part3:nsICloudStorageRequest
Attachment #8655895 - Attachment is obsolete: true
Created attachment 8656649 [details] [diff] [review]
Part3:nsICloudStorageRequest

Update nsCloudStorageRequest implementation and nsCloudStorageRequestManager implementation
Created attachment 8656652 [details] [diff] [review]
Part3:nsICloudStorageRequest

Update nsCloudStorageRequest and nsCloudStorageRequestManager implementation. Remove duplicate attachments.
Attachment #8656483 - Attachment is obsolete: true
Attachment #8656649 - Attachment is obsolete: true
Created attachment 8659008 [details] [diff] [review]
Part1:nsICloudStorageRequest and nsICloudStorageCallback

Integrate nsICloudStorageRequest and nsICloudStorageCallback implementation into one part
Attachment #8655894 - Attachment is obsolete: true
Attachment #8656652 - Attachment is obsolete: true
Created attachment 8659009 [details] [diff] [review]
Part2:FileSystemProvider WebIDL and implementation

Integrate FileSystemProvider WebIDL and implementation into one patch.
Created attachment 8659010 [details] [diff] [review]
Part2:FileSystemProvider WebIDL and implementation

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
Created attachment 8659119 [details] [diff] [review]
Part1:nsICloudStorageRequest and nsICloudStorageCallback

update nsICloudStorageCallback function implementation
Attachment #8659008 - Attachment is obsolete: true
(Assignee)

Comment 33

3 years ago
Created attachment 8671260 [details] [diff] [review]
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
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-
(Assignee)

Comment 35

3 years ago
(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)
(Assignee)

Comment 41

3 years ago
Created attachment 8692527 [details] [diff] [review]
Part1: WebIDL Implementations
Attachment #8671260 - Attachment is obsolete: true
Attachment #8692527 - Flags: feedback?(amarchesini)
(Assignee)

Comment 42

3 years ago
Created attachment 8692528 [details] [diff] [review]
Part2: Implementations of XPCOM components
Attachment #8692528 - Flags: feedback?(amarchesini)
(Assignee)

Comment 43

3 years ago
Created attachment 8692529 [details] [diff] [review]
Part3: Test cases
Attachment #8692529 - Flags: feedback?(amarchesini)
(Assignee)

Comment 44

3 years ago
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.
(Assignee)

Comment 46

3 years ago
> 
> 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
(Assignee)

Comment 48

3 years ago
Created attachment 8725051 [details] [diff] [review]
Part1: WebIDL Implementations
Attachment #8692527 - Attachment is obsolete: true
Attachment #8692528 - Attachment is obsolete: true
Attachment #8692529 - Attachment is obsolete: true
Attachment #8725051 - Flags: review?(amarchesini)
(Assignee)

Comment 49

3 years ago
Created attachment 8725052 [details] [diff] [review]
Part2: VirtualFileSystem Service and RequestManager
Attachment #8725052 - Flags: review?(amarchesini)
(Assignee)

Comment 50

3 years ago
Created attachment 8725053 [details] [diff] [review]
Part3: IPC
Attachment #8725053 - Flags: review?(amarchesini)
(Assignee)

Comment 51

3 years ago
Created attachment 8725054 [details] [diff] [review]
Part4: Tests
Attachment #8725054 - Flags: review?(amarchesini)
(Assignee)

Comment 52

3 years ago
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-
(Assignee)

Comment 54

3 years ago
(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.
(Assignee)

Comment 55

3 years ago
Created attachment 8732804 [details] [diff] [review]
Part1: WebIDL Implementations - v2

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)
(Assignee)

Comment 56

3 years ago
Created attachment 8732805 [details] [diff] [review]
Part2: VirtualFileSystem Service and RequestManager - v2

Rebase and fix some nits.
Attachment #8732805 - Flags: review?(amarchesini)
(Assignee)

Comment 57

3 years ago
Created attachment 8732806 [details] [diff] [review]
Part3: IPC - v2

Rebase and fix nits.
Attachment #8732806 - Flags: review?(amarchesini)
(Assignee)

Comment 58

3 years ago
Created attachment 8732807 [details] [diff] [review]
Part4: Tests - v2

Rebase
Attachment #8732807 - Flags: review?(amarchesini)
(Assignee)

Comment 59

3 years ago
Created attachment 8732808 [details] [diff] [review]
Interdiff for v1 and v2 of part1.
Created attachment 8738861 [details] [diff] [review]
Part5: Write Part Support - v1

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
(Assignee)

Updated

3 years ago
Attachment #8732804 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
Attachment #8732805 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
Attachment #8732806 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
Attachment #8732807 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
Attachment #8738861 - Flags: review?(amarchesini)
(Assignee)

Comment 63

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

Thanks for baku's effort.
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.