463384 - "Tabbed mode" is an unnecessary complication

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Dave Townsend [:mossop]]

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.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 1

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.

Comment 3

[Mike Connor [: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?

Comment 4

[John P Baker]

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

Comment 5

[Dave Townsend [:mossop]]

(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

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 2

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?

Comment 7

[Dave Townsend [:mossop]]

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.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Adds some notes that these bits are going away in the future.

Comment 10

[Dave Townsend [:mossop]]

Pushed. Keeping an eye on perf numbers.

http://hg.mozilla.org/mozilla-central/rev/4b5e00c182be

Comment 11

[Dave Townsend [:mossop]]

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.

Comment 12

[Dave Townsend [:mossop]]

We don't need to block on this now.

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch v4

Comment 14

[Asaf Romano (gone)]

Comment on attachment 491348 [details] [diff] [review]
patch v4

r=mano

Comment 15

[Dão Gottwald [:dao]]

Attachment: patch v4, updated to tip

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/projects/cedar/rev/1d50cf645e9a

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1d50cf645e9a

Comment 18

[Dão Gottwald [:dao]]

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

Apparently enterTabbedMode wasn't documented.