1029209 - Extract IndexedDB FileHandle from core FileHandle implementation

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jan Varga [:janv]]

This is the final part that will allow creation of WebIDL interfaces for FileSystem FileHandle.

Comment 1

[Jan Varga [:janv]]

Attachment: patch

Comment 2

[Jan Varga [:janv]]

Attachment: diff -w of mochitest changes for easier review

Comment 3

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

Comment on attachment 8444943 [details] [diff] [review]
patch

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

This looks mostly fine but we shouldn't rename the things that are exposed to the web until we really need to.

::: dom/indexedDB/IDBFileHandle.cpp
@@ +155,5 @@
> +nsresult
> +IDBFileHandle::OnCompleteOrAbort(bool aAborted)
> +{
> +  nsCOMPtr<nsIDOMEvent> event;
> +  if (mAborted) {

Hm, you're using mAborted here, not aAborted. Either hide mAborted (make it private in base class) or stop passing it here.

::: dom/indexedDB/IDBFileRequest.h
@@ +24,5 @@
> +namespace indexedDB {
> +
> +class IDBFileHandle;
> +
> +class IDBFileRequest : public DOMRequest,

MOZ_FINAL

::: dom/indexedDB/IDBMutableFile.cpp
@@ +28,4 @@
>  
> +namespace mozilla {
> +namespace dom {
> +namespace indexedDB {

This should go after your anonymous namespace.

@@ +241,5 @@
> +  return request.forget();
> +}
> +
> +nsresult
> +GetFileHelper::GetSuccessResult(JSContext* aCx,

This needs a main thread assertion.

::: dom/indexedDB/IDBMutableFile.h
@@ +36,2 @@
>  
> +class IDBMutableFile : public DOMEventTargetHelper,

Nit: MOZ_FINAL

::: dom/indexedDB/test/moz.build
@@ +3,5 @@
>  # 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/.
>  
> +DIRS += ['extensions', 'filehandle']

I'd rather not have another test directory... Can you move these into the regular test dir?

::: dom/tests/mochitest/general/test_interfaces.html
@@ -360,5 @@
>      {name: "External", b2g: false},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "File",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> -    "FileHandle",

This isn't ok, you're breaking web compat here. Let's wait to rename this when there's actually another FileHandle that needs to be exposed to the web.

Comment 4

[Jan Varga [:janv]]

Jonas, do you think we can make an exception here and break things like:
"if ('FileHandle' in window) { ... }"

Actually, we already did that in https://hg.mozilla.org/mozilla-central/rev/00d924bed2d1
so: "if ('LockedFile' in window) { ... }" won't work anyway.

As we talked there are only few consumers of this API and FileHandle has been always marked as experimental (hence deprecated IDBDatabase.mozCreateFileHandle)

Comment 6

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

Yes, changing interface names should be totally fine.

Comment 7

[Jan Varga [:janv]]

Ok, I addressed other review comments in the meantime, will submit a new patch once I update it to the tip.

Comment 8

[Jan Varga [:janv]]

Attachment: patch

Comment 9

[Jan Varga [:janv]]

Attachment: interdiff

Comment 10

[Jan Varga [:janv]]

Attachment: hazard fix

For some reason, this is needed too.

Here's try with all tests (before the hazard fix):
https://tbpl.mozilla.org/?tree=Try&rev=80c3721211b8

Here's try with just the hazard test (hazard fix included):
https://tbpl.mozilla.org/?tree=Try&rev=b498f827f401

Comment 11

[Jan Varga [:janv]]

this fixes the hazard for real
https://tbpl.mozilla.org/?tree=Try&rev=2cb549b45df3

Comment 12

[Jan Varga [:janv]]

Attachment: hazard fix

Ok, so rooting analysis passes with this patch.

Comment 13

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

Comment on attachment 8454901 [details] [diff] [review]
hazard fix

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +796,5 @@
> +    JS::Rooted<JSObject*> result(aCx, mutableFile->WrapObject(aCx));
> +    if (NS_WARN_IF(!result)) {
> +      return false;
> +    }
> + 

Nit: extra whitespace

Comment 14

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

Comment on attachment 8454533 [details] [diff] [review]
interdiff

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

Wait, the *interdiff* is 135k?!

Comment 15

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

Comment on attachment 8454533 [details] [diff] [review]
interdiff

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

Oh, I see, it's mainly moving the tests.

Comment 16

[Jan Varga [:janv]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #14)
> Comment on attachment 8454533 [details] [diff] [review]
> interdiff
> 
> Review of attachment 8454533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wait, the *interdiff* is 135k?!

Oh, sorry, it was a pain to produce the interdiff and I gave up on making it perfect.
Content of tests hasn't changed, only the location, but it obviously you figured that out.

Comment 17

[Steve Fink [:sfink] [:s:]]

(In reply to Jan Varga [:janv] from comment #12)
> Created attachment 8454901 [details] [diff] [review]
> hazard fix
> 
> Ok, so rooting analysis passes with this patch.

For the record, an alternative way to fix this hazard that wouldn't require so much restructuring:

JSObject *
CreateAndWrapMutableFile(...)
{
    JS::Rooted<JSObject*> result(aCx);

    // Scope to allow ~nsRefPtr to GC all it wants without messing up the return value
    {
        nsRefPtr<IWhatZit> foo = ...;
        result = foo->WrapObject(aCx);
    }

    return result;
}

In general, functions that are creating JSObjects can return them directly. The only reason why the analysis complained about this one is the subtle issue with GCing destructors that can invalidate a returning pointer by moving the data somewhere else.

Comment 18

[Jan Varga [:janv]]

Ok, thanks for the info

Patches just landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/027f3828d0ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d72b50b8460

Comment 19

[Jan Varga [:janv]]

Attachment: patch for jamun

Patch for jamun branch

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/027f3828d0ab
https://hg.mozilla.org/mozilla-central/rev/9d72b50b8460