921478 - Remove BackstagePass IDB constructor resolve hook and use Cu.importGlobalProperties

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

Jan, what's the status here?

Comment 2

[Jan Varga [:janv]]

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.

Comment 3

[Jan Varga [:janv]]

Attachment: patch

Comment 4

[Vendelin Ruzicka [:vendo]]

Attachment: patch part 2

Conversion of test_contacts_upgrade.html to chrome mochitest.
Try https://tbpl.mozilla.org/?tree=Try&rev=532ff0f32eeb

Comment 5

[Vendelin Ruzicka [:vendo]]

Attachment: diff (part 2)

Diff including Patch part 2 with ignore white space option (-w) for better visibility of changes.

Comment 6

[Jan Varga [:janv]]

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

Comment 7

[Vendelin Ruzicka [:vendo]]

Attachment: patch part 2

done, tests running at https://tbpl.mozilla.org/?tree=Try&rev=9243a9dbf43a

Comment 8

[Jan Varga [:janv]]

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

Comment 9

[Vendelin Ruzicka [:vendo]]

Attachment: patch part 2

updated, now it should be ok
https://tbpl.mozilla.org/?tree=Try&rev=3731a184126a

Comment 10

[Jan Varga [:janv]]

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

Comment 11

[Vendelin Ruzicka [:vendo]]

Attachment: patch part 2

bug numbers in test updated

Comment 12

[Jan Varga [:janv]]

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.

Comment 13

[Bobby Holley (:bholley)]

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

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 823300 [details] [diff] [review]
patch

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

Looks good! Sorry for the delay.

Comment 15

[Jan Varga [:janv]]

rebased and pushed

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cda44ae1ba0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e07cab8a2732

Comment 16

[Carsten Book [:Tomcat]]

(In reply to Jan Varga [:janv] from comment #15)
> rebased and pushed
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5cda44ae1ba0
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e07cab8a2732

Hi Jan,

had to backout this changes in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=282c409d2a35 because of perma failures/oranges like https://tbpl.mozilla.org/php/getParsedLog.php?id=30754893&tree=Mozilla-Inbound in B2G Desktop Linux x64 Opt

Comment 17

[Jan Varga [:janv]]

Oh, that test just needs to be disabled on B2G Desktop, I'll do it.

Comment 18

[Jan Varga [:janv]]

https://tbpl.mozilla.org/?tree=Try&rev=db759ea9acbd

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc6579a5b551
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce8a25e53bda

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc6579a5b551
https://hg.mozilla.org/mozilla-central/rev/ce8a25e53bda

Comment 20

[Jan Varga [:janv]]

I think we should land this on aurora too, otherwise FF27 will ship with "automatic lazy IDB initialization". See bug 932345.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Let's get the right flags set.