Closed Bug 666643 Opened 8 years ago Closed 8 years ago

convert snapshotWindow to SpecialPowers


(Testing :: Mochitest, defect)

Not set


(Not tracked)



(Reporter: jmaher, Assigned: jmaher)



(Whiteboard: [specialpowers][inbound])


(1 file, 2 obsolete files)

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
patch that works on try server!  This requires the patch from bug 666636
Assignee: nobody → jmaher
Attachment #541430 - Flags: review?(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?
Attachment #541430 - Flags: review?(ted.mielczarek) → review-
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:)
Attachment #541430 - Attachment is obsolete: true
Attachment #547363 - Flags: review?(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.
Attachment #547363 - Flags: review?(ted.mielczarek) → review+
Whiteboard: [specialpowers] → [specialpowers][inbound]
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [specialpowers][inbound] → [specialpowers]
Target Milestone: --- → mozilla8
This broke some tests.  See bug 677002.
Depends on: 677002
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.
Resolution: FIXED → ---
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.
Attachment #547363 - Attachment is obsolete: true
Attachment #566180 - Flags: review?(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?
Attachment #566180 - Flags: review?(ted.mielczarek) → review+
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: mozilla8 → mozilla10
Depends on: 737755
You need to log in before you can comment on or make changes to this bug.