Closed
Bug 752376
Opened 11 years ago
Closed 10 years ago
Avoid calling scrollbox.ensureElementIsVisible() if the tab strip doesn't overflow to prevent layout flushes
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 19
People
(Reporter: ttaubert, Assigned: dao)
References
(Blocks 1 open bug)
Details
(Keywords: perf, Whiteboard: [Snappy:p2])
Attachments
(1 file, 5 obsolete files)
4.21 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #621476 -
Flags: review?(dao)
Assignee | ||
Comment 1•11 years ago
|
||
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?
Attachment #621476 -
Flags: review?(dao) → review-
Reporter | ||
Comment 2•11 years ago
|
||
(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.
Assignee: nobody → ttaubert
Attachment #621476 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #621554 -
Flags: review?(dao)
Assignee | ||
Comment 3•11 years ago
|
||
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?
Reporter | ||
Comment 4•11 years ago
|
||
(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?
Reporter | ||
Comment 5•11 years ago
|
||
(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.
Attachment #621554 -
Attachment is obsolete: true
Attachment #621554 -
Flags: review?(dao)
Attachment #621561 -
Flags: review?(dao)
Reporter | ||
Updated•11 years ago
|
Component: Tabbed Browser → XUL Widgets
Product: Firefox → Toolkit
QA Contact: tabbed.browser → xul.widgets
Reporter | ||
Updated•11 years ago
|
Summary: Prevent unnecessary layout flushes when switching tabs → Make scrollbox.ensureElementIsVisible() a no-op if it doesn't overflow to prevent layout flushes
Assignee | ||
Updated•11 years ago
|
Attachment #621561 -
Flags: review?(dao) → review+
![]() |
||
Comment 6•11 years ago
|
||
> 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•11 years ago
|
||
I guess the question is whether the test in comment 4 was performed while doing a tab switch, or in some other way.
Reporter | ||
Comment 8•11 years ago
|
||
No, I called scrollbox.ensureElementIsVisible() right away, didn't know about the short-circuit.
Reporter | ||
Comment 9•11 years ago
|
||
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•11 years ago
|
||
Well, you could do ensureElementIsVisible() off a refresh tick instead of synchronously. that might help a good bit.
Reporter | ||
Updated•11 years ago
|
Attachment #621561 -
Attachment is obsolete: true
Reporter | ||
Comment 11•11 years ago
|
||
Will not work on this anytime soon.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Comment 12•11 years ago
|
||
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•11 years ago
|
||
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•11 years ago
|
||
Attachment #646559 -
Attachment is obsolete: true
Attachment #646610 -
Flags: review?(neil)
Assignee | ||
Comment 15•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Component: XUL Widgets → Tabbed Browser
Product: Toolkit → Firefox
Summary: Make scrollbox.ensureElementIsVisible() a no-op if it doesn't overflow to prevent layout flushes → Avoid calling scrollbox.ensureElementIsVisible() if the tab strip doesn't overflow to prevent layout flushes
Assignee | ||
Comment 20•10 years ago
|
||
(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.
Attachment #674636 -
Flags: review?(enndeakin)
Comment 21•10 years ago
|
||
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
Assignee | ||
Comment 22•10 years ago
|
||
(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•10 years ago
|
||
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.
Attachment #674636 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9379c0915c6a
Assignee: enndeakin → dao
Assignee | ||
Updated•10 years ago
|
Attachment #646610 -
Attachment is obsolete: true
Attachment #646610 -
Flags: review?(neil)
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9379c0915c6a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in
before you can comment on or make changes to this bug.
Description
•