979161 - paint_listener.js shouldn't require running as a chrome mochitest

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Use SpecialPowers.getDOMWindowUtils in 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.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Wrap up paint_listener.js so it doesn't leak globals

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Wrap long lines in paint_listener.js

And also simplify the code somewhat

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Add a non-flushing version of waitForAllPaints to paint_listener.js

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.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Wrap up paint_listener.js so it doesn't leak globals

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Wrap long lines in paint_listener.js

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Use SpecialPowers.getDOMWindowUtils in paint_listener.js

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Add a non-flushing version of waitForAllPaints to paint_listener.js

Comment 12

[Matt Woodrow (:mattwoodrow)]

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 13

[Matt Woodrow (:mattwoodrow)]

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?

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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?

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Wrap up paint_listener.js so it doesn't leak globals (omitting whitespace differences)

Here is part 1 without the whitespace changes

Comment 16

[Matt Woodrow (:mattwoodrow)]

(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 :)

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Add a non-flushing version of waitForAllPaints to paint_listener.js

Address review feedback

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 19

[Carsten Book [:Tomcat]]

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