Closed Bug 1029209 Opened 6 years ago Closed 6 years ago

Extract IndexedDB FileHandle from core FileHandle implementation

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: janv, Assigned: janv)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

This is the final part that will allow creation of WebIDL interfaces for FileSystem FileHandle.
Blocks: 942542
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
Attachment #8444943 - Flags: review?(bent.mozilla)
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.
Attachment #8444943 - Flags: review?(bent.mozilla) → review-
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)
Flags: needinfo?(jonas)
Duplicate of this bug: 1026735
Yes, changing interface names should be totally fine.
Flags: needinfo?(jonas)
Ok, I addressed other review comments in the meantime, will submit a new patch once I update it to the tip.
Attached patch patchSplinter Review
Attachment #8444943 - Attachment is obsolete: true
Attachment #8444944 - Attachment is obsolete: true
Attachment #8454532 - Flags: review?(bent.mozilla)
Attached patch interdiffSplinter Review
Attached patch hazard fix (obsolete) — Splinter Review
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
Attachment #8454630 - Attachment is patch: true
Attachment #8454630 - Attachment mime type: text/x-patch → text/plain
this fixes the hazard for real
https://tbpl.mozilla.org/?tree=Try&rev=2cb549b45df3
Attached patch hazard fixSplinter Review
Ok, so rooting analysis passes with this patch.
Attachment #8454630 - Attachment is obsolete: true
Attachment #8454901 - Flags: review?(bent.mozilla)
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
Attachment #8454901 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8454533 [details] [diff] [review]
interdiff

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

Wait, the *interdiff* is 135k?!
Comment on attachment 8454533 [details] [diff] [review]
interdiff

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

Oh, I see, it's mainly moving the tests.
Attachment #8454532 - Flags: review?(bent.mozilla) → review+
(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.
(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.
Attached patch patch for jamunSplinter Review
Patch for jamun branch
https://hg.mozilla.org/mozilla-central/rev/027f3828d0ab
https://hg.mozilla.org/mozilla-central/rev/9d72b50b8460
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.