Closed Bug 854323 Opened 11 years ago Closed 11 years ago

Move IDBFactory to Paris bindings

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: janv, Assigned: janv)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

      No description provided.
Assignee: nobody → Jan.Varga
Blocks: 785884
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #728875 - Flags: review?(khuey)
https://tbpl.mozilla.org/?tree=Try&rev=65d44ba15df4
https://tbpl.mozilla.org/?tree=Try&rev=f0ff4efc7889

The M3 failure on "B2G Arm (VM) opt" is unrelated to this patch
No longer blocks: ParisBindings
Attached patch patch (obsolete) — Splinter Review
better patch
Attachment #728875 - Attachment is obsolete: true
Attachment #728875 - Flags: review?(khuey)
Attachment #731135 - Flags: review?(khuey)
Comment on attachment 731135 [details] [diff] [review]
patch

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

This is a good start.  But I think you should go ahead and remove nsIIDBFactory now and then you won't need to add DoOpen (in addition to Open, OpenCommon, OpenInternal, and who knows what else).

::: dom/bindings/Bindings.conf
@@ +491,5 @@
>  
> +'IDBFactory': {
> +    'nativeType': 'mozilla::dom::indexedDB::IDBFactory',
> +    'implicitJSContext': [ 'open', 'deleteDatabase', 'openForPrincipal',
> +                           'deleteForPrincipal' ],

Long term we should move getting the caller's location into the binding layer.

::: dom/bindings/Errors.msg
@@ +35,5 @@
>  MSG_DEF(MSG_GLOBAL_NOT_NATIVE, 0, "global is not a native object")
>  MSG_DEF(MSG_ENCODING_NOT_SUPPORTED, 1, "The given encoding '{0}' is not supported.")
>  MSG_DEF(MSG_DOM_ENCODING_NOT_UTF, 0, "The encoding must be utf-8, utf-16, or utf-16be.")
>  MSG_DEF(MSG_NOT_FINITE, 0, "Floating-point value is not finite.")
> +MSG_DEF(MSG_INVALID_VERSION, 0, "The value of version is 0 (zero).")

How about "0 (Zero) is not a valid database version."

::: dom/indexedDB/IDBFactory.cpp
@@ +687,5 @@
> +                             ErrorResult& aRv)
> +{
> +  // Just to be on the extra-safe side
> +  if (!nsContentUtils::IsCallerChrome()) {
> +    MOZ_NOT_REACHED("Shouldn't be possible to get here");

MOZ_NOT_REACHED doesn't do what you think it does in an opt build.  It tells the compiler "we'll never get here, so you don't need to compile the code for this branch".  And then we won't throw an exception ...  We should probably just MOZ_CRASH here if you insist on having the security check.
Attachment #731135 - Flags: review?(khuey) → review-
Attached patch updated patchSplinter Review
Attachment #731135 - Attachment is obsolete: true
Attachment #731456 - Flags: review?(khuey)
Comment on attachment 731456 [details] [diff] [review]
updated patch

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

Do the addon-sdk changes need to go to upstream somewhere?

::: dom/base/nsGlobalWindow.h
@@ +1146,5 @@
>    nsDataHashtable<nsPtrHashKey<nsXBLPrototypeHandler>, JSObject*> mCachedXBLPrototypeHandlers;
>  
>    nsCOMPtr<nsIDocument> mSuspendedDoc;
>  
> +  nsCOMPtr<nsISupports> mIndexedDB;

nsRefPtr<mozilla::dom::indexedDB::IDBFactory>

::: dom/indexedDB/IDBFactory.h
@@ +55,5 @@
>  
>    // Called when using IndexedDB from a window in the current process.
>    static nsresult Create(nsPIDOMWindow* aWindow,
>                           ContentParent* aContentParent,
> +                         nsISupports** aFactory)

This should be an IDBFactory**.
Attachment #731456 - Flags: review?(khuey)
Attachment #731456 - Flags: review?(dtownsend+bugmail)
Attachment #731456 - Flags: review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> Comment on attachment 731456 [details] [diff] [review]
> updated patch
> 
> Review of attachment 731456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do the addon-sdk changes need to go to upstream somewhere?

cc'ing Gabor

Gabor, can you take a look at addon-sdk changes ?

> 
> ::: dom/base/nsGlobalWindow.h
> @@ +1146,5 @@
> >    nsDataHashtable<nsPtrHashKey<nsXBLPrototypeHandler>, JSObject*> mCachedXBLPrototypeHandlers;
> >  
> >    nsCOMPtr<nsIDocument> mSuspendedDoc;
> >  
> > +  nsCOMPtr<nsISupports> mIndexedDB;
> 
> nsRefPtr<mozilla::dom::indexedDB::IDBFactory>

ok

> 
> ::: dom/indexedDB/IDBFactory.h
> @@ +55,5 @@
> >  
> >    // Called when using IndexedDB from a window in the current process.
> >    static nsresult Create(nsPIDOMWindow* aWindow,
> >                           ContentParent* aContentParent,
> > +                         nsISupports** aFactory)
> 
> This should be an IDBFactory**.

ok
oh, sorry, I overlooked the additional review request

it already landed on m-c
https://hg.mozilla.org/mozilla-central/rev/0b7c27024048

let me know if there are any problems with addon-sdk, caused by this bug
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(In reply to Jan Varga [:janv] from comment #8)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> Gabor, can you take a look at addon-sdk changes ?

So nsIIDBFactory is not reachable from Ci? I don't think that should cause any issue for us. On the other hand I wish I could tell from the push if it has broken the add-on SDK but at the moment of the push all the tests were broken because of some other push :( (https://tbpl.mozilla.org/?tree=Try&rev=0c8c104ad792&showall=1) Soon our tests will be unhidden on try and this will not be a problem. Anyway I will check this out soon if it has broke our tests or not.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> Comment on attachment 731456 [details] [diff] [review]
> Do the addon-sdk changes need to go to upstream somewhere?

Very good question! I let Dave to answer this one. Normally the process would be to file a pull request on github, but if I'm not mistaken we do a regular merge from the trunk back to our tree every now and then, so I hope the answer is that you don't have to do anything else. Can you correct or confirm me, Dave?. In the meanwhile I think it's time for me to cook-up a survival guide for platform developers about the add-on SDK.
No we have to manually push changes to our git repository or we will overwrite them in the next merge. Wes filed bug 856403 to track doing that already.
Blocks: 856403
Attachment #731456 - Flags: review?(dtownsend+bugmail) → review+
Comment on attachment 731456 [details] [diff] [review]
updated patch

>+    if (version < 1) {
>+      aRv.ThrowTypeError(MSG_INVALID_VERSION);
>+      return nullptr;
>+    }

This exception is not a TypeError but a DOMException per the spec.
https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBFactory-open-IDBOpenDBRequest-DOMString-name-unsigned-long-long-version
You should define an error message in dom/base/domerr.msg instead of dom/bindings/Errors.msg.
Depends on: 861714
https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBFactory-open-IDBOpenDBRequest-DOMString-name-unsigned-long-long-version

"If the value of version is 0 (zero) or a negative number, this method MUST throw a JavaScript TypeError exception."

but the spec is not consistent about it, in the table below it mentions a DOMException
Blocks: idbwebidl
You need to log in before you can comment on or make changes to this bug.