1271015 - Add tests for things not happening (optimizations) in response to media query changes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This adds a few basic tests for expectations of when we do and don't
restyle, construct frames, and reflow in response to changes of media
queries.  They don't give us a lot of coverage, but often the tiny bits
of coverage at the beginning are the most useful.

In general, I'd like us to have more tests for specific optimizations,
i.e., for specific things that we expect not to happen in certain cases.
The elementsRestyled, framesConstructed, and framesReflowed getters on
DOMWindowUtils are a good way to make such measurements for a number of
things in layout; that's why I added them.

(Inspired a bit by bug 1259641.)

MozReview-Commit-ID: JFtlPO1eyoD

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Add tests for things not happening (optimizations) in response to media query changes

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8749908 [details] [diff] [review]
patch 2 - Add tests for things not happening (optimizations) in response to media query changes

Oops, one of those members on DOMWindowUtils was only in a patch in my tree!  I'll attach it here as patch 1.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Add mechanism for testing the number of elements restyled

This is useful for writing tests that test particular optimizations,
such as that a particular operation doesn't cause restyles.  It sits
next to similar counters for frames constructed and frames reflowed.

I also snuck in a preference for the less-expensive mPresContext over
the more expensive mFrame->PresContext() (which dereferences multiple
pointers).

(Originally written for work I planned to be part of bug 1189598.)

MozReview-Commit-ID: 8GxcWsjB0aU

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8749939 [details] [diff] [review]
patch 1 - Add mechanism for testing the number of elements restyled

>+  MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome());

And I'll remove this, since the rest were removed in bug 1210294.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8749908 [details] [diff] [review]
patch 2 - Add tests for things not happening (optimizations) in response to media query changes

Review of attachment 8749908 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_media_queries_dynamic.html
@@ +170,5 @@
> +    // FIXME: We restyle here because
> +    // nsIPresShell::RestyleForCSSRuleChanges posts a restyle, but it's
> +    // probably avoidable if we wanted to avoid it.
> +    { restyle: true, construct: false, reflow: false });
> +

Maybe add another test in here where we resize and stay within the same matching @media rule, with { restyle: true, construct: false, reflow: true }?

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8749939 [details] [diff] [review]
patch 1 - Add mechanism for testing the number of elements restyled

Review of attachment 8749939 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1781,5 @@
>     */
>    void askPermission(in nsIContentPermissionRequest aRequest);
>  
>    /**
> +   * Number of elements restyled for the curent document.

I think RestyleManager::Restyle might get called for more than one frame for an element (e.g. it's called for each ib split sibling in ComputeStyleChangeFor).  So I guess the comment should be tweaked, and maybe the IDL attribute renamed, although I can't think of a better concise name.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #5)
> Maybe add another test in here where we resize and stay within the same
> matching @media rule, with { restyle: true, construct: false, reflow: true }?

I added one for height and one for width (one before the other resize and one after).  But it's with restyle: false, though.


And I adjusted the comment in nsIDOMWindowUtils.idl but kept the name.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c02ff079d45144e04587124f1d6ec3e407b6a11
Bug 1271015 patch 1 - Add mechanism for testing the number of elements restyled.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/048db7c7e4881e6da9fb5dd5742326485ced03de
Bug 1271015 patch 2 - Add tests for things not happening (optimizations) in response to media query changes.  r=heycam

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c02ff079d45
https://hg.mozilla.org/mozilla-central/rev/048db7c7e488

Comment 10

[Eric Shepherd [:sheppy]]

I just documented this for the hell of it.