Closed Bug 921478 Opened 6 years ago Closed 6 years ago

Remove BackstagePass IDB constructor resolve hook and use Cu.importGlobalProperties

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bholley, Assigned: vendo)

References

Details

Attachments

(3 files, 3 obsolete files)

Jan promised to rip out the resolve hook machinery from bug 920179 and use the Cu.importGlobalProperties machinery from bug 920553. He's going to land IDBKeyRange using the existing resolve hook for now (since it's a Q3 goal), and then fix this all up sometime next week.
Jan, what's the status here?
Flags: needinfo?(Jan.Varga)
Bobby, sorry I know that I promised to fix this ASAP, but I've been extremely busy with wrapping up sync IDB for review.
Anyway, Vendo is willing to finish my 90% complete patch for this bug. The only thing that needs to be done is to rewrite test_contact_upgrade.html to XUL. I explained the problem to him in person.
Assignee: Jan.Varga → vendo.ruzicka
Flags: needinfo?(Jan.Varga)
Attached patch patchSplinter Review
Attached patch patch part 2 (obsolete) — Splinter Review
Conversion of test_contacts_upgrade.html to chrome mochitest.
Try https://tbpl.mozilla.org/?tree=Try&rev=532ff0f32eeb
Attached patch diff (part 2)Splinter Review
Diff including Patch part 2 with ignore white space option (-w) for better visibility of changes.
Vendo, dom/datastore has been added in the meantime, I think we need to add |Cu.importGlobalProperties(["indexedDB"])| after |Cu.import("resource://gre/modules/XPCOMUtils.jsm");| in dom/datastore/DataStoreDB.jsm
Attachment #823550 - Attachment is patch: true
Attachment #823550 - Attachment mime type: text/x-patch → text/plain
Attached patch patch part 2 (obsolete) — Splinter Review
done, tests running at https://tbpl.mozilla.org/?tree=Try&rev=9243a9dbf43a
Attachment #823546 - Attachment is obsolete: true
Comment on attachment 823300 [details] [diff] [review]
patch

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

::: dom/indexedDB/test/unit/xpcshell.ini
@@ +31,3 @@
>  [test_globalObjects_ipc.js]
>  # FIXME/bug 575918: out-of-process xpcshell is broken on OS X
> +#skip-if = os == "mac" || os == "android"

note, this needs to be uncommented
Attached patch patch part 2Splinter Review
updated, now it should be ok
https://tbpl.mozilla.org/?tree=Try&rev=3731a184126a
Attachment #824199 - Attachment is obsolete: true
Attachment #823300 - Flags: review?(bobbyholley+bmo)
Attachment #823300 - Flags: review?(bent.mozilla)
Attachment #824558 - Flags: review?(bent.mozilla)
Comment on attachment 824558 [details] [diff] [review]
patch part 2

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

::: dom/contacts/tests/test_contacts_upgrade.xul
@@ +8,2 @@
>  
> +<window title="Mozilla Bug 889239"

All 889239 should be changed to 921478 in this test
Attached patch patch part 2 (obsolete) — Splinter Review
bug numbers in test updated
Attachment #824558 - Attachment is obsolete: true
Attachment #824558 - Flags: review?(bent.mozilla)
Attachment #824583 - Flags: review?(bent.mozilla)
Attachment #824558 - Attachment is obsolete: false
Attachment #824558 - Flags: review?(bent.mozilla)
Comment on attachment 824583 [details] [diff] [review]
patch part 2

Sorry for the spam, I take back my comment about changing the bug number in the test (it was too early in the morning), sorry again.
Attachment #824583 - Attachment is obsolete: true
Attachment #824583 - Flags: review?(bent.mozilla)
Comment on attachment 823300 [details] [diff] [review]
patch

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

I'll let bent review the IDB testing changes. r=bholley on the rest.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +424,5 @@
> +      !IDBObjectStoreBinding::GetConstructorObject(aCx, aGlobal) ||
> +      !IDBOpenDBRequestBinding::GetConstructorObject(aCx, aGlobal) ||
> +      !IDBRequestBinding::GetConstructorObject(aCx, aGlobal) ||
> +      !IDBTransactionBinding::GetConstructorObject(aCx, aGlobal) ||
> +      !IDBVersionChangeEventBinding::GetConstructorObject(aCx, aGlobal)) {

uber nit - brace on its own line due  to the lack of visual separation from a multiline condition. The Gecko style guide is silent on this issue, but the SM style guide describes it pretty explicitly:

https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style#Other_whitespace
Attachment #823300 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 823300 [details] [diff] [review]
patch

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

Looks good! Sorry for the delay.
Attachment #823300 - Flags: review?(bent.mozilla) → review+
Attachment #824558 - Flags: review?(bent.mozilla) → review+
Oh, that test just needs to be disabled on B2G Desktop, I'll do it.
https://hg.mozilla.org/mozilla-central/rev/fc6579a5b551
https://hg.mozilla.org/mozilla-central/rev/ce8a25e53bda
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
I think we should land this on aurora too, otherwise FF27 will ship with "automatic lazy IDB initialization". See bug 932345.
Let's get the right flags set.
You need to log in before you can comment on or make changes to this bug.