722586 - Correctly update active/inactive browsers

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Attachment: patch

So, there are two main issues here here. First of all, we should be using type="content-targetable" instead of type="content" for our inactive browsers. That's what tabbrowser.xml does on desktop, and it's the preferred value according to MDN (I think there are security reasons behind this).

The second main issue is that we're never setting active = false anywhere right now. Since we're not reading Tab.active anywhere, and we always want to be setting the active browser and selected tab at the same time, I decided to just put BrowserApp in charge of setting the active browser when it sets selectedTab.

Let me know if there are good reasons why things are the way they are now, but I assume things are just messy because we've been changing lots of things.

(This is on top of my patch for bug 719868)

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to Margaret Leibovic [:margaret] from comment #0)
> The second main issue is that we're never setting active = false anywhere
> right now.

Hmm, does that mean you're not benefiting from the various fixes that check docShell.isActive, like bug 379535? That would be pretty bad!

Comment 2

[:Margaret Leibovic]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #1)
> (In reply to Margaret Leibovic [:margaret] from comment #0)
> > The second main issue is that we're never setting active = false anywhere
> > right now.
> 
> Hmm, does that mean you're not benefiting from the various fixes that check
> docShell.isActive, like bug 379535? That would be pretty bad!

Looks like we're not! It appears we were doing that for XUL fennec [1], but not in native fennec. XUL fennec was setting browser.docShell.isActive, but do we want to be setting browser.isDocShellActive instead, like tabbrowser.xml does [2]?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/bindings/browser.js#703
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#859

Comment 3

[:Margaret Leibovic]

Attachment: patch v2

Taking Gavin's comment into account. I decided to do it tabbrowser style.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 593135 [details] [diff] [review]
patch v2

Good catch!

Comment 5

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9730ad2d5d06

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9730ad2d5d06

Comment 7

[:Margaret Leibovic]

Comment on attachment 593135 [details] [diff] [review]
patch v2

[Approval Request Comment]
Performance win that missed getting ported over from XUL fennec. Low-risk.

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 593135 [details] [diff] [review]
patch v2

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/15d8f3c6072d

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0794dc567735

Comment 11

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-02-07)
20120207031136
http://hg.mozilla.org/mozilla-central/rev/b077059c575a
Device: HTC Desire Z
OS: Android 2.3.3

Firefox 12.0a2 (2012-02-06)
20120206042011
http://hg.mozilla.org/releases/mozilla-aurora/rev/9fb0c06ceb49
Device: HTC Desire Z
OS: Android 2.3.3

Firefox 11.0
20120206202409
http://hg.mozilla.org/releases/mozilla-beta/rev/1c0aba74d116
Device: HTC Desire Z
OS: Android 2.3.3