Closed Bug 463384 Opened 12 years ago Closed 10 years ago

"Tabbed mode" is an unnecessary complication

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 5

People

(Reporter: mossop, Assigned: dao)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

Since the Firefox 1 days tabbrowser has had two modes, "tabbed mode" and single browser mode. It was, as best as I can tell, used to slightly improve the performance of progress listeners in the case where there was only one tab.

We enter tabbed mode whenever there is more than one tab, the tab strip is visible or more than one progress listener is added. In effect we are almost never in tabbed mode anymore. Certainly the default configuration automatically enters tabbed mode as soon as you start up the browser.

By removing it we simplify the code with no real loss. I'm pretty sure I can spot bugs where people have been doing things that would upset non-tabbed mode too.
Reading this back I realise it doesn't specify that I'm actually removing the non-tabbed mode and just making us tabbed mode all the time.
Blocks: 463387
Attached patch patch rev 1 (obsolete) — Splinter Review
This is mostly simple code removal, all we need to do is hook up the progress and event listeners to the first tab in the constructor. It shouldn't impact perf because that already happens when we enterTabbedMode during startup anyway.

There are no tests because there shouldn't be any change in behaviour here, all existing browser chrome tests pass with this.
Attachment #346689 - Flags: review?(mconnor)
Status: NEW → ASSIGNED
Comment on attachment 346689 [details] [diff] [review]
patch rev 1

oy.  premature optimization?  I presume this doesn't affect Ts on tryserver?  If not yet tested, please do so, just to be sure?
Attachment #346689 - Flags: review?(mconnor) → review+
-      <field name="mTabbedMode">
-        false

I have no evidence that any extensions are using it but, depending on when this is planned for, it might be tempting to do

       <field name="mTabbedMode">
-        false
+        true
       </field>

and remove the field for Firefox.next
(In reply to comment #3)
> (From update of attachment 346689 [details] [diff] [review])
> oy.  premature optimization?  I presume this doesn't affect Ts on tryserver? 
> If not yet tested, please do so, just to be sure?

I don't really believe tryserver talos results are worthwhile, however I got 3 data points and they suggested this was either not a perf impact or a perf win
Attached patch patch rev 2 (obsolete) — Splinter Review
Yeah probably smart. This just re-adds the mTabbedMode and enterTabbedMode items to avoid any possible breakage of sensible extensions. Want to just sign off on that change Mike?
Attachment #346689 - Attachment is obsolete: true
Attachment #347767 - Flags: review?(mconnor)
Flagging as late-compat. While the latest patch should mean that any extension that behaves sensibly shouldn't see any fallout from this I guess it would be sensible to note it somewhere, assuming we take it for 3.1 of course.
Keywords: late-compat
Connor, given comment 7 can you move this to the top of your pile? I'd like to get all the late-compat stuff in for Beta 2.
Flags: blocking-firefox3.1+
Target Milestone: --- → Firefox 3.1b2
Attached patch patch rev 3 (obsolete) — Splinter Review
Adds some notes that these bits are going away in the future.
Attachment #347767 - Attachment is obsolete: true
Attachment #348615 - Flags: review?(mconnor)
Attachment #347767 - Flags: review?(mconnor)
Attachment #348615 - Flags: review?(mconnor) → review+
Whiteboard: [needs landing]
Pushed. Keeping an eye on perf numbers.

http://hg.mozilla.org/mozilla-central/rev/4b5e00c182be
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Depends on: 465883
Backed out per bug 465883. This breaks if gBrowser is resolved too early because the inner browser has yet to have an docshell so the attempt to add a progress listener in the constructor of tabbrowser fails.
No longer blocks: 463387
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: late-compat
We don't need to block on this now.
Flags: blocking-firefox3.1+ → blocking-firefox3.1-
Target Milestone: Firefox 3.1b2 → ---
Status: REOPENED → ASSIGNED
Status: ASSIGNED → NEW
Assignee: dtownsend → nobody
Assignee: nobody → dao
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #348615 - Attachment is obsolete: true
Attachment #491348 - Flags: review?(mano)
Attachment #491348 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/1d50cf645e9a
Status: NEW → RESOLVED
Closed: 12 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
I've removed the "tabbed mode" documentation from https://developer.mozilla.org/En/Listening_to_events

Apparently enterTabbedMode wasn't documented.
Target Milestone: --- → Firefox4.2
Target Milestone: Firefox5 → Firefox 5
You need to log in before you can comment on or make changes to this bug.