1358721 - 5.05ms uninterruptible reflow at adjustTabstrip@chrome://browser/content/tabbrowser.xml:5949:24

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Brian Carpenter [:geeknik]]

Here's the stack:

adjustTabstrip@chrome://browser/content/tabbrowser.xml:5949:24
_handleNewTab@chrome://browser/content/tabbrowser.xml:6420:11
addTab/<@chrome://browser/content/tabbrowser.xml:2301:17
setTimeout handler*addTab@chrome://browser/content/tabbrowser.xml:2300:15
loadOneTab@chrome://browser/content/tabbrowser.xml:1522:23
enter@resource:///modules/CustomizeMode.jsm:219:19
toggle@resource:///modules/CustomizeMode.jsm:159:7
oncommand@chrome://browser/content/browser.xul:1:1

Comment 1

[Florian Quèze [:florian]]

We improved adjustTabstrip to not cause a sync flush when there are overflowing tabs in bug 1356655, but I guess this is a stack for a case without overflow.

Comment 3

[Dão Gottwald [:dao]]

Attachment: Bug 1358721 - Avoid flushing layout in adjustTabstrip.

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

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8872119 [details]
Bug 1358721 - Avoid flushing layout in adjustTabstrip.

https://reviewboard.mozilla.org/r/143612/#review147550

::: browser/base/content/tabbrowser.xml:6045
(Diff revision 1)
>  
> -          let numTabs = this.childNodes.length -
> -                        this.tabbrowser._removingTabs.length;
> -          if (numTabs > 2) {
> -            // This is an optimization to avoid layout flushes by calling
> -            // getBoundingClientRect() when we just opened a second tab. In
> +          // Wait until after the next paint to get current layout data without
> +          // flushing layout. Use nested requestAnimationFrame rather than
> +          // MozAfterPaint to invalidate layout as late as possible.
> +          window.requestAnimationFrame(() => {
> +            window.requestAnimationFrame(() => {

Same question as I had in bug 1368208: why is this 2 nested requestAnimationFrame rather than a requestAnimationFrame in a MozAfterPaint listener?

Comment 5

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)

> > +          // Wait until after the next paint to get current layout data without
> > +          // flushing layout. Use nested requestAnimationFrame rather than
> > +          // MozAfterPaint to invalidate layout as late as possible.
> > +          window.requestAnimationFrame(() => {
> > +            window.requestAnimationFrame(() => {
> 
> Same question as I had in bug 1368208: why is this 2 nested
> requestAnimationFrame rather than a requestAnimationFrame in a MozAfterPaint
> listener?

Although the patch here does no flushing inside the requestAnimationFrame callback, so I'm fine with the code; just confused by the comment.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8872119 [details]
Bug 1358721 - Avoid flushing layout in adjustTabstrip.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143612/diff/1-2/

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8872119 [details]
Bug 1358721 - Avoid flushing layout in adjustTabstrip.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143612/diff/2-3/

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8872119 [details]
Bug 1358721 - Avoid flushing layout in adjustTabstrip.

Forwarding the review request to Mike who has more expertise here than I do.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8872119 [details]
Bug 1358721 - Avoid flushing layout in adjustTabstrip.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143612/diff/3-4/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8872119 [details]
Bug 1358721 - Avoid flushing layout in adjustTabstrip.

https://reviewboard.mozilla.org/r/143612/#review147654

::: browser/base/content/tabbrowser.xml:6047
(Diff revision 4)
> -            let tab = this.tabbrowser.visibleTabs[this.tabbrowser._numPinnedTabs];
> +              if (this.getAttribute("overflow") == "true") {
> -            if (tab && tab.getBoundingClientRect().width <= this.mTabClipWidth) {
> -              this.setAttribute("closebuttons", "activetab");
> +                this.setAttribute("closebuttons", "activetab");
> -              return;
> +                return;
> -            }
> +              }

Hm. This is the second time I've seen this pattern where we repeat ourselves after a double-nested rAF for something like this. I understand the need - I wonder if we could find some kind of general solution. Like, it's a shame we've had to put the same "deal with overflow" code in two places.

Perhaps it's worth having a comment near lines 6034-6037 referring to this block - "if you change this, change that", etc. At your discretion.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8872119 [details]
Bug 1358721 - Avoid flushing layout in adjustTabstrip.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143612/diff/4-5/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84a99d06f8db
Avoid flushing layout in adjustTabstrip. r=mconley

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84a99d06f8db