Move IDBFactory to Paris bindings

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: janv, Assigned: janv)

Tracking

(Blocks: 1 bug)

unspecified
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Assignee: nobody → Jan.Varga
Blocks: 785884
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 728875 [details] [diff] [review]
patch
Attachment #728875 - Flags: review?(khuey)

Updated

5 years ago
Blocks: 580070
(Assignee)

Comment 2

5 years ago
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: 580070

Updated

5 years ago
Blocks: 580070
(Assignee)

Comment 3

5 years ago
Created attachment 731135 [details] [diff] [review]
patch

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

Comment 6

5 years ago
Created attachment 731456 [details] [diff] [review]
updated patch
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+
(Assignee)

Comment 8

5 years ago
(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
(Assignee)

Comment 9

5 years ago
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
Last Resolved: 5 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
(Assignee)

Comment 13

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 763231
You need to log in before you can comment on or make changes to this bug.