1135091 - convert remaining SpecialPowers.setBoolPref to pushPrefEnv

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Anish]

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

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

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)

Comment 2

[Martijn Wargers (dead)]

I filed bug 1135383 on converting the marionette unit tests.

Comment 3

[Anish]

Attachment: setBool1.patch

Comment 4

[Martijn Wargers (dead)]

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.

Comment 5

[Anish]

Attachment: setBool1.patch

Comment 6

[Martijn Wargers (dead)]

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.

Comment 7

[Martijn Wargers (dead)]

(In reply to Anish from comment #5)
> Created attachment 8567535 [details] [diff] [review]
> setBool1.patch

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3950bfa3313a

Comment 8

[Anish]

Attachment: marionette.patch

patch for marionette files

Comment 9

[Martijn Wargers (dead)]

(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 10

[Joel Maher ( :jmaher ) (UTC -8)]

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 ;

Comment 11

[Martijn Wargers (dead)]

(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 12

[Martijn Wargers (dead)]

Comment on attachment 8567625 [details] [diff] [review]
marionette.patch

This is now done in bug 1135383.

Comment 13

[Anish]

Attachment: setBool1.patch

made the changes :jmaher

Comment 14

[Martijn Wargers (dead)]

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.

Comment 15

[Martijn Wargers (dead)]

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?

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 17

[Anish]

Attachment: Bool.patch

Comment 18

[Anish]

Attachment: Bool.patch

Comment 19

[Martijn Wargers (dead)]

(In reply to Anish from comment #18)
> Created attachment 8570931 [details] [diff] [review]
> Bool.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9330a201471a

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

the try server push is for marionette, we need one for mochitests.

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 24

[Anish]

Attachment: Bool.patch

Comment 25

[Anish]

Attachment: Bool.patch

Comment 26

[Anish]

Attachment: Bool.patch

haha ! Finally !

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/346347b1255d

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/346347b1255d

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

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

There are others besides those above, but fixing those would leave the list much shorter!

Comment 30

[Martijn Wargers (dead)]

(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.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8d23b1e2cc0f

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/02547d8f9c13