Closed Bug 736311 Opened 9 years ago Closed 9 years ago

Deactivate the selected tab when Fennec is running in the background

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- +

People

(Reporter: mbrubeck, Assigned: wesj)

References

Details

(Keywords: perf)

Attachments

(2 files, 1 obsolete file)

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.
We should do this to let website better handle power/cpu and for WebApp support
blocking-fennec1.0: --- → +
Assignee: nobody → wjohnston
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
Duplicate of this bug: 734908
Attached patch Patch (obsolete) — Splinter Review
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?
(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 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+
Attached patch PatchSplinter Review
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)
Attached file test page
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+
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/0bc32fccce4c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Blocks: 739729
Depends on: 749624
You need to log in before you can comment on or make changes to this bug.