1290913 - Prevent flushing the layout when dimmed highlight background is constructed

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Mike de Boer [:mikedeboer]]

FinderHighlighter#_getWindowDimensions() causes a layout flush, because it accesses window properties that inherently cause that to happen: innerWidth, innerHeight, scrollMaxX, scrollMaxY, scrollMinX, scrollMinY.

We can accomplish this by making smart use of `nsIDOMWindowUtils::GetBoundsWithoutFlushing()`.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1290913 - prevent flushing the layout when dimmed highlight background is constructed.

Review commit: https://reviewboard.mozilla.org/r/68312/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68312/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8776595 [details]
Bug 1290913 - prevent flushing the layout when dimmed highlight background is constructed.

https://reviewboard.mozilla.org/r/68312/#review65408

::: toolkit/modules/FinderHighlighter.jsm:489
(Diff revision 1)
>     *                  properties
>     */
>    _getWindowDimensions(window) {
> -    let width = window.innerWidth + window.scrollMaxX - window.scrollMinX;
> -    let height = window.innerHeight + window.scrollMaxY - window.scrollMinY;
> +    // First we'll try without flushing layout, because it's way faster.
> +    let dwu = this._getDWU(window);
> +    let {width, height} = dwu.getBoundsWithoutFlushing(window.document.body);

Have you tested this code with different zoom factors? http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/Graphs.js#939

Pleae file a bug if it doesn't work as you expect since it looks like Graphs.js doesn't mention an associated bug for this behavior.

::: toolkit/modules/FinderHighlighter.jsm:491
(Diff revision 1)
> +    if (!width || !height) {
> +      // We need a flush after all :'(
> +      width = window.innerWidth + window.scrollMaxX - window.scrollMinX;

Looking through the codebase, I don't see any code that interprets a 0-width or 0-height response as layout needing to be flushed. Some code never checks the response, and other does look for 0-width or 0-height and treats the node as invisible.

Can you look at how getBoundsWithoutFlushing is implementing? And if it is possible for it to return a stale value if it is currently in the process of recalculating layout?

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Have you tested this code with different zoom factors?
> http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/
> Graphs.js#939
> 
> Pleae file a bug if it doesn't work as you expect since it looks like
> Graphs.js doesn't mention an associated bug for this behavior.

I just did and I'm unhappy to say that Graphs.js got it wrong; when you zoom the page, it does indeed cause a reflow which changes the layout state of the page. It does invalidate, but not necessarily flush layout to force re-querying the rects.
Since `nsIDOMWindowUtils::getBoundsWithoutFlushing()` explicitly does not flush, it'll get rects back that add up to 0 on the axes that were invalidated by the reflow. Since I don't want to work with zeroes, I deem it unlikely that the result is correct and go down the path that _forces_ a flush by accessing the relevant properties on the window object.
I'd actually like to advocate this flow to be used by devtools as well, including the highlighters, as it might save a few milliseconds here and there.

> Looking through the codebase, I don't see any code that interprets a 0-width
> or 0-height response as layout needing to be flushed. Some code never checks
> the response, and other does look for 0-width or 0-height and treats the
> node as invisible.

Not checking the response is a bit silly, because you can't trust a 0-response from a call that doesn't flush layout, except for 'top' and 'left' properties. Therefore treating the node as invisible when height or width are 0 using this method is also not correct, because of, well, the same story as above.

> Can you look at how getBoundsWithoutFlushing is implementing? And if it is
> possible for it to return a stale value if it is currently in the process of
> recalculating layout?

In other words, I think we're already good when I treat 0-responses as fishy and good enough reason to request the rects using a 'more reliable' method.

Tim, since you introduced `nsIDOMWindowUtils::getBoundsWithoutFlushing()`, can you corroborate this story of mine? Or tell me I'm dead wrong? ;-)

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Not checking the response is a bit silly, because you can't trust a
> 0-response from a call that doesn't flush layout, except for 'top' and
> 'left' properties.

Silly in the context of this bug, because we're working with _content_ that can change due to so many variables out of our control.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> In other words, I think we're already good when I treat 0-responses as fishy
> and good enough reason to request the rects using a 'more reliable' method.
> 
> Tim, since you introduced `nsIDOMWindowUtils::getBoundsWithoutFlushing()`,
> can you corroborate this story of mine? Or tell me I'm dead wrong? ;-)

Querying bounds without flushing is of course always a little fishy as the values might be wrong. I *think* I introduced it to prevent us from flushing the layout while opening a new tab. It wasn't introduced because it was very visible to users but rather because it was hitting the "open a new tab and check for reflows" test, whatever that's called.

So I think that probably we shouldn't use this method unless we can live with wrong values. Window sizes in sessionstore are somewhat less critical and we can correct wrong ones more easily.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Tim Taubert [:ttaubert] from comment #5)
> So I think that probably we shouldn't use this method unless we can live
> with wrong values. Window sizes in sessionstore are somewhat less critical
> and we can correct wrong ones more easily.

Thanks Tim! In this case we can live with wrong values and when we've got a suspicion that they are (in which case '0' is reported back), we'll query using the more expensive will-flush method.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8776595 [details]
Bug 1290913 - prevent flushing the layout when dimmed highlight background is constructed.

https://reviewboard.mozilla.org/r/68312/#review67316

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Okay, r=me

Comment 9

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/69c852865f06248fdf0b12180e5cdb07c9b6a6ff
Bug 1290913 - prevent flushing the layout when dimmed highlight background is constructed. r=jaws

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69c852865f06