Closed Bug 771288 Opened 7 years ago Closed 4 years ago

Multiprocess FileHandle support (FileHandle on PBackground)

Categories

(Core :: DOM: IndexedDB, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
blocking-kilimanjaro +
blocking-basecamp -

People

(Reporter: overholt, Assigned: janv)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 16 obsolete files)

64.57 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
62.39 KB, patch
Details | Diff | Splinter Review
482.21 KB, patch
baku
: review+
Details | Diff | Splinter Review
20.16 KB, patch
Details | Diff | Splinter Review
Splitting multiprocess file handle support into its own bug.
Does Media Storage need this?  If so, this should block bug 761930.
bent, this Just Works, right?  Since File only comprises a read-only filename?

I don't think we use File in gaia, but it's possibly a web-compat issue.
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
This needs a bunch more work than just making the IO part work. There's also scheduling of the locks which is very similar to the work we did for IndexedDB.

It's unlikely that this is a web-compat issue since FileHandle is a very new API which hasn't even hit firefox release yet, and is so far a mozilla proposal not picked up by other browsers.
blocking-basecamp: ? → -
blocking-kilimanjaro: ? → +
Oh, this is the writeable Handle?  I thought there was something called "File" related to Blob.

Thanks for the clarification.
yes, this FileHandle:
https://wiki.mozilla.org/WebAPI/FileHandleAPI
Summary: Multiprocess file handle support → Multiprocess FileHandle support
Depends on: 726593
Blocks: 859704
I'm going to take a stab at this, however if somebody who actually knows what they're doing wants it, let me know....
Assignee: nobody → dhylands
I talked to sicking about this today and he had a better idea. Let's just open a file descriptor in the parent, pass it to the child, and then do the IO in the child entirely. That's much simpler for DeviceStorage.
(In reply to ben turner [:bent] from comment #7)
> I talked to sicking about this today and he had a better idea. Let's just
> open a file descriptor in the parent, pass it to the child, and then do the
> IO in the child entirely. That's much simpler for DeviceStorage.

so, no cross process locking of files ?
You still have to do the scheduling of locks in a central location, i.e. in the parent process. But we shouldn't need to shuffle any data through the parent process.

See bug 896810 for why.
(In reply to Jonas Sicking (:sicking) from comment #9)
> You still have to do the scheduling of locks in a central location, i.e. in
> the parent process. But we shouldn't need to shuffle any data through the
> parent process.
> 
> See bug 896810 for why.

ok, we don't have handle quota in DeviceStorage at least :)
Attached file proposal.txt (obsolete) —
This attachment outlines more or less what I think needs to be done.

I'm not sure if I need a real actor or not. The LockedFile and FileHelper objects would exist in both the parent and the child, although for LockedFile, I think we can just create a real LockedFile object in the parent, since FileService only accesses data from it, and doesn't actually call anything.

Some type of FileHelper proxy is needed, but it doesn't have to fully reflect all of the various FileHelper's in the child. I think a simple FileHelperProxy derived from FileHelper would be sufficient, but I'll wait to hear what Ben ahs to say.
Attachment #784675 - Flags: feedback?(bent.mozilla)
Attached patch proposal.patch (obsolete) — Splinter Review
A patch version of the proposal so that review comments can be added easily.
Attachment #784675 - Attachment is obsolete: true
Attachment #784675 - Flags: feedback?(bent.mozilla)
Attachment #784676 - Flags: review?(bent.mozilla)
Blocks: 752724
Comment on attachment 784676 [details] [diff] [review]
proposal.patch

Jan, can you take this review of Dave's proposal off Ben's plate?
Attachment #784676 - Flags: review?(bent.mozilla) → review?(Jan.Varga)
Unassigning myself for the time being, since I'm not sure when I'll be working on this next.
Assignee: dhylands → nobody
No longer blocks: 891030
Comment on attachment 784676 [details] [diff] [review]
proposal.patch

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

This looks good to me, there's just one thing. Doing IO in the child is great and particular implementation of DeviceStorage/FileHandle should be relatively easy. However FileHandle used by IndexedDB and FileSystem API needs to check quota which is now possible only in the main process. So QuotaManager would need multiprocess support too. We could also take this opportunity and rather move QuotaManager to a background IPDL only thread which would enable better integration with LocalStorage and asm.js caching on the main thread.
Attachment #784676 - Flags: review?(Jan.Varga) → review+
Depends on: 961049
Blocks: 786215
No longer depends on: 961049
Blocks: 942542
Summary: Multiprocess FileHandle support → Multiprocess FileHandle support (FileHandle on PBackground)
Attached patch initial patch (obsolete) — Splinter Review
The attached patch is almost ready for review, I just need to go through it again and fix some small things.
It's based on the jamun branch and all tests pass for me on mac (also --e10s).

(Patch 1 in bug 942542 needs to be applied first)
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
Attached patch Initial patch (obsolete) — Splinter Review
Rebased against latest jamun
Attachment #8476386 - Attachment is obsolete: true
Attached patch Initial patch (obsolete) — Splinter Review
Attachment #8478539 - Attachment is obsolete: true
Attachment #8479328 - Flags: review?(bent.mozilla)
Attachment #8479172 - Attachment is obsolete: true
Rebased and sent to the try server:
https://tbpl.mozilla.org/?tree=Try&rev=bd124356d42b
Attachment #784676 - Attachment is obsolete: true
Attachment #8479328 - Attachment is obsolete: true
Attachment #8479328 - Flags: review?(bent.mozilla)
Attachment #8485806 - Flags: review?(bent.mozilla)
Attached patch patch v1 (obsolete) — Splinter Review
This is it. I tried to match the style of IDB on PBackground to make it easier to review. Actually FileHandle in ActorsParent.cpp is very similar to TransactionBase/NormalTransaction in indexedDB/ActorsParent.cpp
Other parts that handle actual requests have been heavily modified relatively to old filehandle implementation. FileService which is now called FileHandleThreadPool is implemented in the anonymous namespace of ActorsParent.cpp. This integration simplifies some stuff, for example FileHandle objects can be passed to it, instead of passing filehandle id, mode, etc.
I think we could integrate TransactionThreadPool into indexedDB/ActorsParent.cpp too at some point.

Here's a try link:
https://tbpl.mozilla.org/?tree=Try&rev=5e35f090b656
Attachment #8478541 - Attachment is obsolete: true
Attachment #8488744 - Flags: review?(bent.mozilla)
Blocks: 1068975
Attached patch Remove unused stuff (obsolete) — Splinter Review
rebased

rebased main patch (with support for disabling file handle api via a pref) is coming too
Attachment #8496855 - Flags: review?(bent.mozilla)
Attachment #8485806 - Attachment is obsolete: true
Attachment #8485806 - Flags: review?(bent.mozilla)
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8488744 - Attachment is obsolete: true
Attachment #8488744 - Flags: review?(bent.mozilla)
Attachment #8497083 - Flags: review?(bent.mozilla)
Attached patch Remove unused stuff (obsolete) — Splinter Review
Attachment #8496855 - Attachment is obsolete: true
Attachment #8496855 - Flags: review?(bent.mozilla)
Attachment #8504248 - Flags: review?(bent.mozilla)
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8497083 - Attachment is obsolete: true
Attachment #8497083 - Flags: review?(bent.mozilla)
Attachment #8504249 - Flags: review?(bent.mozilla)
Attachment #8504249 - Attachment is patch: true
Attached patch Remove unused stuff (obsolete) — Splinter Review
Attachment #8504248 - Attachment is obsolete: true
Attachment #8504248 - Flags: review?(bent.mozilla)
Attachment #8506299 - Flags: review?(bent.mozilla)
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8504249 - Attachment is obsolete: true
Attachment #8504249 - Flags: review?(bent.mozilla)
Attachment #8506300 - Flags: review?(bent.mozilla)
Comment on attachment 8506299 [details] [diff] [review]
Remove unused stuff

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

bent, ok, I'll try to split those 2 big patches for easier review (after we finish other more important stuff)

Anyway, just in case you are bored :) this patch just removes unused stuff and in my opinion doesn't require splitting and should be rather easy to review.
rebased
Attachment #8506299 - Attachment is obsolete: true
Attachment #8506299 - Flags: review?(bent.mozilla)
Attachment #8517410 - Flags: review?(bent.mozilla)
-w produces better patch for reviewing in this case
Attached patch patch v1 (obsolete) — Splinter Review
- rebased
- renamed test_bug771288.js to test_mutableFileUpgrade.js
- reworked the above test to use standard DOM File Reader
Attachment #8506300 - Attachment is obsolete: true
Attachment #8506300 - Flags: review?(bent.mozilla)
Attachment #8530635 - Flags: review?(bent.mozilla)
Comment on attachment 8517410 [details] [diff] [review]
Remove unused stuff

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

::: dom/indexedDB/ActorsParent.cpp
@@ -5319,5 @@
> -  {
> -    MOZ_ASSERT(NS_IsMainThread());
> -
> -    // XXX This is racy, is this correct?
> -    return !!mTransactionCount;

Can you just remove mTransactionCount, NoteNewTransaction(), and NoteFinishedTransaction() now?

::: dom/ipc/PContent.ipdl
@@ +329,5 @@
>      FileDescriptor;
>      void_t;
>  };
>  
> +union OptionalWindowId

Please rename to OptionalContentId
Attachment #8517410 - Flags: review?(bent.mozilla) → review+
Ben, I think I found out how to avoid the upgrade of file ids (or do it lazily), so we wouldn't have to upgrade the whole database at opening time.

Regarding multiprocess support, I propose to just disable FileHandle on B2G by adding:
pref("dom.indexedDB.enabled", false);
to b2g/app/b2g.js

So, FileHandle in IndexedDB will still work in Firefox Desktop Nightly with enabled e10s, but we won't expose this API on B2G.

Does it sound ok to you ?
Flags: needinfo?(bent.mozilla)
Comment on attachment 8530635 [details] [diff] [review]
patch v1

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

I sent comments over email, basically there are some architectural changes I'd like to see with this patch.
Attachment #8530635 - Flags: review?(bent.mozilla) → review-
Flags: needinfo?(bent.mozilla)
No longer blocks: 942542
Attached patch patchSplinter Review
Attachment #8530635 - Attachment is obsolete: true
Attachment #8645604 - Flags: review?(amarchesini)
Comment on attachment 8645604 [details] [diff] [review]
patch

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

looks very good to me. Just small NITs here and there.

::: dom/filehandle/FileHandleStorage.h
@@ +12,5 @@
> +
> +enum FileHandleStorage
> +{
> +  FILE_HANDLE_STORAGE_IDB = 0,
> +  //FILE_HANDLE_STORAGE_FS

This is a comment, because it will be a follow up, correct? in case you have a bug ID, would be nice to add it.

::: dom/indexedDB/ActorsParent.cpp
@@ +3600,5 @@
> +#if defined(MOZ_B2G)
> +
> +  // We don't have to do the upgrade of file ids on B2G. The old format was
> +  // only used by the previous single process implementation and B2G was
> +  // always multi process. This is a nice optimization since the upgrade needs

is this 80chars?

::: dom/indexedDB/FileInfo.cpp
@@ +132,5 @@
>  FileInfo::UpdateReferences(ThreadSafeAutoRefCnt& aRefCount,
>                             int32_t aDelta)
>  {
>    // XXX This can go away once DOM objects no longer hold FileInfo objects...
> +  //     Looking at you, BlobImplBase...

heh

::: dom/indexedDB/FileSnapshot.cpp
@@ +64,5 @@
> +    MOZ_ASSERT(IsOnOwningThread());
> +  }
> +
> +  void
> +  Finish() {

{ in a new line

@@ +224,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +StreamWrapper::Available(uint64_t* _retval)

I don't know if we have a guideline for this. Sometimes I see _retval, sometimes aRetval.

::: dom/indexedDB/IDBFileHandle.cpp
@@ +83,5 @@
> +    return nullptr;
> +  }
> +
> +  // Argument checking for get metadata.
> +  if (!aParameters.mSize && !aParameters.mLastModified) {

mSize can be 0.

::: dom/indexedDB/IDBMutableFile.cpp
@@ +71,3 @@
>  
> +  int64_t fileId;
> +  if (mBackgroundActor && mBackgroundActor->SendGetFileId(&fileId)) {

if (!mBackgroundActor ||
    NS_WARN_IF(!mBackgroundActor->SendGetFileId(&fileId))) {
  return -1;
}

return fileId;

@@ +178,5 @@
> +    return nullptr;
> +  }
> +
> +  // Do nothing if the window is closed
> +  if (!GetOwner()) {

can you do this without returning an error code?
Have you tested it?
I would return a generic error.

@@ +204,5 @@
> +
> +already_AddRefed<DOMRequest>
> +IDBMutableFile::GetFile(ErrorResult& aError)
> +{
> +  nsRefPtr<IDBFileHandle> fileHandle = Open(FileMode::Readonly, aError);

Yes, definitely it's better if you return an error in Open()

@@ +211,5 @@
> +  }
> +
> +  FileRequestGetFileParams params;
> +
> +  nsRefPtr<IDBFileRequest> request = 

extra space.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +232,5 @@
>      return JS_WriteBytes(aWriter, &value, sizeof(value));
>    }
>  
>    IDBMutableFile* mutableFile;
>    if (NS_SUCCEEDED(UNWRAP_OBJECT(IDBMutableFile, aObj, mutableFile))) {

here you can do:

if (!cloneWriteInfo->mDatabase->IsFileHandleDisabled() &&
    NS_SUCCEEDED(UNWRAP_OBJECT(IDBMutableFile, aObj, mutableFile))) {

...

@@ +274,5 @@
>        MOZ_ASSERT(false, "Fix the structured clone data to use a bigger type!");
>        return false;
>      }
>  
> +    const uint32_t index = uint32_t(cloneWriteInfo->mBlobOrMutableFiles.Length());

why this casting? Length() should return uint32_t or size_t.

@@ +323,5 @@
>          return false;
>        }
>  
>        const uint32_t index =
> +        uint32_t(cloneWriteInfo->mBlobOrMutableFiles.Length());

remove this casting.

@@ +352,2 @@
>  
>          if (!JS_WriteBytes(aWriter, &lastModifiedDate, sizeof(lastModifiedDate)) || 

extra space here?

@@ +434,5 @@
>  bool
> +ResolveMysteryMutableFile(IDBMutableFile* aMutableFile,
> +                          const nsString& aName,
> +                          const nsString& aType)
> +{

MOZ_ASSERT(aMutableFile);

@@ +586,4 @@
>                             StructuredCloneFile& aFile,
>                             const MutableFileData& aData,
>                             JS::MutableHandle<JSObject*> aResult)
>    {

MOZ_ASSERT(aCx);

@@ +591,2 @@
>  
> +    if (!NS_IsMainThread()) {

if (!aFile.mMutable || !NS_IsMainThread()) {
  return false;
}

@@ +597,2 @@
>  
> +    if (aFile.mMutableFile) {

remove this.

@@ +603,5 @@
> +      }
> +
> +      result = aFile.mMutableFile->WrapObject(aCx, nullptr);
> +    } else {
> +      result = JS_NewPlainObject(aCx);

I would remove this } else { completely.

@@ +616,5 @@
>                            IDBDatabase* aDatabase,
>                            StructuredCloneFile& aFile,
>                            const BlobOrFileData& aData,
>                            JS::MutableHandle<JSObject*> aResult)
>    {

MOZ_ASSERT(aCx);

@@ +776,5 @@
>      return true;
>    }
>  };
>  
> +#if !defined(MOZ_B2G)

what about B2G ?

@@ +809,5 @@
> +                          IDBDatabase* aDatabase,
> +                          StructuredCloneFile& aFile,
> +                          const BlobOrFileData& aData,
> +                          JS::MutableHandle<JSObject*> aResult)
> +  {

MOZ_ASSERT(aCx);

@@ +1157,5 @@
> +
> +  static JSStructuredCloneCallbacks callbacks = {
> +    CommonStructuredCloneReadCallback<UpgradeDeserializationHelper>,
> +    nullptr,
> +    nullptr

this wants other 3 callbacks for the tranferring objects. Probably you want them to be nullptr.

@@ +1160,5 @@
> +    nullptr,
> +    nullptr
> +  };
> +
> +  if (!JS_ReadStructuredClone(aCx, data, dataLen, JS_STRUCTURED_CLONE_VERSION,

Consider to use StructuredCloneHelper for this.

::: dom/indexedDB/test/unit/xpcshell-shared.ini
@@ +21,5 @@
>  [test_deleteDatabase.js]
>  [test_deleteDatabase_interactions.js]
>  [test_event_source.js]
> +[test_filehandle_append_read_data.js]
> +skip-if = true

are you planning to enable it in a follow up?
Otherwise remove it.

::: dom/webidl/IDBFileHandle.webidl
@@ +25,5 @@
>    IDBFileRequest? readAsText(unsigned long long size,
>                               optional DOMString? encoding = null);
>  
>    [Throws]
> +  IDBFileRequest? write(DOMString value);

is there a reason why this cannot be:
IDBFileRequest? write((DOMString or ArrayBuffer or ArrayBufferView or Blob) value)

?

same for append.
Attachment #8645604 - Flags: review?(amarchesini) → review+
baku, thanks for the comments!
(In reply to Andrea Marchesini (PTO until 8/9) from comment #39)
> Comment on attachment 8645604 [details] [diff] [review]
> patch
> 
> Review of attachment 8645604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks very good to me. Just small NITs here and there.

Thanks!

> 
> ::: dom/filehandle/FileHandleStorage.h
> @@ +12,5 @@
> > +
> > +enum FileHandleStorage
> > +{
> > +  FILE_HANDLE_STORAGE_IDB = 0,
> > +  //FILE_HANDLE_STORAGE_FS
> 
> This is a comment, because it will be a follow up, correct? in case you have
> a bug ID, would be nice to add it.

Added.

> 
> ::: dom/indexedDB/ActorsParent.cpp
> @@ +3600,5 @@
> > +#if defined(MOZ_B2G)
> > +
> > +  // We don't have to do the upgrade of file ids on B2G. The old format was
> > +  // only used by the previous single process implementation and B2G was
> > +  // always multi process. This is a nice optimization since the upgrade needs
> 
> is this 80chars?

Yes, it is.

> 
> ::: dom/indexedDB/FileInfo.cpp
> @@ +132,5 @@
> >  FileInfo::UpdateReferences(ThreadSafeAutoRefCnt& aRefCount,
> >                             int32_t aDelta)
> >  {
> >    // XXX This can go away once DOM objects no longer hold FileInfo objects...
> > +  //     Looking at you, BlobImplBase...
> 
> heh

Yeah, added a reference to the bug 1068975.

> 
> ::: dom/indexedDB/FileSnapshot.cpp
> @@ +64,5 @@
> > +    MOZ_ASSERT(IsOnOwningThread());
> > +  }
> > +
> > +  void
> > +  Finish() {
> 
> { in a new line

Ok.

> 
> @@ +224,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +NS_IMETHODIMP
> > +StreamWrapper::Available(uint64_t* _retval)
> 
> I don't know if we have a guideline for this. Sometimes I see _retval,
> sometimes aRetval.

I think all these NS_IMETHODIMP methods usually use _retval.

> 
> ::: dom/indexedDB/IDBFileHandle.cpp
> @@ +83,5 @@
> > +    return nullptr;
> > +  }
> > +
> > +  // Argument checking for get metadata.
> > +  if (!aParameters.mSize && !aParameters.mLastModified) {
> 
> mSize can be 0.

This is actually a boolean. So mSize here means "I want to get the size of the file".
And if the parameters struct has all booleans set to false, we want to throw.

> 
> ::: dom/indexedDB/IDBMutableFile.cpp
> @@ +71,3 @@
> >  
> > +  int64_t fileId;
> > +  if (mBackgroundActor && mBackgroundActor->SendGetFileId(&fileId)) {
> 
> if (!mBackgroundActor ||
>     NS_WARN_IF(!mBackgroundActor->SendGetFileId(&fileId))) {
>   return -1;
> }
> 
> return fileId;

Ok.

> 
> @@ +178,5 @@
> > +    return nullptr;
> > +  }
> > +
> > +  // Do nothing if the window is closed
> > +  if (!GetOwner()) {
> 
> can you do this without returning an error code?
> Have you tested it?
> I would return a generic error.

Ok.

> 
> @@ +204,5 @@
> > +
> > +already_AddRefed<DOMRequest>
> > +IDBMutableFile::GetFile(ErrorResult& aError)
> > +{
> > +  nsRefPtr<IDBFileHandle> fileHandle = Open(FileMode::Readonly, aError);
> 
> Yes, definitely it's better if you return an error in Open()

Ok.

> 
> @@ +211,5 @@
> > +  }
> > +
> > +  FileRequestGetFileParams params;
> > +
> > +  nsRefPtr<IDBFileRequest> request = 
> 
> extra space.

Fixed.

> 
> ::: dom/indexedDB/IDBObjectStore.cpp
> @@ +232,5 @@
> >      return JS_WriteBytes(aWriter, &value, sizeof(value));
> >    }
> >  
> >    IDBMutableFile* mutableFile;
> >    if (NS_SUCCEEDED(UNWRAP_OBJECT(IDBMutableFile, aObj, mutableFile))) {
> 
> here you can do:
> 
> if (!cloneWriteInfo->mDatabase->IsFileHandleDisabled() &&
>     NS_SUCCEEDED(UNWRAP_OBJECT(IDBMutableFile, aObj, mutableFile))) {

Hm, then the other blocks below would get executed, no ?

> 
> ...
> 
> @@ +274,5 @@
> >        MOZ_ASSERT(false, "Fix the structured clone data to use a bigger type!");
> >        return false;
> >      }
> >  
> > +    const uint32_t index = uint32_t(cloneWriteInfo->mBlobOrMutableFiles.Length());
> 
> why this casting? Length() should return uint32_t or size_t.

Fixed.

> 
> @@ +323,5 @@
> >          return false;
> >        }
> >  
> >        const uint32_t index =
> > +        uint32_t(cloneWriteInfo->mBlobOrMutableFiles.Length());
> 
> remove this casting.

Ok.

> 
> @@ +352,2 @@
> >  
> >          if (!JS_WriteBytes(aWriter, &lastModifiedDate, sizeof(lastModifiedDate)) || 
> 
> extra space here?

Removed.

> 
> @@ +434,5 @@
> >  bool
> > +ResolveMysteryMutableFile(IDBMutableFile* aMutableFile,
> > +                          const nsString& aName,
> > +                          const nsString& aType)
> > +{
> 
> MOZ_ASSERT(aMutableFile);

Ok.

> 
> @@ +586,4 @@
> >                             StructuredCloneFile& aFile,
> >                             const MutableFileData& aData,
> >                             JS::MutableHandle<JSObject*> aResult)
> >    {
> 
> MOZ_ASSERT(aCx);

Ok.

> 
> @@ +591,2 @@
> >  
> > +    if (!NS_IsMainThread()) {
> 
> if (!aFile.mMutable || !NS_IsMainThread()) {
>   return false;
> }

Ok.

> 
> @@ +597,2 @@
> >  
> > +    if (aFile.mMutableFile) {
> 
> remove this.

Ok.

> 
> @@ +603,5 @@
> > +      }
> > +
> > +      result = aFile.mMutableFile->WrapObject(aCx, nullptr);
> > +    } else {
> > +      result = JS_NewPlainObject(aCx);
> 
> I would remove this } else { completely.

Yeah, I reworked it all.

> 
> @@ +616,5 @@
> >                            IDBDatabase* aDatabase,
> >                            StructuredCloneFile& aFile,
> >                            const BlobOrFileData& aData,
> >                            JS::MutableHandle<JSObject*> aResult)
> >    {
> 
> MOZ_ASSERT(aCx);

Ok.

> 
> @@ +776,5 @@
> >      return true;
> >    }
> >  };
> >  
> > +#if !defined(MOZ_B2G)
> 
> what about B2G ?

Added a comment for that.

> 
> @@ +809,5 @@
> > +                          IDBDatabase* aDatabase,
> > +                          StructuredCloneFile& aFile,
> > +                          const BlobOrFileData& aData,
> > +                          JS::MutableHandle<JSObject*> aResult)
> > +  {
> 
> MOZ_ASSERT(aCx);

Ok.

> 
> @@ +1157,5 @@
> > +
> > +  static JSStructuredCloneCallbacks callbacks = {
> > +    CommonStructuredCloneReadCallback<UpgradeDeserializationHelper>,
> > +    nullptr,
> > +    nullptr
> 
> this wants other 3 callbacks for the tranferring objects. Probably you want
> them to be nullptr.

Ok.

> 
> @@ +1160,5 @@
> > +    nullptr,
> > +    nullptr
> > +  };
> > +
> > +  if (!JS_ReadStructuredClone(aCx, data, dataLen, JS_STRUCTURED_CLONE_VERSION,
> 
> Consider to use StructuredCloneHelper for this.

This just copies what we already have in IDBObjectStore::DeserializeIndexValue()
and IDBObjectStore::DeserializeValue()
I added a comment though.

> 
> ::: dom/indexedDB/test/unit/xpcshell-shared.ini
> @@ +21,5 @@
> >  [test_deleteDatabase.js]
> >  [test_deleteDatabase_interactions.js]
> >  [test_event_source.js]
> > +[test_filehandle_append_read_data.js]
> > +skip-if = true
> 
> are you planning to enable it in a follow up?
> Otherwise remove it.

Removed.

> 
> ::: dom/webidl/IDBFileHandle.webidl
> @@ +25,5 @@
> >    IDBFileRequest? readAsText(unsigned long long size,
> >                               optional DOMString? encoding = null);
> >  
> >    [Throws]
> > +  IDBFileRequest? write(DOMString value);
> 
> is there a reason why this cannot be:
> IDBFileRequest? write((DOMString or ArrayBuffer or ArrayBufferView or Blob)
> value)
> 
> ?
> 
> same for append.

Ok, I reworked it.
Attached patch interdiffSplinter Review
Depends on: 1203803
Is there more to be done in this bug?  It wasn't resolved because it has leave-open on it...
Depends on: 1204162
Forgot to remove the flag, that was for a pre-patch.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.