Correctly update active/inactive browsers

VERIFIED FIXED in Firefox 11

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 13
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, firefox13 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 592938 [details] [diff] [review]
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)
Attachment #592938 - Flags: review?(mark.finkle)
(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!
(Assignee)

Comment 2

6 years ago
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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
(Assignee)

Comment 3

6 years ago
Created attachment 593135 [details] [diff] [review]
patch v2

Taking Gavin's comment into account. I decided to do it tabbrowser style.
Attachment #592938 - Attachment is obsolete: true
Attachment #592938 - Flags: review?(mark.finkle)
Attachment #593135 - Flags: review?(mark.finkle)
Comment on attachment 593135 [details] [diff] [review]
patch v2

Good catch!
Attachment #593135 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 5

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

Comment 6

6 years ago
https://hg.mozilla.org/mozilla-central/rev/9730ad2d5d06
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
(Assignee)

Comment 7

6 years ago
Comment on attachment 593135 [details] [diff] [review]
patch v2

[Approval Request Comment]
Performance win that missed getting ported over from XUL fennec. Low-risk.
Attachment #593135 - Flags: approval-mozilla-beta?
Attachment #593135 - Flags: approval-mozilla-aurora?

Comment 8

6 years ago
Comment on attachment 593135 [details] [diff] [review]
patch v2

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.
Attachment #593135 - Flags: approval-mozilla-beta?
Attachment #593135 - Flags: approval-mozilla-beta+
Attachment #593135 - Flags: approval-mozilla-aurora?
Attachment #593135 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/15d8f3c6072d
status-firefox11: --- → affected
status-firefox12: --- → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/0794dc567735
status-firefox11: affected → fixed
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
Status: RESOLVED → VERIFIED
status-firefox13: --- → fixed
You need to log in before you can comment on or make changes to this bug.