<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dcfb660c4f2

Reporter

Comment 13

•

11 years ago

Comment on attachment 8561001 [details] [diff] [review] setBool.patch ># HG changeset patch ># Parent 940118b1adcd83967fbd49c96217857a91a2b2d0 >diff --git a/accessible/tests/mochitest/elm/test_canvas.html b/accessible/tests/mochitest/elm/test_canvas.html >--- a/accessible/tests/mochitest/elm/test_canvas.html >+++ b/accessible/tests/mochitest/elm/test_canvas.html > SimpleTest.waitForExplicitFinish(); >- addA11yLoadEvent(doTest); >- </script> >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['canvas.hitregions.enabled', true] >+ ] >+}, function fn(){addA11yLoadEvent(doTest);}) You have the indentation wrong here. = > SimpleTest.waitForExplicitFinish(); >- addA11yLoadEvent(doTest); >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['canvas.hitregions.enabled', true] >+ ] >+}, function fn(){addA11yLoadEvent(doTest);}) Again, indentation seems wrong here, please fix that. > </script> > </head> > <body> > > <a target="_blank" > href="https://bugzilla.mozilla.org/show_bug.cgi?id=966591" > title="nsIAccessible::childAtPoint() for canvas hit regions from browser tests">Mozilla Bug 966591</a> > >diff --git a/browser/base/content/test/general/test_contextmenu.html b/browser/base/content/test/general/test_contextmenu.html >--- a/browser/base/content/test/general/test_contextmenu.html >+++ b/browser/base/content/test/general/test_contextmenu.html >-var subwindow = window.open("./subtst_contextmenu.html", "contextmenu-subtext", "width=600,height=800"); >-subwindow.addEventListener("MozAfterPaint", waitForEvents, false); >-subwindow.onload = waitForEvents; >+function onLoad() >+{ >+ var subwindow = window.open("./subtst_contextmenu.html", "contextmenu-subtext", "width=600,height=800"); >+ subwindow.addEventListener("MozAfterPaint", waitForEvents, false); >+ subwindow.onload = waitForEvents; >+} > >+SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['full-screen-api.approval-required', false], >+ ['full-screen-api.allow-trusted-requests-only', false], >+ ['plugins.click_to_play', true] >+ ] >+}, onLoad()) The () part of onLoad can be removed. Also, perhaps better do name the function startTest or something? I don't think the name onLoad is really correct here. > SimpleTest.waitForExplicitFinish(); > </script> > </pre> > </body> > </html> >diff --git a/caps/tests/mochitest/test_app_principal_equality.html b/caps/tests/mochitest/test_app_principal_equality.html >--- a/caps/tests/mochitest/test_app_principal_equality.html >+++ b/caps/tests/mochitest/test_app_principal_equality.html >@@ -11,19 +11,16 @@ https://bugzilla.mozilla.org/show_bug.cg > </head> > <body> > <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=777467">Mozilla Bug 777467</a> > <p id="display"></p> > <script> > // Initialization. > SpecialPowers.addPermission("browser", true, document); > SpecialPowers.addPermission("embed-apps", true, document); You can replace these addPermission calls with pushPermissions. It's a similar api as pushPrefEnv. Otoh, you can do that later, if you prefer. >- >- SpecialPowers.setBoolPref('dom.mozBrowserFramesEnabled', true); >- SpecialPowers.setBoolPref("dom.ipc.browser_frames.oop_by_default", false); > </script> > <div id="content" style="display: none;"> > <iframe src="error404"></iframe> > <iframe mozbrowser src="error404"></iframe> > <iframe mozapp="http://example.org/manifest.webapp" mozbrowser src="error404"></iframe> > </div> > <pre id="test"> > <script type="application/javascript"> >@@ -37,32 +34,36 @@ function canAccessDocument(win) { > try { > win.document; > } catch(e) { > result = false; > } > return result; > } > >-addLoadEvent(function() { >+SimpleTest.waitForExplicitFinish(); >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['dom.mozBrowserFramesEnabled', true], >+ ['dom.ipc.browser_frames.oop_by_default', false] >+ ] >+}, function fn(){addLoadEvent(function() { I think it's better to just name a function (startTest?) and then seperate it from pushPrefEnv, it makes it easier to read. So SpecialPowers.pushPrefEnv("set": [prefs], startTest) Or in this case when you also do the pushPermissions thing, then you do something like: SpecialPowers.pushPrefEnv("set": [prefs], function(){ SpecialPowers.pushPermissions([....], startTest)) >diff --git a/docshell/test/navigation/test_bug430723.html b/docshell/test/navigation/test_bug430723.html >--- a/docshell/test/navigation/test_bug430723.html >+++ b/docshell/test/navigation/test_bug430723.html >@@ -32,25 +32,27 @@ var gTallRedBoxURI = "data:text/html;cha > var gTallBlueBoxURI = "data:text/html;charset=utf-8;base64,PGh0bWw%2BPGhlYWQ%2BPHNjcmlwdD53aW5kb3cuYWRkRXZlbnRMaXN0ZW5lcigncGFnZXNob3cnLCBmdW5jdGlvbigpe29wZW5lci5uZXh0VGVzdCgpO30sIGZhbHNlKTs8L3NjcmlwdD48L2hlYWQ%2BPGJvZHk%2BPGRpdiBzdHlsZT0icG9zaXRpb246YWJzb2x1dGU7IGxlZnQ6MHB4OyB0b3A6MHB4OyB3aWR0aDo1MCU7IGhlaWdodDoxNTAlOyBiYWNrZ3JvdW5kLWNvbG9yOmJsdWUiPjxwPlRoaXMgaXMgYSB2ZXJ5IHRhbGwgYmx1ZSBib3guPC9wPjwvZGl2PjwvYm9keT48L2h0bWw%2B"; > // <html><head> > // < script > window.addEventListener("pageshow", function(){opener.nextTest();}, false); < /script > > // </head><body> > // <div style="position:absolute; left:0px; top:0px; width:50%; height:150%; background-color:blue"> > // <p>This is a very tall blue box.</p> > // </div></body></html> > >- >-window.onload = runTest; >+var smoothScrollPref = "general.smoothScroll"; >+SpecialPowers.pushPrefEnv({ >+ "set": [ >+ [smoothScrollPref, false] >+ ] >+}, function fn(){window.onload = runTest;}) This is not good. This pushPrefEnv call might be finished before or after the window has loaded. You should call the pushPrefEnv function inside the window.onload. >diff --git a/dom/apps/tests/test_app_update.html b/dom/apps/tests/test_app_update.html >--- a/dom/apps/tests/test_app_update.html >+++ b/dom/apps/tests/test_app_update.html >@@ -13,25 +13,24 @@ https://bugzilla.mozilla.org/show_bug.cg > /** Test for Bug 826058 **/ > > SimpleTest.waitForExplicitFinish(); > > var gBaseURL = 'http://test/tests/dom/apps/tests/'; > var gHostedManifestURL = gBaseURL + 'file_app.sjs?apptype=hosted&getmanifest=true'; > var gCachedManifestURL = gBaseURL + 'file_app.sjs?apptype=cached&getmanifest=true'; > var gTrustedManifestURL = gBaseURL + 'file_app.sjs?apptype=trusted&getmanifest=true'; >- var gGenerator = runTest(); >- >+ var gGenerator; > // We need to set the trusted hosted app csp pref since it's only in > // b2g.js for now. > function setCSPPrefs() { > SpecialPowers.pushPrefEnv({'set': > [["security.apps.trusted.CSP.default", >- "default-src *; object-src 'none'"]]}, >- function() { gGenerator.next(); }); >+ "default-src *; object-src 'none'"],["dom.mozBrowserFramesEnabled",true]]}, >+ function() { gGenerator = runTest(); gGenerator.next(); }); I hope this works. runTest() seems like an asynchronous function, which gGenerator.next() might depend upon. >diff --git a/dom/base/test/test_EventSource_redirects.html b/dom/base/test/test_EventSource_redirects.html >--- a/dom/base/test/test_EventSource_redirects.html >+++ b/dom/base/test/test_EventSource_redirects.html >@@ -44,16 +43,20 @@ https://bugzilla.mozilla.org/show_bug.cg > ok(false, "received onError: " + event); > source.close(); > SimpleTest.finish(); > }; > > } > > SimpleTest.waitForExplicitFinish(); >- addLoadEvent(doTest); >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['dom.server-events.enabled', true] >+ ] >+}, function fn(){addLoadEvent(doTest);}) The other way around please. The pushPrefEnv call inside the addLoadEvent function. >diff --git a/dom/base/test/test_bug666604.html b/dom/base/test/test_bug666604.html >--- a/dom/base/test/test_bug666604.html >+++ b/dom/base/test/test_bug666604.html >@@ -111,43 +111,48 @@ function test6() { > test7(); > } > dispatchDOMActivate(testlink); > } > > var oldPref; > function test7() { > oldPref = SpecialPowers.getBoolPref("dom.disable_open_during_load"); >- SpecialPowers.setBoolPref("dom.disable_open_during_load", false); > testlink.href = "javascript:opener.activationListener(); window.close();"; > testlink.target = "_blank"; > activationListener = > function() { > ok(true, "Click() should activate a link"); >- setTimeout(test8, 0); >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['dom.disable_open_during_load', false] >+ ] >+}, function fn(){setTimeout(test8, 0);}) That seems weird to put it in here. Also the indentation is wrong. > } > > function test8() { >- SpecialPowers.setBoolPref("dom.disable_open_during_load", true); > testlink.href = "javascript:opener.activationListener(); window.close();"; > testlink.target = "_blank"; > activationListener = > function() { > ok(false, "Click() should not activate a link"); >- setTimeout(test9, 0); >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['dom.disable_open_during_load', true] >+ ] >+}, function fn(){setTimeout(test9, 0);}); Here also. >diff --git a/dom/base/test/test_bug715041.xul b/dom/base/test/test_bug715041.xul >--- a/dom/base/test/test_bug715041.xul >+++ b/dom/base/test/test_bug715041.xul >@@ -14,18 +14,17 @@ xmlns="http://www.mozilla.org/keymaster/ >- caseZero(); >- >+ SimpleTest.waitForExplicitFinish(); >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['dom.idle-observers-api.fuzz_time.disabled', true] >+ ] >+}, caseZero();) Indentation wrong and get rid of () in caseZero. >diff --git a/dom/base/test/test_bug715041_removal.xul b/dom/base/test/test_bug715041_removal.xul >--- a/dom/base/test/test_bug715041_removal.xul >+++ b/dom/base/test/test_bug715041_removal.xul >+ >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['dom.idle-observers-api.fuzz_time.disabled', true] >+ ] >+}, RemoveHeadIdleObserverWhileActive();}) >+ Indentation wrong. > ]]> > </script> > </window> > >diff --git a/dom/base/test/test_child_process_shutdown_message.html b/dom/base/test/test_child_process_shutdown_message.html >--- a/dom/base/test/test_child_process_shutdown_message.html >+++ b/dom/base/test/test_child_process_shutdown_message.html >@@ -112,25 +112,26 @@ function expectFrameProcessShutdown(ifra > ok(true, "Received 'message-manager-disconnect' notification with " + > "frame message manager"); > countMessage(); > } > }, "message-manager-disconnect", false); > } > > function setUp() { >- SpecialPowers.setBoolPref("dom.mozBrowserFramesEnabled", true); >- SpecialPowers.setBoolPref("dom.ipc.browser_frames.oop_by_default", true); > SpecialPowers.addPermission("browser", true, window.document); > SpecialPowers.addPermission("embed-apps", true, window.document); > >- // TODO: remove in bug 820712 >- SpecialPowers.setBoolPref("network.disable.ipc.security", true); >- >- runNextTest(); >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['dom.mozBrowserFramesEnabled', true], >+ ['dom.ipc.browser_frames.oop_by_default', true], >+ ['network.disable.ipc.security', true] >+ ] >+}, function fn(){runNextTest();}) > } Indentation wrong and just use runNextTest here. >diff --git a/dom/base/test/test_messagemanager_assertpermission.html b/dom/base/test/test_messagemanager_assertpermission.html >--- a/dom/base/test/test_messagemanager_assertpermission.html >+++ b/dom/base/test/test_messagemanager_assertpermission.html >@@ -22,26 +22,29 @@ const CHILD_PROCESS_SHUTDOWN_MESSAGE = " > let ppmm = SpecialPowers.Cc["@mozilla.org/parentprocessmessagemanager;1"] > .getService(SpecialPowers.Ci.nsIMessageBroadcaster); > let cpmm = SpecialPowers.Cc["@mozilla.org/childprocessmessagemanager;1"] > .getService(SpecialPowers.Ci.nsISyncMessageSender); > let gAppsService = SpecialPowers.Cc["@mozilla.org/AppsService;1"] > .getService(SpecialPowers.Ci.nsIAppsService); > > function setUp() { >- SpecialPowers.setBoolPref("dom.mozBrowserFramesEnabled", true); >- SpecialPowers.setBoolPref("dom.ipc.browser_frames.oop_by_default", true); > SpecialPowers.addPermission("browser", true, window.document); > SpecialPowers.addPermission("embed-apps", true, window.document); > > let appId = gAppsService.getAppLocalIdByManifestURL(APP_MANIFEST); > SpecialPowers.addPermission("foobar", true, { url: APP_URL, > appId: appId, > isInBrowserElement: false }); >- runNextTest(); >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['dom.mozBrowserFramesEnabled', true], >+ ['dom.ipc.browser_frames.oop_by_default', true] >+ ] >+}, runNextTest()); > } Indentation wrong and get rid of (). >diff --git a/dom/base/test/test_websocket_permessage_deflate.html b/dom/base/test/test_websocket_permessage_deflate.html >--- a/dom/base/test/test_websocket_permessage_deflate.html >+++ b/dom/base/test/test_websocket_permessage_deflate.html >+function loadDeflate(){ >+ SpecialPowers.pushPrefEnv({ >+ "set": [ >+ ['network.websocket.extensions.permessage-deflate', tests[testIdx][0]] >+ ] >+}, testDeflate()}) >+} Indentation wrong and get rid of (). > SimpleTest.waitForExplicitFinish(); > > </script> > </pre> > </body> > </html> >diff --git a/dom/browser-element/mochitest/priority/test_Preallocated.html b/dom/browser-element/mochitest/priority/test_Preallocated.html >--- a/dom/browser-element/mochitest/priority/test_Preallocated.html >+++ b/dom/browser-element/mochitest/priority/test_Preallocated.html >-addEventListener('testready', runTest); >+SpecialPowers.pushPrefEnv({'set': >+ [["dom.ipc.processPrelaunch.enabled",true]]}, >+ function() { addEventListener('testready', runTest); }); I assume 'testready' can't fire earlier than the pushPrefEnv call? I'm going to skip the rest of the patch. But I saw there also cases where you made similar mistakes as I described before. Also make sure that the mochitests run still fine afterwards. If you can't get them to pass, then just leave them out of the patch for now.

Carsten Book [:Tomcat]

Comment 14

•

11 years ago

Keywords: checkin-needed

Kan-Ru Chen [:kanru] (UTC+9)

Comment 15

•

11 years ago

Comment on attachment 8561001 [details] [diff] [review] setBool.patch Review of attachment 8561001 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/mochitest/priority/test_Preallocated.html @@ +66,4 @@ > } > +SpecialPowers.pushPrefEnv({'set': > + [["dom.ipc.processPrelaunch.enabled",true]]}, > + function() { addEventListener('testready', runTest); }); This should be convoluted like addEventListener('testready', function() { SpecialPowers.pushPrefEnv({...}, runTest) });

Comment 16

•

11 years ago

Attached patch setBool.patch (obsolete) — Details — Splinter Review

Updated based on comments with additional files

Attachment #8561001 - Attachment is obsolete: true

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

Comment 17

•

11 years ago

https://hg.mozilla.org/mozilla-central/rev/4dcfb660c4f2

•

11 years ago

Comment 22

•

11 years ago

Attached patch setBool.patch (obsolete) — Details — Splinter Review

Based on previous try and review

Attachment #8562119 - Attachment is obsolete: true

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

Comment 23

•

11 years ago

Reporter

Comment 24

•

11 years ago

I see one failure in test_classified_annotations.html . It might be because you're doing ["privacy.trackingprotection.enabled", true], ["channelclassifier.allowlist_exampl", true] 2 times with pushPrefEnv. You need to get rid of the 2nd one later in the file. For the rest, it seems to be going all well. I noticed various indentation issues in your patch, though. They are easily noticeable, you might want to fix them, before I'll raise them in my review.

Comment 25

•

11 years ago

Comment on attachment 8562970 [details] [diff] [review] setBool.patch Review of attachment 8562970 [details] [diff] [review]: ----------------------------------------------------------------- lots of little things, the try server is very green in comparison to previous ones! Lets give this one more cleanup and then we can test on all the platforms. ::: accessible/tests/mochitest/jsat/test_alive.html @@ +73,5 @@ > title="[AccessFu] Add mochitest for enabling"> > Mozilla Bug 811307 > </a> > </body> > +</html> if there are no changes to this file, please revert any whitespace changes so we don't accidentally modify it. ::: accessible/tests/mochitest/jsat/test_live_regions.html @@ +471,5 @@ > <p id="text_remove_descendant2">Descendant Text Removed</p> > </div> > </div> > </body> > +</html> no changes to this file either, please revert ::: accessible/tests/mochitest/jsat/test_quicknav_modes.html @@ +99,5 @@ > title="[AccessFu] Add mochitest for enabling"> > Mozilla Bug 811307 > </a> > </body> > +</html> no changes to this file either, please revert ::: accessible/tests/mochitest/jsat/test_tables.html @@ +569,5 @@ > <tr><td>Row2</td></tr> > </table> > </div> > </body> > +</html> no changes to this file either, please revert ::: caps/tests/mochitest/test_app_principal_equality.html @@ +19,1 @@ > SpecialPowers.setBoolPref('dom.mozBrowserFramesEnabled', true); I understand there are issues with removing these 3 lines, we can address them in a followup. Please add a comment to indicate that, or remove all changes to this file. ::: dom/base/test/test_bug666604.html @@ +133,3 @@ > } > + testlink.click(); > +SimpleTest.finish(); whitespace after .click(); please indent SimpleTest.finish() properly. ::: dom/base/test/test_child_process_shutdown_message.html @@ +122,1 @@ > // TODO: remove in bug 820712 this comment should apply only to: network.disable.ipc.security, please change it to be: // TODO: remove "network.disable.ipc.security" in bug 820712 ::: dom/base/test/test_websocket_permessage_deflate.html @@ +94,5 @@ > } > } > } > > +function loadDeflate(){ nit: space between () { ::: dom/canvas/test/test_canvas_focusring.html @@ +90,5 @@ > SimpleTest.finish(); > } > > +SpecialPowers.pushPrefEnv({"set":[["canvas.focusring.enabled", true], > + ["canvas.customfocusring.enabled", true]]}, addLoadEvent(runTests)) add a ; at the end the callback should be: function () { addLoadEvent(runTests); } ::: dom/canvas/test/test_canvas_path.html @@ +436,4 @@ > SimpleTest.finish(); > } > > +SpecialPowers.pushPrefEnv({"set":[["canvas.path.enabled", true]]},function(){addLoadEvent(runTests);}) ; at the end ::: dom/canvas/test/test_hitregion_canvas.html @@ +79,3 @@ > } > > +SpecialPowers.pushPrefEnv({"set":[["canvas.hitregions.enabled", true]]}, addLoadEvent(runTests)) nit: ; at the end. ::: dom/datastore/tests/test_bug976311.html @@ +93,5 @@ > SpecialPowers.Cu.import("resource://gre/modules/DataStoreChangeNotifier.jsm"); > } > > SpecialPowers.setAllAppsLaunchable(true); > + SpecialPowers.pushPrefEnv({"set":[["dom.mozBrowserFramesEnabled", true]]}, runTest) nit: missing ; at the end. ::: dom/datastore/tests/test_oop_events.html @@ +117,5 @@ > SpecialPowers.Cu.import("resource://gre/modules/DataStoreChangeNotifier.jsm"); > } > > SpecialPowers.setAllAppsLaunchable(true); > + SpecialPowers.pushPrefEnv({"set":[["dom.mozBrowserFramesEnabled", true]]}, runTest) nit: ; at the end. ::: dom/datastore/tests/test_readonly.html @@ +13,5 @@ > var gHostedManifestURL2 = 'http://example.com/tests/dom/datastore/tests/file_app.sjs?testToken=file_readonly.html&template=file_app2.template.webapp'; > + var gGenerator; > + > + SpecialPowers.pushPrefEnv({"set": [["dom.mozBrowserFramesEnabled", true]]}, function(){gGenerator = runTest()}); > + nit: lines 15 and 17 are blank with just whitespace. ::: dom/datastore/tests/test_sync_worker.html @@ +87,5 @@ > if (SpecialPowers.isMainProcess()) { > SpecialPowers.Cu.import("resource://gre/modules/DataStoreChangeNotifier.jsm"); > } > > + SpecialPowers.pushPrefEnv({"set":[["dom.mozBrowserFramesEnabled", true]]}, runTest) nit: ; at the end. ::: dom/datastore/tests/test_worker_close.html @@ +88,5 @@ > SpecialPowers.Cu.import("resource://gre/modules/DataStoreChangeNotifier.jsm"); > } > > SpecialPowers.setAllAppsLaunchable(true); > + SpecialPowers.pushPrefEnv({"set":[["dom.mozBrowserFramesEnabled", true]]}, runTest) nit: ; at the end. ::: dom/events/test/test_bug607464.html @@ +73,5 @@ > + SpecialPowers.pushPrefEnv({"set":[["general.smoothScroll", true], > + ["mousewheel.acceleration.start", -1], > + ["mousewheel.system_scroll_override_on_root_content.enabled", false] > + ]},function(){ SimpleTest.executeSoon(runTest);}); > + nit: blank line with whitespace, just remove the blank line. ::: dom/events/test/test_eventTimeStamp.html @@ +118,5 @@ > </script> > </pre> > </body> > </html> > + only a blank line added in this file, please revert this. ::: dom/html/test/file_fullscreen-denied.html @@ +42,5 @@ > > // Request full-screen from a non trusted context (this script isn't a user > // generated event!). > + SpecialPowers.pushPrefEnv({"set":[["full-screen-api.allow-trusted-requests-only", true]]}, startTest); > + nit: remove the whitespace at tend of line 45, and whitespace on line 46. @@ +43,5 @@ > // Request full-screen from a non trusted context (this script isn't a user > // generated event!). > + SpecialPowers.pushPrefEnv({"set":[["full-screen-api.allow-trusted-requests-only", true]]}, startTest); > + > + function startTest(){ nit: add space between: () {

Comment 26

•

11 years ago

Attached patch setBool.patch (obsolete) — Details — Splinter Review

Attachment #8562970 - Attachment is obsolete: true

Reporter

Comment 27

•

11 years ago

(In reply to Anish from comment #26) > Created attachment 8563237 [details] [diff] [review] > setBool.patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d014685984b

Reporter

Comment 28

•

11 years ago

2 failures in the try run: +++ b/dom/canvas/test/test_canvas_focusring.html -addLoadEvent(runTests); +SpecialPowers.pushPrefEnv({"set":[["canvas.focusring.enabled", true], + ["canvas.customfocusring.enabled", true]]}, function() { addLoadEvent(runTests) }); You need to put the pushPrefEnv call inside the addLoadEvent (and runTests then has to be inside the pushPrefEnv call. +++ b/toolkit/components/url-classifier/tests/mochitest/test_classified_annotations.html Like I tried to explain you on irc, don't try to set all the prefs here in 1 place, try to keep the pref setting in the same place as it was (so 2 pushPrefEnv calls in 2 separate places). Not sure if it would fix this timeout failure, though. If it doesn't, then leave this test out of the patch, for now.

Reporter

Comment 29

•

11 years ago

Comment on attachment 8563237 [details] [diff] [review] setBool.patch Review of attachment 8563237 [details] [diff] [review]: ----------------------------------------------------------------- I started a bit with reviewing some of the files, tomorrow I'll try to review the rest. ::: accessible/tests/mochitest/elm/test_canvas.html @@ +38,5 @@ > } > > SimpleTest.waitForExplicitFinish(); > + SpecialPowers.pushPrefEnv({"set": [['canvas.hitregions.enabled', true]]}, > + function(){addA11yLoadEvent(doTest);}) This line has to be indented by 2 spaces. ::: accessible/tests/mochitest/hittest/test_canvas_hitregion.html @@ +70,3 @@ > SimpleTest.waitForExplicitFinish(); > + SpecialPowers.pushPrefEnv({"set": [['canvas.hitregions.enabled', true]]}, > + function(){addA11yLoadEvent(doTest);}) Same here. ::: browser/base/content/test/general/test_contextmenu.html @@ +850,3 @@ > setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY); > > +function strtTest() strtTest? Make it startTest please. ::: caps/tests/mochitest/test_app_principal_equality.html @@ +13,5 @@ > <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=777467">Mozilla Bug 777467</a> > <p id="display"></p> > <script> > // Initialization. > + //Removing these lines causes error even after the introduction of pushPrefEnv That's because mozbrowser and mozapp iframes are only instantiated during creation, not afterwards. So you'll have to create the iframes dynamically if you want to get this to work. Perhaps better to tackle this file in a different patch, though. ::: docshell/test/navigation/test_bug430723.html @@ +40,2 @@ > > window.onload = runTest; Why not put the anonmous function with the pushPrefEnv call here, with inside it, the reference to the runTest function? That seems less messy to me. ::: dom/apps/tests/test_app_update.html @@ +22,5 @@ > // We need to set the trusted hosted app csp pref since it's only in > // b2g.js for now. > function setCSPPrefs() { > + SpecialPowers.pushPrefEnv({'set':[["security.apps.trusted.CSP.default", > +"default-src *; object-src 'none'"],["dom.mozBrowserFramesEnabled",true]]}, Note that the common style is to do "], [", not "],[". So please fix that, if you can.

Attachment #8563237 - Flags: review-

•

11 years ago

Attached patch setBool.patch (obsolete) — Details — Splinter Review

Attachment #8563576 - Attachment is obsolete: true

Reporter

Comment 37

•

11 years ago

(In reply to Anish from comment #36) > Created attachment 8564784 [details] [diff] [review] > setBool.patch https://tbpl.mozilla.org/?tree=Try&rev=9fbb145565ce

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

Comment 38

•

11 years ago

Reporter

Comment 39

•

11 years ago

Comment on attachment 8564784 [details] [diff] [review] setBool.patch Review of attachment 8564784 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Martijn Wargers [:mwargers] (QA) from comment #37) > (In reply to Anish from comment #36) > > Created attachment 8564784 [details] [diff] [review] > > setBool.patch > > https://tbpl.mozilla.org/?tree=Try&rev=9fbb145565ce All tests are green! I still found some things that you need to fix (and some minor things), but we're almost there, I think. ::: caps/tests/mochitest/test_app_principal_equality.html @@ +57,5 @@ > for (var i=1; i<frames.length; ++i) { > is(canAccessDocument(frames[i]), false, > "should not be able to access the other frames"); > } > You didn't improve this file. I think it's better to not include this file in this patch, currently. This file needs some major changes, to make it use pushPrefEnv and pushPermissions. ::: dom/base/test/test_bug666604.html @@ +135,1 @@ > SimpleTest.finish(); This should probably be SimpleTest.executeSoon(SimpleTest.finish); ::: dom/html/test/forms/test_input_number_key_events.html @@ +34,1 @@ > }); Sorry, I missed that in my previous review. First call waitForFocus, then pushPrefEnv, then function() {etc.. ::: dom/html/test/forms/test_input_range_key_events.html @@ +34,1 @@ > }); First waitForFocus, then pushPrefEnv, then function() {test(); SimpleTest.finish()} ::: dom/tests/mochitest/general/test_vibrator.html @@ +78,3 @@ > // Everything should be the same when the vibrator is disabled -- in > // particular, a disabled vibrator shouldn't eat failures we'd otherwise > // observe. You removed the case for with dom.vibrator.enabled=false. I think you need to do something like: SpecialPowers.pushPrefEnv({"set": [['dom.vibrator.enabled', true]]}, function() { tests(); SpecialPowers.pushPrefEnv({"set": [['dom.vibrator.enabled', false]]}, tests); }); ::: dom/tests/mochitest/localstorage/test_appIsolation.html @@ +18,5 @@ > > try { > previousPrefs.mozBrowserFramesEnabled = SpecialPowers.getBoolPref('dom.mozBrowserFramesEnabled'); > } catch(e) > { I think you can also remove the previousPrefs.mozBrowserFramesEnabled references. ::: dom/xul/test/test_bug757137.xul @@ +44,5 @@ > + ok(chromeWindow === null, 'mozbrowser iframe should not get ChromeWindow'); > + > + otherWindow.close(); > + SimpleTest.waitForFocus(function() { > + SimpleTest.finish(); I think SimpleTest.waitForFocus(SimpleTest.finish)) also works. ::: extensions/cookie/test/test_permissionmanager_app_isolation.html @@ +35,5 @@ > }; > > try { > previousPrefs.mozBrowserFramesEnabled = SpecialPowers.getBoolPref('dom.mozBrowserFramesEnabled'); > } catch(e) I think you can remove the previousPrefs.mozBrowserFramesEnabled part now in this test.

Attachment #8564784 - Flags: review-

Reporter

Comment 40

•

11 years ago

(In reply to Joel Maher (:jmaher) from comment #38) > https://tbpl.mozilla.org/?tree=Try&rev=ef52c2ba8186 Thanks Joel. I should have done a complete mochitest try run for all platforms. There are some failures there: dom/canvas/test/test_canvas_focusring.html on Win7/WinXP: - There is bug 1005846, which calls it an intermittent failure. I think this is because of the use of canvas.customfocusring.enabled pref, which enables drawCustomFocusRing. This will be removed in bug 1120371. There is nothing wrong with your code here. In fact, this change here exposes an otherwise intermittent failure lurking in Windows (which is bug 1005846). We should disable this test on Windows. I'll do that in bug 1005846. dom/tests/mochitest/pointerlock/test_pointerlock-api.html on MacOSX 10.6: - you forgot a comma between ['full-screen-api.enabled', true] and ['full-screen-api.allow-trusted-requests-only', false]. editor/libeditor/tests/test_bug674770-2.html on MacOSX10.6 and Windows. - You forgot to set the prefs there, it seems: + SpecialPowers.pushPrefEnv({"set":[["", false], ["", true]]}, startTest); dom/html/test/forms/test_input_radio_radiogroup.html on Android: - You haven't modified this test. However, because of some other files you've changed, you might have exposed a bug that some pref has not been undone with these tests. I suspect that the snav.enabled pref has something to do with this, which is enabled by default on Android. So I think you need to modify this test to enable the snav.enabled pref, e.g., use SpecialPowers.pushPrefEnv snav.enabled here. dom/ipc/tests/test_NuwaProcessCreation.html on B2G emulator: - I think onTestReady might be called more than once in this case. I would just leave this test file out of the patch for now. The test author already knew to use pushPrefEnv, but didn't use it there for some reason.

Comment 41

•

11 years ago

Thanks :mwargers! A good summary of the failures and a review of the patch. Good news is we are down to roughly 5 failures and smaller review feedback- this is shaping up to be a good week :)

Comment 42

•

11 years ago

Attached patch setBool.patch (obsolete) — Details — Splinter Review

Attachment #8564784 - Attachment is obsolete: true

Reporter

Comment 43

•

11 years ago

(In reply to Anish from comment #42) > Created attachment 8565070 [details] [diff] [review] > setBool.patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=33a2908a4367

Reporter

•

11 years ago

Attached patch setBool.patch (obsolete) — Details — Splinter Review

Attachment #8565531 - Attachment is obsolete: true

Attachment #8565548 - Attachment is obsolete: true

Comment 52

Comment 68

•

11 years ago

(In reply to Anish from comment #67) > Created attachment 8566524 [details] [diff] [review] > setBool.patch Pushed to try for the crashtest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2e8ba0c916

•

11 years ago

Looks like it stuck this time. Let's resolve this bug! Thanks for you work, Kaustabh and Anish!

Status: NEW → RESOLVED

Closed: 11 years ago

Resolution: --- → FIXED