Closed
Bug 736311
Opened 12 years ago
Closed 12 years ago
Deactivate the selected tab when Fennec is running in the background
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(blocking-fennec1.0 +)
RESOLVED
FIXED
Firefox 14
Tracking | Status | |
---|---|---|
blocking-fennec1.0 | --- | + |
People
(Reporter: mbrubeck, Assigned: wesj)
References
Details
(Keywords: perf)
Attachments
(2 files, 1 obsolete file)
2.26 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
729 bytes,
text/html
|
Details |
Fennec should set isActive=false for the docshell of the selected tab when the Fennec activity is in the background, and set isActive=true for the selected tab when Fennec returns to the foreground. This will reduce CPU/battery use when Fennec is in the background, for example by throttling timers. We implemented this for XUL Fennec 9 (bug 679923), but it was disabled in Fennec 10 (bug 712517) because of bugs that caused Fennec to mistakenly think it was in the background after the device was rotated or woken from sleep. So we need to make sure we don't reintroduce those bugs when implementing this in native Fennec.
Comment 1•12 years ago
|
||
We should do this to let website better handle power/cpu and for WebApp support
blocking-fennec1.0: --- → +
Updated•12 years ago
|
Assignee: nobody → wjohnston
Comment 2•12 years ago
|
||
What does docShell.isActive=false do, btw? When Fennec is in the background (after tapping the Android home button), these things should be suspended ideally: - Timers/javascript - Animated gif/png/ico images - Dom events - Audio/video - Flash plugins Does docShell.isActive do that?
Blocks: 735246
Assignee | ||
Comment 4•12 years ago
|
||
This is pretty much a copy paste from XUL Fennec. Everything seems fine on my phone. I see the events triggered in the right order. Turned my phone on and off. Rotated is. Opened and closed tabs. Sent Fennec to the background and back. Turn phone off, rotate and then turn back on.. etc. etc. But I can run with this for a few days and see if anything else pops up. Or I can throw a build up for QA to try on some other devices for a bit?
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #2) > What does docShell.isActive=false do, btw? > When Fennec is in the background (after tapping the Android home button), > these things should be suspended ideally: > - Timers/javascript > - Animated gif/png/ico images > - Dom events > - Audio/video > - Flash plugins > > Does docShell.isActive do that? AFAICT (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp), isActive mainly throttles timers, unlocks images so that their data can be thrown away on memory pressure, and hides plugins (we also pause them from snorp's comments I guess). I wonder if we need to fire a memory pressure notification to ensure we slim down when going into the background as well? I don't see anything about Audio/Video (that doesn't mean we don't do anything, I just don't see it here), and I'm not sure what you mean by DOM events.
Comment 6•12 years ago
|
||
Comment on attachment 607390 [details] [diff] [review] Patch For Android, I only see "application-background" and "application-foreground" notifications being fired. I'm not sure if those fire when the screen turns off or not. Would be interesting to see if it does. You could remove the other notifications for now, since they don't apply to Android. I could live with the setTimeout in Android too, just in case of races, but remove the "Maemo" comment. Use a test page to see if a webpage using setTimeout(..., 100) gets clamped to 1sec when we go in the background.
Attachment #607390 -
Flags: feedback+
Assignee | ||
Comment 7•12 years ago
|
||
AFAICT, this works fine. I've got a test page to make sure timers are throttled in the background that I'll toss up here.
Attachment #607390 -
Attachment is obsolete: true
Attachment #608139 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 8•12 years ago
|
||
Reporter | ||
Comment 9•12 years ago
|
||
Comment on attachment 608139 [details] [diff] [review] Patch Looks good but I have some possible nits... >+ ActivityObserver.init(); Do we need an uninit() function? (Does anyone care if we leak observers at shutdown? I have no idea.) >+ if (BrowserApp.selectedTab.getActive() != isForeground) { >+ setTimeout(function() { >+ BrowserApp.selectedTab.setActive(isForeground); >+ }, 0); >+ } If the timeout is necessary here, you should move the "if" statement inside of the timeout. That'll prevent a (highly-unlikely but theoretically possible) bug if this code gets called a second time with a different values before the first timeout fires. (If the timeout is not necessary, we should remove it.) Have you tested that this also throttles timers when the screen is locked? I think it will but I'm not certain.
Attachment #608139 -
Flags: review?(mbrubeck) → review+
Comment 10•12 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #5) > I don't see anything about Audio/Video (that doesn't mean we don't do > anything, I just don't see it here), and I'm not sure what you mean by DOM > events. I just meant any event that could be firing (onload, onunload, etc). I filed bug 738254 about Audio/Video running in background tabs.
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bc32fccce4c > Do we need an uninit() function? (Does anyone care if we leak observers at > shutdown? I have no idea.) Me either. But Desktop is trying to remove unnecessary crap at shutdown, so I feel good about this. > (If the timeout is not necessary, we should remove it.) Timeout is from Maemo, so I just removed it.
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0bc32fccce4c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•