Closed Bug 1252444 Opened 4 years ago Closed 4 years ago

Rewrite some dom mochitests to use pushPrefEnv instead of setCharPref

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 1 obsolete file)

Patch coming up
Attached patch 1252444_setcharpref.diff (obsolete) — Splinter Review
Perhaps someone else should review this, Joel? Although the changes are quite simple. Sorry, one of them is a chrome mochitest instead of a plain mochitest.
Attachment #8725234 - Flags: review?(jmaher)
Comment on attachment 8725234 [details] [diff] [review]
1252444_setcharpref.diff

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

all is well but one file.

::: dom/base/test/test_navigator_language.html
@@ +76,5 @@
>        nextTest();
>      }
>  
>      setTimeout(function() {
> +      SpecialPowers.pushPrefEnv({"set": [['intl.accept_languages', 'testArrayCached']]});

why do we have this without a function, do we really clear the prefs?  This is a common pattern in this file.  Is it possible to rewrite this file more?  If not, can we verify that the prefs are cleared upon exit?
Attachment #8725234 - Flags: review?(jmaher) → review-
(In reply to Joel Maher (:jmaher) from comment #3)
> why do we have this without a function, do we really clear the prefs?  This
> is a common pattern in this file.  Is it possible to rewrite this file more?
> If not, can we verify that the prefs are cleared upon exit?

That's tested by:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/Harness_sanity/test_SpecialPowersExtension.html?force=1#186
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/Harness_sanity/test_SpecialPowersExtension2.html?force=1#14

I believe changing the intl.accept_languages pref is triggering languagechange event in this test, that's why there is no follow-up function in these pushPrefEnv calls.
Is that a good explanation or do you still think I should rewrite this file more?
Flags: needinfo?(jmaher)
Comment on attachment 8725234 [details] [diff] [review]
1252444_setcharpref.diff

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

thanks for the clarification!
Attachment #8725234 - Flags: review- → review+
Comment on attachment 8725234 [details] [diff] [review]
1252444_setcharpref.diff

Thanks Joel.
I'll let Mounir also look at the test_navigator_language.html changes, just to be sure
Flags: needinfo?(jmaher)
Attachment #8725234 - Flags: review?(mounir)
Whiteboard: btpp-active
Comment on attachment 8725234 [details] [diff] [review]
1252444_setcharpref.diff

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

I'm afraid I don't have the bandwidth to review that :(
Attachment #8725234 - Flags: review?(mounir)
:mw22, I am fine with a try run, maybe retrigger the jobs that run this test a few times in case it is intermittently green :)
Ok, I retriggered the jobs with the affected tests 5 times.
Retriggered the jobs with the affected tests 5 times and they were all green.
Attachment #8725234 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1b0a91b8897f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.