Last Comment Bug 722586 - Correctly update active/inactive browsers
: Correctly update active/inactive browsers
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 13
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-30 17:35 PST by :Margaret Leibovic
Modified: 2012-02-07 05:10 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
patch (4.08 KB, patch)
2012-01-30 17:35 PST, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch v2 (4.17 KB, patch)
2012-01-31 09:36 PST, :Margaret Leibovic
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description :Margaret Leibovic 2012-01-30 17:35:09 PST
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)
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-30 18:13:07 PST
(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 2012-01-31 09:27:54 PST
(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
Comment 3 :Margaret Leibovic 2012-01-31 09:36:48 PST
Created attachment 593135 [details] [diff] [review]
patch v2

Taking Gavin's comment into account. I decided to do it tabbrowser style.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-31 14:01:48 PST
Comment on attachment 593135 [details] [diff] [review]
patch v2

Good catch!
Comment 6 Ed Morley [:emorley] 2012-02-01 11:04:22 PST
https://hg.mozilla.org/mozilla-central/rev/9730ad2d5d06
Comment 7 :Margaret Leibovic 2012-02-01 11:42:54 PST
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] 2012-02-02 06:43:30 PST
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) 2012-02-02 10:27:02 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/15d8f3c6072d
Comment 10 Brad Lassey [:blassey] (use needinfo?) 2012-02-06 18:42:34 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/0794dc567735
Comment 11 Cristian Nicolae (:xti) 2012-02-07 05:10:06 PST
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

Note You need to log in before you can comment on or make changes to this bug.