Currently paint_listener.js can only be used from Chrome mochitests due to the way it fetches DOMWindowUtils. This is due to no_clip_iframe_window.xul (loaded by layout/base/tests/chrome/test_no_clip_iframe.xul) which fails when using SpecialPowers.DOMWindowUtils because of the way SpecialPowers works with parent windows. Basically, when we bind special powers we look for a parent window and use its SpecialPowers if available. For paint_listener.js this means we get a DOMWindowUtils object bound to the parent window and so the resulting accumulated rectangle is the wrong size. I think we can work around this by using SpecialPowers.getDOMWindowUtils. Patches forthcoming for this and a few other tweaks to paint_listener.js

We can't just use SpecialPowers.DOMWindowUtils since otherwise we end up with a DOMWindowUtils object that is bound to the parent object.

And also simplify the code somewhat

This patch adds waitForAllPaints which does *not* call getBoundingClientRect since that can cause a flush. Sometimes this flush is undesirable since it can mask bugs in the code under test which should be performing this flush itself.

Currently running through tryserver: https://tbpl.mozilla.org/?tree=Try&rev=4a1be3925ab9

(In reply to Brian Birtles (:birtles) from comment #5) > Currently running through tryserver: > https://tbpl.mozilla.org/?tree=Try&rev=4a1be3925ab9 Looks like I broke a bunch of stuff.

Try again: https://tbpl.mozilla.org/?tree=Try&rev=a18a6e2deac2

Comment on attachment 8385929 [details] [diff] [review] part 1 - Wrap up paint_listener.js so it doesn't leak globals Review of attachment 8385929 [details] [diff] [review]: ----------------------------------------------------------------- I don't know enough about JS to know why we need to do this.

Comment on attachment 8385932 [details] [diff] [review] part 4 - Add a non-flushing version of waitForAllPaints to paint_listener.js Review of attachment 8385932 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/tests/SimpleTest/paint_listener.js @@ +27,5 @@ > + function waitForPaints(callback, subdoc, flushMode) { > + // The call to getBoundingClientRect will flush pending layout > + // notifications. Sometimes, however, this is undesirable since it can mask > + // bugs where the code under test should be performing the flush. > + if (flushMode == "flush") { String comparison is a bit sad, can we just use bool instead?

(In reply to Matt Woodrow (:mattwoodrow) from comment #13) > Comment on attachment 8385932 [details] [diff] [review] > part 4 - Add a non-flushing version of waitForAllPaints to paint_listener.js > > Review of attachment 8385932 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/mochitest/tests/SimpleTest/paint_listener.js > @@ +27,5 @@ > > + function waitForPaints(callback, subdoc, flushMode) { > > + // The call to getBoundingClientRect will flush pending layout > > + // notifications. Sometimes, however, this is undesirable since it can mask > > + // bugs where the code under test should be performing the flush. > > + if (flushMode == "flush") { > > String comparison is a bit sad, can we just use bool instead? The problem is that from the call site you see: waitForPaints(function() { ... }, frame, true); And it's really not clear what 'true' means. I thought we tried to avoid that. What's the concern with string comparisons? Performance? Possible typos?

Here is part 1 without the whitespace changes

(In reply to Brian Birtles (:birtles) from comment #14) > > The problem is that from the call site you see: > > waitForPaints(function() { ... }, frame, true); > > And it's really not clear what 'true' means. I thought we tried to avoid > that. > > What's the concern with string comparisons? Performance? Possible typos? Yeah, good point about the callsites. The problem is both of those things, plus we're fundamentally testing a boolean condition 'do we want to flush', and representing that as a string is weird. How about using named constants/ enums instead? That would be the preferred approach for c++. Or you can ask someone else who knows more about JS for their thoughts :)

Address review feedback

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c793fa8b589 https://hg.mozilla.org/integration/mozilla-inbound/rev/bb870818480e https://hg.mozilla.org/integration/mozilla-inbound/rev/b0d218561219 https://hg.mozilla.org/integration/mozilla-inbound/rev/2d0080a92365

https://hg.mozilla.org/mozilla-central/rev/6c793fa8b589 https://hg.mozilla.org/mozilla-central/rev/bb870818480e https://hg.mozilla.org/mozilla-central/rev/b0d218561219 https://hg.mozilla.org/mozilla-central/rev/2d0080a92365