convert snapshotWindow to SpecialPowers

RESOLVED FIXED in mozilla10

Status

Testing
Mochitest
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

unspecified
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [specialpowers][inbound])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
Created attachment 541430 [details] [diff] [review]
move snapshotWindow to SpecialPowers (1.0)

patch that works on try server!  This requires the patch from bug 666636
Assignee: nobody → jmaher
Attachment #541430 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

6 years ago
Depends on: 666636
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-
(Assignee)

Comment 3

6 years ago
Created attachment 547363 [details] [diff] [review]
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:)
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+
(Assignee)

Updated

6 years ago
Whiteboard: [specialpowers] → [specialpowers][inbound]
http://hg.mozilla.org/mozilla-central/rev/f1a42c14a26a
Status: NEW → RESOLVED
Last Resolved: 6 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

6 years ago
Created attachment 566180 [details] [diff] [review]
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.
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+
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0f443496ac
Whiteboard: [specialpowers] → [specialpowers][inbound]
https://hg.mozilla.org/mozilla-central/rev/8e0f443496ac
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 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.