Closed
Bug 854323
Opened 11 years ago
Closed 11 years ago
Move IDBFactory to Paris bindings
Categories
(Core :: Storage: IndexedDB, defect)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: janv, Assigned: janv)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
38.97 KB,
patch
|
khuey
:
review+
mossop
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #728875 -
Flags: review?(khuey)
Updated•11 years ago
|
Blocks: ParisBindings
Assignee | ||
Comment 2•11 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: ParisBindings
Updated•11 years ago
|
Blocks: ParisBindings
Assignee | ||
Comment 3•11 years ago
|
||
better patch
Attachment #728875 -
Attachment is obsolete: true
Attachment #728875 -
Flags: review?(khuey)
Attachment #731135 -
Flags: review?(khuey)
Assignee | ||
Comment 4•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0c8c104ad792
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•11 years ago
|
||
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•11 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•11 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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 10•11 years ago
|
||
(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.
Comment 11•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #731456 -
Flags: review?(dtownsend+bugmail) → review+
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 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
Comment 14•11 years ago
|
||
Commit pushed to firefox22 at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/26281574a22036d54dfb582085aa43740668ef1c Bug 854323 - Move IDBFactory to Paris bindings. r=khuey
You need to log in
before you can comment on or make changes to this bug.
Description
•