736311 - Deactivate the selected tab when Fennec is running in the background

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

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

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

We should do this to let website better handle power/cpu and for WebApp support

Comment 2

[Martijn Wargers (dead)]

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?

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

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?

Comment 5

[Wesley Johnston (:wesj)]

(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

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

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.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

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.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: test page

Comment 9

[Matt Brubeck (:mbrubeck)]

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.

Comment 10

[Martijn Wargers (dead)]

(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.

Comment 11

[Wesley Johnston (:wesj)]

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

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/0bc32fccce4c