Closed Bug 463384 Opened 13 years ago Closed 11 years ago
"Tabbed mode" is an unnecessary complication
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.
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)
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
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?
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.
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.
Target Milestone: --- → Firefox 3.1b2
Adds some notes that these bits are going away in the future.
Attachment #348615 - Flags: review?(mconnor) → review+
Pushed. Keeping an eye on perf numbers. http://hg.mozilla.org/mozilla-central/rev/4b5e00c182be
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
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 → ---
We don't need to block on this now.
Flags: blocking-firefox3.1+ → blocking-firefox3.1-
Target Milestone: Firefox 3.1b2 → ---
Comment on attachment 491348 [details] [diff] [review] patch v4 r=mano
Attachment #491348 - Flags: review?(mano) → review+
Status: NEW → RESOLVED
Closed: 13 years ago → 11 years ago
Resolution: --- → FIXED
I've removed the "tabbed mode" documentation from https://developer.mozilla.org/En/Listening_to_events Apparently enterTabbedMode wasn't documented.
You need to log in before you can comment on or make changes to this bug.