Last Comment Bug 752376 - Avoid calling scrollbox.ensureElementIsVisible() if the tab strip doesn't overflow to prevent layout flushes
: Avoid calling scrollbox.ensureElementIsVisible() if the tab strip doesn't ove...
Status: RESOLVED FIXED
[Snappy:p2]
: perf
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 19
Assigned To: Dão Gottwald [:dao]
:
Mentors:
: 789575 804948 (view as bug list)
Depends on:
Blocks: tabswitch
  Show dependency treegraph
 
Reported: 2012-05-06 16:41 PDT by Tim Taubert [:ttaubert]
Modified: 2012-10-25 18:29 PDT (History)
14 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (1.58 KB, patch)
2012-05-06 16:41 PDT, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Splinter Review
patch v2 (2.34 KB, patch)
2012-05-07 04:33 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v3 (1.65 KB, patch)
2012-05-07 05:01 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review
Patch (4.91 KB, patch)
2012-07-27 06:47 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
Patch, addressing comments and add a test line (5.61 KB, patch)
2012-07-27 09:49 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
another patch (4.21 KB, patch)
2012-10-24 05:12 PDT, Dão Gottwald [:dao]
enndeakin: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-05-06 16:41:38 PDT
Created attachment 621476 [details] [diff] [review]
patch v1

Boris did some profiling for tab switching:

"Layout flushes(break in PresShell::FlushPendingNotifications) from getBoundingClientRect, via get_scrollClientRect called from ensureElementIsVisible called from _handleTabSelect called from onxblTabSelect called from updateCurrentBrowser called from onselect called from set_selectedIndex etc.  This is about 15% and includes restyling, reflow, plugin geometry updates(!!)."

While we could implement something like a ensureElementIsVisible() cache in nsScrollBoxObject we should just not call tabstrip.ensureElementIsVisible(tab) if the tabbar doesn't overflow. This should probably be the case for the majority of users most of their browsing time and give us a little improvement.
Comment 1 Dão Gottwald [:dao] 2012-05-06 16:47:23 PDT
Comment on attachment 621476 [details] [diff] [review]
patch v1

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -1994,17 +1994,18 @@
>         <![CDATA[
>           Array.forEach(this.tabs, function(tab) {
>             if (aTabs.indexOf(tab) == -1)
>               this.hideTab(tab);
>             else
>               this.showTab(tab);
>           }, this);
> 
>-          this.tabContainer.mTabstrip.ensureElementIsVisible(this.selectedTab, false);
>+          if (this.getAttribute("overflow") == "true")
>+            this.tabContainer.mTabstrip.ensureElementIsVisible(this.selectedTab, false);

You're checking the attribute on the wrong element here.
It actually looks like _handleTabSelect should just be called here.

I assume you didn't make ensureElementIsVisible return early because it can't easily tell whether it's overflowing?
Comment 2 Tim Taubert [:ttaubert] 2012-05-07 04:33:22 PDT
Created attachment 621554 [details] [diff] [review]
patch v2

(In reply to Dão Gottwald [:dao] from comment #1)
> You're checking the attribute on the wrong element here.
> It actually looks like _handleTabSelect should just be called here.

Fixed.

> I assume you didn't make ensureElementIsVisible return early because it
> can't easily tell whether it's overflowing?

Didn't think of that. Added it to the scrollbox widget.
Comment 3 Dão Gottwald [:dao] 2012-05-07 04:38:05 PDT
Comment on attachment 621554 [details] [diff] [review]
patch v2

>         <![CDATA[
>           Array.forEach(this.tabs, function(tab) {
>             if (aTabs.indexOf(tab) == -1)
>               this.hideTab(tab);
>             else
>               this.showTab(tab);
>           }, this);
> 
>-          this.tabContainer.mTabstrip.ensureElementIsVisible(this.selectedTab, false);
>+          this.tabContainer._handleTabSelect();

I just realized that the old code disabled smooth scrolling. I'm not sure if we care about this.

>+      <property name="_isOverflowing" readonly="true">
>+        <getter><![CDATA[
>+          return (this.scrollPosition > 0 ||
>+                  this.scrollSize > this.scrollClientSize);
>+        ]]></getter>
>+      </property>

This doesn't flush layout?
Comment 4 Tim Taubert [:ttaubert] 2012-05-07 04:56:13 PDT
(In reply to Dão Gottwald [:dao] from comment #3)
> >-          this.tabContainer.mTabstrip.ensureElementIsVisible(this.selectedTab, false);
> >+          this.tabContainer._handleTabSelect();
> 
> I just realized that the old code disabled smooth scrolling. I'm not sure if
> we care about this.

So should we revert this? We don't really need to touch it for this bug anyway.

> >+      <property name="_isOverflowing" readonly="true">
> >+        <getter><![CDATA[
> >+          return (this.scrollPosition > 0 ||
> >+                  this.scrollSize > this.scrollClientSize);
> >+        ]]></getter>
> >+      </property>
> 
> This doesn't flush layout?

I set a breakpoint in PresShell::FlushPendingNotifications and the method wasn't called. So I'm pretty sure this doesn't flush layout but we should maybe ask bz if it does?
Comment 5 Tim Taubert [:ttaubert] 2012-05-07 05:01:58 PDT
Created attachment 621561 [details] [diff] [review]
patch v3

(In reply to Tim Taubert [:ttaubert] from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > >-          this.tabContainer.mTabstrip.ensureElementIsVisible(this.selectedTab, false);
> > >+          this.tabContainer._handleTabSelect();
> > 
> > I just realized that the old code disabled smooth scrolling. I'm not sure if
> > we care about this.
> 
> So should we revert this? We don't really need to touch it for this bug
> anyway.

Reverted this change.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-05-07 09:24:26 PDT
> So I'm pretty sure this doesn't flush layout but we should maybe ask bz if it does?

PresShell::FlushPendingNotifications is only reached if there are pending layout changes to flush.  Otherwise the flush attempt is short-circuited in nsDocument::FlushPendingNotifications.

In this case .scrollPosition will certainly try to flush layout.  So will .scrollSize.  So will .scrollClientSize.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-05-07 09:27:07 PDT
I guess the question is whether the test in comment 4 was performed while doing a tab switch, or in some other way.
Comment 8 Tim Taubert [:ttaubert] 2012-05-07 09:28:58 PDT
No, I called scrollbox.ensureElementIsVisible() right away, didn't know about the short-circuit.
Comment 9 Tim Taubert [:ttaubert] 2012-05-07 11:09:07 PDT
Sounds like there isn't an easy solution for this. We can't even go back to checking the overflow=true attribute because I suppose the overflow event might not have been thrown yet for any unprocessed style changes. This could cause us to not make the element visible when we actually should.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-05-07 11:29:03 PDT
Well, you could do ensureElementIsVisible() off a refresh tick instead of synchronously.  that might help a good bit.
Comment 11 Tim Taubert [:ttaubert] 2012-07-11 06:36:44 PDT
Will not work on this anytime soon.
Comment 12 Neil Deakin (away until Oct 3) 2012-07-27 06:47:45 PDT
Created attachment 646559 [details] [diff] [review]
Patch

This patch adds two non-flushing properties to scrollboxes to check if the scrollbox is overflowing. One of these is then used to skip calling ensureElementIsVisible if not.
Comment 13 Dão Gottwald [:dao] 2012-07-27 06:53:15 PDT
Comment on attachment 646559 [details] [diff] [review]
Patch

>+      <property name="overflowsHorizontally" readonly="true">
>+        <getter><![CDATA[
>+          return this.scrollBoxObject.overflowsHorizontally;
>+        ]]></getter>
>+      </property>
>+
>+      <property name="overflowsVertically" readonly="true">
>+        <getter><![CDATA[
>+          return this.scrollBoxObject.overflowsVertically;
>+        ]]></getter>
>+      </property>

how about:

      <property name="overflows" readonly="true">
        <getter><![CDATA[
          return this.orient == "vertical" ?
                 this.scrollBoxObject.overflowsVertically :
                 this.scrollBoxObject.overflowsHorizontally;
        ]]></getter>
      </property>
Comment 14 Neil Deakin (away until Oct 3) 2012-07-27 09:49:38 PDT
Created attachment 646610 [details] [diff] [review]
Patch, addressing comments and add a test line
Comment 15 Dão Gottwald [:dao] 2012-07-28 04:01:30 PDT
Comment on attachment 646610 [details] [diff] [review]
Patch, addressing comments and add a test line

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml

>       <method name="_handleTabSelect">
>         <body><![CDATA[
>-          this.mTabstrip.ensureElementIsVisible(this.selectedItem);
>+          if (this.mTabstrip.overflowsHorizontally) {
>+            this.mTabstrip.ensureElementIsVisible(this.selectedItem);
>+          }

s/overflowsHorizontally/overflows/

I assume you're not checking 'overflows' early in ensureElementIsVisible because doing so wouldn't be safe without flushing layout?
Comment 16 Neil Deakin (away until Oct 3) 2012-07-30 08:58:17 PDT
(In reply to Dão Gottwald [:dao] from comment #15)

> 
> I assume you're not checking 'overflows' early in ensureElementIsVisible
> because doing so wouldn't be safe without flushing layout?

I'm doing so as to not change the behaviour of ensureElementIsVisible, as the new overflow properties rely on unflushed state.
Comment 17 neil@parkwaycc.co.uk 2012-07-30 13:49:43 PDT
So, what happens in the case where we're adding a tab which causes the tabstrip to overflow? Do we know that it's overflowing as we try to switch to the newly added tab yet?
Comment 18 Dão Gottwald [:dao] 2012-10-24 01:52:25 PDT
*** Bug 804948 has been marked as a duplicate of this bug. ***
Comment 19 Dão Gottwald [:dao] 2012-10-24 01:55:05 PDT
*** Bug 789575 has been marked as a duplicate of this bug. ***
Comment 20 Dão Gottwald [:dao] 2012-10-24 05:12:38 PDT
Created attachment 674636 [details] [diff] [review]
another patch

(In reply to neil@parkwaycc.co.uk from comment #17)
> So, what happens in the case where we're adding a tab which causes the
> tabstrip to overflow? Do we know that it's overflowing as we try to switch
> to the newly added tab yet?

We need to call ensureElementIsVisible as soon as we know we're overflowing. We already do this via _positionPinnedTabs in the overflow handler, which is somewhat fragile. I made this more explicit.

I don't think we need to extend nsIScrollBoxObject for this.
Comment 21 Neil Deakin (away until Oct 3) 2012-10-25 05:44:38 PDT
Comment on attachment 674636 [details] [diff] [review]
another patch

>         var tabs = document.getBindingParent(this);
>         tabs.setAttribute("overflow", "true");
>         tabs._positionPinnedTabs();
>+        tabs._handleTabSelect(false);

Since _positionPinnedTabs can also call _handleTabSelect doesn't this mean that _handleTabSelect can be called twice?

>       <method name="_handleTabSelect">
>+        <parameter name="aSmoothScroll"/>
>         <body><![CDATA[
>-          this.mTabstrip.ensureElementIsVisible(this.selectedItem);
>+          if (this.getAttribute("overflow") == "true")
>+            this.mTabstrip.ensureElementIsVisible(this.selectedItem, aSmoothScroll);

_handleTabSelect is never called with aSmoothScroll = true
Comment 22 Dão Gottwald [:dao] 2012-10-25 06:14:23 PDT
(In reply to Neil Deakin from comment #21)
> Comment on attachment 674636 [details] [diff] [review]
> another patch
> 
> >         var tabs = document.getBindingParent(this);
> >         tabs.setAttribute("overflow", "true");
> >         tabs._positionPinnedTabs();
> >+        tabs._handleTabSelect(false);
> 
> Since _positionPinnedTabs can also call _handleTabSelect doesn't this mean
> that _handleTabSelect can be called twice?

The _lastNumPinned check protects against this.

> >       <method name="_handleTabSelect">
> >+        <parameter name="aSmoothScroll"/>
> >         <body><![CDATA[
> >-          this.mTabstrip.ensureElementIsVisible(this.selectedItem);
> >+          if (this.getAttribute("overflow") == "true")
> >+            this.mTabstrip.ensureElementIsVisible(this.selectedItem, aSmoothScroll);
> 
> _handleTabSelect is never called with aSmoothScroll = true

It's called with the parameter omitted, which is different from false; the default value is true.
Comment 23 Neil Deakin (away until Oct 3) 2012-10-25 06:22:32 PDT
Comment on attachment 674636 [details] [diff] [review]
another patch

> The _lastNumPinned check protects against this.

I see. From the "overflow" handler, the number of pinned tabs doesn't change.
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-10-25 18:29:04 PDT
https://hg.mozilla.org/mozilla-central/rev/9379c0915c6a

Note You need to log in before you can comment on or make changes to this bug.