771288 - Multiprocess FileHandle support (FileHandle on PBackground)

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Andrew Overholt [:overholt]]

Splitting multiprocess file handle support into its own bug.

Comment 1

[Andrew Overholt [:overholt]]

Does Media Storage need this?  If so, this should block bug 761930.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 3

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

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.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh, this is the writeable Handle?  I thought there was something called "File" related to Blob.

Thanks for the clarification.

Comment 5

[Jan Varga [:janv]]

yes, this FileHandle:
https://wiki.mozilla.org/WebAPI/FileHandleAPI

Comment 6

[Dave Hylands [:dhylands]]

I'm going to take a stab at this, however if somebody who actually knows what they're doing wants it, let me know....

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 8

[Jan Varga [:janv]]

(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 ?

Comment 9

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

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.

Comment 10

[Jan Varga [:janv]]

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

Comment 11

[Dave Hylands [:dhylands]]

Attachment: proposal.txt

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.

Comment 12

[Dave Hylands [:dhylands]]

Attachment: proposal.patch

A patch version of the proposal so that review comments can be added easily.

Comment 13

[Andrew Overholt [:overholt]]

Comment on attachment 784676 [details] [diff] [review]
proposal.patch

Jan, can you take this review of Dave's proposal off Ben's plate?

Comment 14

[Dave Hylands [:dhylands]]

Unassigning myself for the time being, since I'm not sure when I'll be working on this next.

Comment 15

[Jan Varga [:janv]]

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.

Comment 16

[Jan Varga [:janv]]

Attachment: initial patch

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)

Comment 17

[Jan Varga [:janv]]

Attachment: Initial patch

Rebased against latest jamun

Comment 18

[Jan Varga [:janv]]

Attachment: Initial patch

Comment 19

[Jan Varga [:janv]]

Attachment: Remove unused stuff (needs to be applied before Initial patch)

Comment 20

[Jan Varga [:janv]]

Attachment: Remove unused stuff (needs to be applied before Initial patch)

Comment 21

[Jan Varga [:janv]]

Attachment: Remove unused stuff (needs to be applied before Initial patch)

Rebased and sent to the try server:
https://tbpl.mozilla.org/?tree=Try&rev=bd124356d42b

Comment 22

[Jan Varga [:janv]]

Attachment: patch v1

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

Comment 23

[Jan Varga [:janv]]

Attachment: Remove unused stuff

rebased

rebased main patch (with support for disabling file handle api via a pref) is coming too

Comment 24

[Jan Varga [:janv]]

Attachment: patch v1

Comment 25

[Jan Varga [:janv]]

Attachment: Remove unused stuff

Comment 26

[Jan Varga [:janv]]

Attachment: patch v1

Comment 27

[Jan Varga [:janv]]

Attachment: Remove unused stuff

Comment 28

[Jan Varga [:janv]]

Attachment: patch v1

Comment 29

[Jan Varga [:janv]]

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.

Comment 30

[Jan Varga [:janv]]

Attachment: Remove unused stuff

rebased

Comment 31

[Jan Varga [:janv]]

Attachment: Remove unused stuff (-w version)

-w produces better patch for reviewing in this case

Comment 32

[Jan Varga [:janv]]

Attachment: patch v1

- rebased
- renamed test_bug771288.js to test_mutableFileUpgrade.js
- reworked the above test to use standard DOM File Reader

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 34

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5e60f0a258

Comment 35

[Jan Varga [:janv]]

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 ?

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba5e60f0a258

Comment 37

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 38

[Jan Varga [:janv]]

Attachment: patch

Comment 39

[Andrea Marchesini [:baku]]

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.

Comment 40

[Jan Varga [:janv]]

baku, thanks for the comments!

Comment 41

[Jan Varga [:janv]]

(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.

Comment 42

[Jan Varga [:janv]]

Attachment: interdiff

Comment 43

[Jan Varga [:janv]]

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da6757938237

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac51f970e85d

Comment 45

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

https://hg.mozilla.org/mozilla-central/rev/ac51f970e85d

Comment 46

[(no longer active)]

Is there more to be done in this bug?  It wasn't resolved because it has leave-open on it...

Comment 47

[Jan Varga [:janv]]

Forgot to remove the flag, that was for a pre-patch.

Comment 48

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.