Last Comment Bug 666643 - convert snapshotWindow to SpecialPowers
: convert snapshotWindow to SpecialPowers
Status: RESOLVED FIXED
[specialpowers][inbound]
:
Product: Testing
Classification: Components
Component: Mochitest (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Joel Maher ( :jmaher)
:
:
Mentors:
Depends on: 666636 677002 737755
Blocks: 649360
  Show dependency treegraph
 
Reported: 2011-06-23 10:51 PDT by Joel Maher ( :jmaher)
Modified: 2014-08-01 08:19 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
move snapshotWindow to SpecialPowers (1.0) (6.42 KB, patch)
2011-06-23 10:53 PDT, Joel Maher ( :jmaher)
ted: review-
Details | Diff | Splinter Review
move snapshotWindow to SpecialPowers (2.0) (5.20 KB, patch)
2011-07-21 04:32 PDT, Joel Maher ( :jmaher)
ted: review+
Details | Diff | Splinter Review
move snapshotWindow to SpecialPowers (3.0) (4.09 KB, patch)
2011-10-11 06:05 PDT, Joel Maher ( :jmaher)
ted: review+
Details | Diff | Splinter Review

Description Joel Maher ( :jmaher) 2011-06-23 10:51:16 PDT
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) 2011-06-23 10:53:03 PDT
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
Comment 2 Ted Mielczarek [:ted.mielczarek] 2011-06-30 11:47:46 PDT
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) 2011-07-21 04:32:22 PDT
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:)
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-07-21 12:01:21 PDT
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 6 Boris Zbarsky [:bz] (still a bit busy) 2011-08-05 19:24:54 PDT
This broke some tests.  See bug 677002.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-09-23 09:33:13 PDT
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) 2011-10-11 06:05:10 PDT
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.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-10-13 08:13:38 PDT
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 11 Ed Morley [:emorley] 2011-10-15 05:39:58 PDT
https://hg.mozilla.org/mozilla-central/rev/8e0f443496ac

Note You need to log in before you can comment on or make changes to this bug.