Closed Bug 905695 Opened 7 years ago Closed 6 years ago

Skip checking for tab overflows if there is only one tab open

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
Firefox 28

People

(Reporter: jaws, Assigned: jaws)

References

Details

(Whiteboard: [Australis:M8])

Attachments

(1 file)

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.
Attached patch PatchSplinter Review
Attachment #790871 - Flags: review?(mnoorenberghe+bmo)
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+
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
(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
(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.
(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.
Depends on: 904109, 897160
(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.
(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?
https://hg.mozilla.org/mozilla-central/rev/7e0adf1211a7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8]
Target Milestone: --- → Firefox 28
Depends on: 980043
Backed out:

https://hg.mozilla.org/mozilla-central/rev/e017c15325ae
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Is this effectively wontfix now?
Flags: needinfo?(jaws)
Yeah
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(jaws)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.