Closed
Bug 905695
Opened 11 years ago
Closed 10 years ago
Skip checking for tab overflows if there is only one tab open
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
WONTFIX
Firefox 28
People
(Reporter: jaws, Assigned: jaws)
References
Details
(Whiteboard: [Australis:M8])
Attachments
(1 file)
2.30 KB,
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
If only one tab is opened and we show the left/right arrows, we are actually removing quite a bit of space that could have been used to show the tab. Scrolling the tabbox in this state is also quite useless, since all the user can do is scroll to see the other parts of the *only* tab. If we make this change, we can skip a synchronous reflow for new windows that only have one tab.
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #790871 -
Flags: review?(mnoorenberghe+bmo)
Assignee | ||
Comment 3•11 years ago
|
||
Pushed to tryserver along with the patch for bug 897160: https://tbpl.mozilla.org/?tree=Try&rev=fbc5556db651 Baseline: https://tbpl.mozilla.org/?tree=Try&rev=af2fd4e37d20
Comment 4•11 years ago
|
||
Comment on attachment 790871 [details] [diff] [review] Patch Review of attachment 790871 [details] [diff] [review]: ----------------------------------------------------------------- Let's wait for more data to come in before landing this. The current numbers show a regression on XP which aligns with what mconley and I saw for bug 899867 attachment 790091 [details] [diff] [review]. It seems like the reflow work just gets shifted later and is possibly needs to be more thorough (taking more time) by that point. ::: browser/base/content/tabbrowser.xml @@ +3177,2 @@ > > var tabs = document.getBindingParent(this); Hoist this line up and use tabs.tabbrowser to remove the tabbrowser declaration.
Attachment #790871 -
Flags: review?(mnoorenberghe+bmo) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Three patches are needed in coordination but we get a .83ms win on XP tpaint, and a 2ms win on Win8 tpaint. See http://compare-talos.mattn.ca/?oldRevs=af2fd4e37d20&newRev=10b86fc9d7cf&submit=true The three patches are found in this try push, https://tbpl.mozilla.org/?tree=Try&rev=10b86fc9d7cf
Comment 6•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #5) > Three patches are needed in coordination… Can you explain how they work together? For all I can tell it could be one perf win while the others are regressions and neutral. Data from comment 3 which only has two of the three patches shows a regression on tpaint for Ubuntu and Windows (OS X is not ready yet): http://compare-talos.mattn.ca/?oldRevs=af2fd4e37d20&newRev=fbc5556db651&submit=true
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #6) > (In reply to Jared Wein [:jaws] from comment #5) > > Three patches are needed in coordination… > > Can you explain how they work together? The resize code in handleEvent calls TabsInTitlebar.updateAppearance, which could cause a reflow, and then immediately after that it would have called this.mTabStrip.boxObject.width, which would cause another reflow if the updateAppearance one is removed. In other words: Bug 904109 fix the TabsInTitlebar.updateApperance call. Bug 897160 and bug 905695 fix the mTabStrip.boxObject call.
Comment 8•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #7) > (In reply to Matthew N. [:MattN] from comment #6) > > (In reply to Jared Wein [:jaws] from comment #5) > > > Three patches are needed in coordination… > > > > Can you explain how they work together? > > The resize code in handleEvent calls TabsInTitlebar.updateAppearance, which > could cause a reflow, and then immediately after that it would have called > this.mTabStrip.boxObject.width, which would cause another reflow if the > updateAppearance one is removed. > > In other words: > Bug 904109 fix the TabsInTitlebar.updateApperance call. > Bug 897160 and bug 905695 fix the mTabStrip.boxObject call. Accessing mTabStrip.boxObject.width just flushes layout, which needs to happen sooner or later anyway; it's only wasteful if layout is invalidated between this point in time and when layout would be flushed next. Also, what does bug 897160 have to do with this bug? If there's a real dependency, then this seems pretty obscure and fragile. Bug 904109 also seems pretty fragile by nature.
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #8) > Also, what does bug 897160 have to do with this bug? If there's a real > dependency, then this seems pretty obscure and fragile. Not a real dependency to say, it just makes the UX of it better. > Bug 904109 also seems pretty fragile by nature. I agree. I have no issue with pushing forward with bug 897218 instead, but we will need to do one of the two.
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/projects/ux/rev/7e0adf1211a7
Whiteboard: [Australis:M8][fixed-in-ux]
Comment 11•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #9) > (In reply to Dão Gottwald [:dao] from comment #8) > > Also, what does bug 897160 have to do with this bug? If there's a real > > dependency, then this seems pretty obscure and fragile. > > Not a real dependency to say, it just makes the UX of it better. How exactly?
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7e0adf1211a7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8]
Target Milestone: --- → Firefox 28
Comment 13•10 years ago
|
||
Backed out: https://hg.mozilla.org/mozilla-central/rev/e017c15325ae
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 15•10 years ago
|
||
Yeah
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Flags: needinfo?(jaws)
Resolution: --- → WONTFIX
Assignee | ||
Comment 16•10 years ago
|
||
See bug 980043 for the conversation.
You need to log in
before you can comment on or make changes to this bug.
Description
•