666643 - convert snapshotWindow to SpecialPowers

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

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

in mochitest eventutils.js, we have a snapshotWindow function which requires enablePrivilege.  We can move this to SpecialPowers and all the tests which use this function will work without enablePrivilege

Comment 1

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

Attachment: move snapshotWindow to SpecialPowers (1.0)

patch that works on try server!  This requires the patch from bug 666636

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 541430 [details] [diff] [review]
move snapshotWindow to SpecialPowers (1.0)

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

r- for the one comment below, but if you have a rebuttal I am all ears. Otherwise this looks good.

::: layout/base/tests/chrome/chrome_content_integration_window.xul
@@ +30,2 @@
>  
> +      var refCanvas = window.opener.SpecialPowers.snapshotWindow(window);

Why doesn't this window get its own SpecialPowers?

::: testing/mochitest/specialpowers/content/specialpowers.js
@@ +51,5 @@
>    this._messageListener = this._messageReceived.bind(this);
>    addMessageListener("SPPingService", this._messageListener);
>  }
>  
> +function bindDOMWindowUtils(window) {

As I mentioned on IRC, I don't see the purpose of these changes. They don't seem to be a functional change, so I'd rather not change the code if it's not necessary.

@@ +221,5 @@
>                         .getInterface(Ci.nsIWebNavigation);
>      webNav.loadURI(uri, referrer, charset, x, y);
>    },
>  
> +  snapshotWindow: function (win, withCaret) {

We should probably start putting short descriptive comments when we add APIs here. Also don't forget to update the docs on MDN to add this!

@@ +231,5 @@
> +
> +    ctx.drawWindow(win, win.scrollX, win.scrollY,
> +                   win.innerWidth, win.innerHeight,
> +                   "rgb(255,255,255)",
> +                   withCaret ? ctx.DRAWWINDOW_DRAW_CARET : 0);

Maybe an odd question, but for e10s/fennec, are they going to want to test with the bits being drawn in the content process, or from the chrome process?

Comment 3

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

Attachment: move snapshotWindow to SpecialPowers (2.0)

new and improved, passes on try and incorporates all your feedback.  I looked into the e10s issues for drawWindow and there is no need for that.  We are in specialpowers and have access to content.  If we cared about the chrome elements, we would need to snapshot in chrome and the problem becomes exponentially more difficult.  If that is the case the tests should be reftests:)

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 547363 [details] [diff] [review]
move snapshotWindow to SpecialPowers (2.0)

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

::: testing/mochitest/specialpowers/content/specialpowers.js
@@ +70,5 @@
>        var oldval = desc[prop];
> +      try {
> +        desc[prop] = function() { return oldval.apply(util, arguments); };
> +      } catch (ex) {
> +        dump("WARNING: Special Powers failed to rebind function: " + desc + "::" + prop + "\n");

Were you seeing this in actual testing? This would be a pretty bad failure.

Comment 5

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/f1a42c14a26a

Comment 6

[Boris Zbarsky [:bzbarsky]]

This broke some tests.  See bug 677002.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Backed out to restore test coverage.

As far as doing this "right", seems like we should just make snapshotWindow check whether SpecialPowers is defined and if so use that, else use the old codepath.

Or if the latter is undesirable and we want to go through SpecialPowers in chrome as well, we should think about how to best do that.

Comment 8

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

Attachment: move snapshotWindow to SpecialPowers (3.0)

updated patch after we backed the old one out.  This has been tested on try and locally to verify the tests we failed to run before are running.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 566180 [details] [diff] [review]
move snapshotWindow to SpecialPowers (3.0)

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

::: testing/mochitest/tests/SimpleTest/WindowSnapshot.js
@@ +8,5 @@
>    gWindowUtils = null;
>  }
>  
>  function snapshotWindow(win, withCaret) {
> +  return SpecialPowers.snapshotWindow(win, withCaret);

Is it too much churn to just replace callers of snapshotWindow with SpecialPowers.snapshotWindow?

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0f443496ac

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e0f443496ac