"Tabbed mode" is an unnecessary complication

RESOLVED FIXED in Firefox 5

Status

()

defect
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: mossop, Assigned: dao)

Tracking

({dev-doc-complete})

Trunk
Firefox 5
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Reporter

Comment 1

11 years ago
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.
Reporter

Updated

11 years ago
Blocks: 463387
Reporter

Comment 2

11 years ago
Posted 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)
Reporter

Updated

11 years ago
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+

Comment 4

11 years ago
-      <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
Reporter

Comment 5

11 years ago
(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
Reporter

Comment 6

11 years ago
Posted 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)
Reporter

Comment 7

11 years ago
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
Reporter

Comment 9

11 years ago
Posted 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: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Reporter

Updated

11 years ago
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 → ---
Reporter

Updated

11 years ago
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
Reporter

Updated

11 years ago
Status: ASSIGNED → NEW
Reporter

Updated

9 years ago
Assignee: dtownsend → nobody
Assignee

Updated

9 years ago
Assignee: nobody → dao
Assignee

Comment 13

9 years ago
Posted patch patch v4 (obsolete) — Splinter Review
Attachment #348615 - Attachment is obsolete: true
Attachment #491348 - Flags: review?(mano)
Assignee

Comment 15

8 years ago
Attachment #491348 - Attachment is obsolete: true

Comment 17

8 years ago
http://hg.mozilla.org/mozilla-central/rev/1d50cf645e9a
Status: NEW → RESOLVED
Closed: 11 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Assignee

Comment 18

8 years ago
I've removed the "tabbed mode" documentation from https://developer.mozilla.org/En/Listening_to_events

Apparently enterTabbedMode wasn't documented.
Assignee

Updated

8 years ago
Target Milestone: --- → Firefox4.2
Assignee

Updated

8 years ago
Target Milestone: Firefox5 → Firefox 5
You need to log in before you can comment on or make changes to this bug.