All users were logged out of Bugzilla on October 13th, 2018

Prevent flushing the layout when dimmed highlight background is constructed

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla51
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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()`.
Flags: qe-verify-
Flags: firefox-backlog+
(Assignee)

Comment 1

2 years ago
Created attachment 8776595 [details]
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/
Attachment #8776595 - Flags: review?(jaws)
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?
Attachment #8776595 - Flags: review?(jaws) → review-
(Assignee)

Comment 3

2 years ago
(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? ;-)
Flags: needinfo?(ttaubert)
(Assignee)

Comment 4

2 years ago
(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.
(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.
Flags: needinfo?(ttaubert)
(Assignee)

Comment 6

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8776595 - Flags: review- → review?(jaws)

Comment 7

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

https://reviewboard.mozilla.org/r/68312/#review67316
Attachment #8776595 - Flags: review?(jaws) → review+
(Assignee)

Comment 9

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/69c852865f06
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

2 years ago
Depends on: 1296822

Updated

2 years ago
Blocks: 1306320
(Assignee)

Updated

2 years ago
No longer blocks: 1306320
You need to log in before you can comment on or make changes to this bug.