Closed Bug 1135091 Opened 5 years ago Closed 5 years ago

convert remaining SpecialPowers.setBoolPref to pushPrefEnv

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(firefox38 fixed, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: anishchandran94, Assigned: anishchandran94, Mentored)

References

()

Details

Attachments

(1 file, 8 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0
Build ID: 20140715214327

Steps to reproduce:

Change existing callers of SpecialPowers.setBoolPref/setIntPref/setCharPref to SpecialPowers.pushPrefEnv
Mentor: jmaher, martijn.martijn
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: mochitest → convert remaining SpecialPowers.setBoolPref to pushPrefEnv
Assignee: nobody → anishchandran94
Depends on: 1056851
I think we should start with a patch of just these files:
layout: https://dxr.mozilla.org/mozilla-central/search?limit=100&redirect=false&q=SpecialPowers.setBoolPref%20path%3Alayout%2F

then we can work on dom/mobilemessage:
https://dxr.mozilla.org/mozilla-central/search?q=SpecialPowers.setBoolPref+path%3Adom%2Fmobilemessage%2F&case=true&redirect=true  (to be honest these are marionette, so we should ensure we know how to test these on try)
I filed bug 1135383 on converting the marionette unit tests.
Attached patch setBool1.patch (obsolete) — Splinter Review
Comment on attachment 8567528 [details] [diff] [review]
setBool1.patch

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

::: layout/base/tests/chrome/test_bug458898.html
@@ +32,2 @@
>  }
>  

You forgot to indent this properly.

::: layout/base/tests/chrome/transformed_scrolling_repaints_3_window.html
@@ +40,5 @@
>  var SpecialPowers = window.opener.wrappedJSObject.SpecialPowers;
>  var is = window.opener.wrappedJSObject.is;
>  
>  var smoothScrollPref = "general.smoothScroll";
>  SpecialPowers.setBoolPref(smoothScrollPref, false);

You forgot to remove this

::: layout/forms/test/test_bug345267.html
@@ +34,1 @@
>  

You need to add the SimpleTest.waitForExplicitFinish() and SimpleTest.finish() logic here, because pushPrefEnv is asynchronous and this test doesn't have the waitForExplicitFinish stuff yet.

::: layout/forms/test/test_bug365410.html
@@ +120,5 @@
>    upDownTest("test4",-1);
>  
>    SimpleTest.finish();
>  }
> +// Turn off spatial nav so that it does not hijack the up and down events.

Keep this comment right above the pushPrefEnv call.

::: layout/forms/test/test_bug563642.html
@@ +72,5 @@
>    upDownTest("test4",-1);
>  
>    SimpleTest.finish();
>  }
> +// Turn off Spatial Navigation because it hijacks down and up key events.

Just keep this comment right above the pushPrefEnv call.

::: layout/forms/test/test_select_prevent_default.html
@@ +17,5 @@
>  
>    function preventDefault(event) {
>      event.preventDefault();
>    }
> +  // Turn off Spatial Navigation because it hijacks arrow keydown events.

Keep this comment right above the pushPrefEnv call.
Attachment #8567528 - Flags: review-
Attached patch setBool1.patch (obsolete) — Splinter Review
Attachment #8567528 - Attachment is obsolete: true
Comment on attachment 8567535 [details] [diff] [review]
setBool1.patch

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

::: layout/forms/test/test_bug365410.html
@@ +122,5 @@
>    SimpleTest.finish();
>  }
>  
>  SimpleTest.waitForExplicitFinish();
> +// Turn off spatial nav so that it does not hijack the up and down events.

I'd rather see it just above the pushPrefEnv.

::: layout/forms/test/test_bug563642.html
@@ +76,2 @@
>  SimpleTest.waitForExplicitFinish();
> +// Turn off Spatial Navigation because it hijacks down and up key events.

Also here.
Attachment #8567535 - Flags: review+
Attachment #8567535 - Flags: review?(jmaher)
Attached patch marionette.patch (obsolete) — Splinter Review
patch for marionette files
Attachment #8567625 - Flags: review+
(In reply to Anish from comment #8)
> Created attachment 8567625 [details] [diff] [review]
> marionette.patch
> 
> patch for marionette files

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c8dd77be8ba
Comment on attachment 8567535 [details] [diff] [review]
setBool1.patch

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

::: layout/base/tests/chrome/transformed_scrolling_repaints_3_window.html
@@ +59,5 @@
> +        window.close();
> +      } else {
> +        t.contentWindow.scrollByLines(1);
> +        waitForAllPaintsFlushed(nextIteration);
> +      }

do we need to have the nextIteration function included inside the anonymous callback function?

couldn't we have a:
function setup() {
 specialpowers.pushprevenv({}, runTests);
}

I am not against this specific change as done, but I wanted to know why it was done this way.

::: layout/forms/test/test_select_prevent_default.html
@@ +22,5 @@
>    function test() {
> +    // Turn off Spatial Navigation because it hijacks arrow keydown events.
> +    SpecialPowers.pushPrefEnv({"set":[["snav.enabled", false]]}, runTest);
> +  }
> +  

nit: blank line has whitespace.

::: layout/generic/test/test_bug791616.html
@@ +58,2 @@
>      });
> +  }); 

nit: trailing whitespace after ;
Attachment #8567535 - Flags: review?(jmaher) → review+
(In reply to Anish from comment #8)
> Created attachment 8567625 [details] [diff] [review]
> marionette.patch
> 
> patch for marionette files

As I mentioned in bug 1135383 (I thought you would attach the patch in that bug), you can also change:
testing/marionette/client/marionette/tests/unit/test_switch_remote_frame.py 
testing/marionette/client/marionette/tests/unit/test_getactiveframe_oop.py (this test doesn't seem to work currently, see bug 925688, comment 124)
Comment on attachment 8567625 [details] [diff] [review]
marionette.patch

This is now done in bug 1135383.
Attachment #8567625 - Attachment is obsolete: true
Attached patch setBool1.patch (obsolete) — Splinter Review
made the changes :jmaher
Attachment #8567535 - Attachment is obsolete: true
Comment on attachment 8567682 [details] [diff] [review]
setBool1.patch

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

Anish, can you set the review flag to me and/or jmaher, otherwise we won't see that you updated your patch.
Attachment #8567682 - Flags: review?(jmaher)
Comment on attachment 8567682 [details] [diff] [review]
setBool1.patch

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

R+, assuming you have tested this (we'll have to do a tryserver run anyway).
I'm not sure about the changes in transformed_scrolling_repaints_3_window.html, but I'll let Joel decide about that.

::: layout/base/tests/chrome/transformed_scrolling_repaints_3_window.html
@@ +74,5 @@
> +          // Clear paint state now and scroll again.
> +          utils.checkAndClearPaintedState(e);
> +          t.contentWindow.scrollByLines(1);
> +          waitForAllPaintsFlushed(nextIteration);
> +        });

Uh, this is what jmaher wanted?
Attachment #8567682 - Flags: review+
Comment on attachment 8567682 [details] [diff] [review]
setBool1.patch

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

functionality this is all good, but I would like to rework transformed-scrolling_repaints_3_window.html

::: layout/base/tests/chrome/transformed_scrolling_repaints_3_window.html
@@ +61,5 @@
> +        window.close();
> +      } else {
> +        t.contentWindow.scrollByLines(1);
> +        waitForAllPaintsFlushed(nextIteration);
> +      }

I don't like how we have nextIteration defined inside of runTest().  Can we clean this up a bit better?

::: layout/generic/test/test_bug791616.html
@@ +33,5 @@
>  t.scrollTop = 0;
>  var targetY = target.getBoundingClientRect().top;
>  
>  SimpleTest.waitForFocus(function() {
> +  SpecialPowers.pushPrefEnv({"set":[[smoothScrollPref, false]]}, function() {

I would prefer to create a function runTest() and then just call it from pushPrefEnv.
Attachment #8567682 - Flags: review?(jmaher) → review-
Attached patch Bool.patch (obsolete) — Splinter Review
Attachment #8567682 - Attachment is obsolete: true
Attached patch Bool.patch (obsolete) — Splinter Review
Attachment #8570925 - Attachment is obsolete: true
Comment on attachment 8570931 [details] [diff] [review]
Bool.patch

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

::: layout/base/tests/chrome/transformed_scrolling_repaints_3_window.html
@@ +76,5 @@
> +  } else {
> +    t.contentWindow.scrollByLines(1);
> +    waitForAllPaintsFlushed(nextIteration);
> +  }
> +} 

nit: trailing whitespace
Attachment #8570931 - Flags: review+
the try server push is for marionette, we need one for mochitests.
when you address the nit, please add a commit message to the patch, it is easiest to just copy the but number and title from the top of the bugzilla report "Bug 1135091 - convert remaining SpecialPowers.setBoolPref to pushPrefEnv" and add a r=<name> at the end.
Attached patch Bool.patch (obsolete) — Splinter Review
Attachment #8570931 - Attachment is obsolete: true
Attached patch Bool.patch (obsolete) — Splinter Review
Attachment #8571360 - Attachment is obsolete: true
Attachment #8571371 - Flags: review?(jmaher)
Attached patch Bool.patchSplinter Review
haha ! Finally !
Attachment #8571371 - Attachment is obsolete: true
Attachment #8571371 - Flags: review?(jmaher)
https://hg.mozilla.org/mozilla-central/rev/346347b1255d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(In reply to Joel Maher (:jmaher) from comment #29)
> we have a few more cases here:
> https://dxr.mozilla.org/mozilla-central/
> search?limit=100&redirect=false&q=SpecialPowers.
> setBoolPref%20path%3Alayout%2F
> https://dxr.mozilla.org/mozilla-central/
> search?limit=100&redirect=false&q=SpecialPowers.setBoolPref%20path%3Adom/
> mobilemessage%2F
> https://dxr.mozilla.org/mozilla-central/
> search?limit=100&redirect=false&q=SpecialPowers.setBoolPref%20path%3Adom/
> tests%2F

We're trying to fix those in bug 1139343, so marking this bug fixed.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.