679923 - Fennec must deactivate last selected tab when goes to background

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Oleg Romashin (:romaxa)]

Fennec deactivating docshell for background tabs, but not selected tab when it goes to background...

Here is Qt Maemo implementation for system notifications 
https://bugzilla.mozilla.org/attachment.cgi?id=553792&action=edit

Something similar needed for android...

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Frontend part

Comment 2

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

Why Util.timeout and not just setTimeout(..., 0) ?

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Deactivate last selected tab on background/inactive signals

No specific reason, just found first usage of timers in other code, and used it here.
Timer used here in order to delay tab deactivation in next event loop... in order to have fennec repainted in TaskSwitcher properly before tab deactivated.

Comment 4

[Alon Zakai (:azakai)]

Comment on attachment 553947 [details] [diff] [review]
Frontend part

Review of attachment 553947 [details] [diff] [review]:
-----------------------------------------------------------------

I am concerned that using a setTimeout for deactivating, but not for activating, could in some extremely rare cases lead to a bug. If we get notifications that cause us to deactivate and then activate immediately one after the other, then because of the delay in deactivating it may happen *after* the activation, leading to the opposite of the right result. The safest way to fix this I think is to check the values again inside the timeout, before deactivating. (Another possibility is to do a delay in both cases, but that relies on timing being identical so it worries me.) Or are we certain that this situation will not arise somehow? If so a comment would be good.

Otherwise this looks fine, but please make sure the order of the observer adding and removing stuff is the same (idle/active is reversed).

I don't think I have the power to single-handedly review a frontend patch though, should ask mfinkle to review it too. Also there is the UI aspect: mfinkle, do we really want to mark as inactive tabs when the screen is off? This would break some sites, like say a radio-playing web page (due to timer throttling) that the user expects to continue to run. Might be worth it though - I am in favor of it myself.

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Android ts notifications

Blind patch for android port... from api
http://developer.android.com/reference/android/app/Activity.html
it seems should be working

Comment 6

[Oleg Romashin (:romaxa)]

Audio playing is not affected by this...

Comment 7

[Wesley Johnston (:wesj)]

If we do do this, I think we should implement something like the page visibility api to give pages an opportunity to opt out. preventDefault() on visibilityChange events could keep the tab alive?

http://www.w3.org/TR/page-visibility/

Comment 8

[Alon Zakai (:azakai)]

(In reply to Oleg Romashin (:romaxa) from comment #6)
> Audio playing is not affected by this...

If you are generating audio through the Audio Data API and relying on a frequent timeout, then the timeout throttling will break you.

Maybe this is rare enough to ignore? I don't know. (But I do hope more people will use the Audio Data API...)

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Deactivate last selected tab on background/inactive signals

Addressed Alon comments..

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 554156 [details] [diff] [review]
Deactivate last selected tab on background/inactive signals

err syntax here is wrong

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: Deactivate last selected tab on background/inactive signals

Ok, seems all visibility, issues was fixed., tested works fine

Comment 13

[Matt Brubeck (:mbrubeck)]

(In reply to Alon Zakai (:azakai) from comment #8)
> If you are generating audio through the Audio Data API and relying on a
> frequent timeout, then the timeout throttling will break you.

Audio players already need to handle this case to work in background tabs, so I'm not concerned that this change will break them.

Comment 14

[Oleg Romashin (:romaxa)]

Comment on attachment 554144 [details] [diff] [review]
Android ts notifications

mbrubek suggested you to review this patch

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 554198 [details] [diff] [review]
Deactivate last selected tab on background/inactive signals

Stealing review because mfinkle is not around.

>+    if ((Browser.selectedTab.active && !this._activeTabState) ||
>+        (!Browser.selectedTab.active && this._activeTabState)) {

I'd prefer "if (Browser.selectedTab.active != this._activeTabState) {"

>+      setTimeout(function() { Browser.selectedTab.active = self._activeTabState; }, 0);

Please add a comment explaining why "setTimeout(..., 0)" is needed here.

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: Deactivate last selected tab on background/inactive signals, fixed nits

Comment 17

[Oleg Romashin (:romaxa)]

Attachment: Deactivate last selected tab on background/inactive signals

Fixed reviewer

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 554144 [details] [diff] [review]
Android ts notifications

Review of attachment 554144 [details] [diff] [review]:
-----------------------------------------------------------------

re-request review if you actually intend for this to happen on screen timeout

::: widget/src/android/nsAppShell.cpp
@@ +355,5 @@
>              prefs->SavePrefFile(nsnull);
>          }
> +        nsCOMPtr<nsIObserverService> obsServ =
> +            mozilla::services::GetObserverService();
> +        obsServ->NotifyObservers(nsnull, "ts-background", nsnull);

ACTIVITY_PAUSING will be sent when the screen times out. I think you actually want ACTIVITY_STOPPING which will get sent when the app goes into the background

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: Android ts, active notifications

ACTIVITY_PAUSING - system-idle
ACTIVITY_RESUME - system-active
ACTIVITY_STOPPING - ts-background
ACTIVITY_RESTART - ts-foreground

Comment 20

[Oleg Romashin (:romaxa)]

Attachment: Android ts, active notifications v2

Is it better matching?
ACTIVITY_PAUSING - system-display-off
ACTIVITY_RESUME - system-display-on
ACTIVITY_STOPPING - ts-background
ACTIVITY_RESTART - ts-foreground
or should I just keep ts-* stuff and forget about ACTIVITY_PAUSING/RESUME for android?

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Oleg Romashin (:romaxa) from comment #20)
> Created attachment 554325 [details] [diff] [review]
> Android ts, active notifications v2
> 
> Is it better matching?
> ACTIVITY_PAUSING - system-display-off
> ACTIVITY_RESUME - system-display-on
> ACTIVITY_STOPPING - ts-background
> ACTIVITY_RESTART - ts-foreground
no, not really.

See [http://developer.android.com/guide/topics/fundamentals/activities.html#ImplementingLifecycleCallbacks] for a description of the android activity lifecycle. Pause/resume happens when the activity is no longer visible while stop/restart are when it goes to the foreground and background. It's obviously a bit nuanced, and usually they come together (i.e. you get a pause event immediately before a stop event), but you will get a pause event without a stop event if the screen times out or if the lock screen comes up. You'll also get a pause event without a stop event if a system dialog pops up, which is in front of the activity but doesn't take the entire screen.


> or should I just keep ts-* stuff and forget about ACTIVITY_PAUSING/RESUME
> for android?

that might be better

Comment 22

[Oleg Romashin (:romaxa)]

Attachment: Android ts, active notifications, only bg/fg handling

Comment 23

[Oleg Romashin (:romaxa)]

Attachment: Android ts, active notifications, only bg/fg handling

Checked on try, and found null vs nsnull problem... fixed here

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

where did the "ts-" prefix come from?

Comment 25

[Oleg Romashin (:romaxa)]

it is short from tasks-switcher, (View where you see opened applications). it is available in Maemo/Dead-WebOS... on android it is just background applications

Comment 26

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

Maybe we could use "application-foreground" and "application-background"

What do you think? "ts-" just isn't very readable

Comment 27

[Oleg Romashin (:romaxa)]

Attachment: Deactivate last selected tab when fennec in background/inactive state

Comment 28

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

Comment on attachment 554569 [details] [diff] [review]
Deactivate last selected tab when fennec in background/inactive state


>+var ActivityObserver = {

>+  observe: function ao_observe(aSubject, aTopic, aData) {

>+    this._activeTabState = !this._inBackground && !this._notActive && !this._isDisplayOff;
>+    if (Browser.selectedTab.active != this._activeTabState) {
>+      let self = this;
>+      // On Maemo all backgrounded applications getting portrait orientation
>+      // so if browser had landscape mode then we need timeout in order
>+      // to finish last rotate/paint operation and have nice lookine browser in TS
>+      setTimeout(function() { Browser.selectedTab.active = self._activeTabState; }, 0);
>+    }
>+  }
>+};

The mobile changes look fine, but I have one question: Do we need _activeTabState to be persistent (a data member)? if not, you can just make it a local and drop the need for "self"

r+, but if we can make it local, that would be better.

Comment 29

[Oleg Romashin (:romaxa)]

Attachment: Deactivate last selected tab when fennec in background/inactive state

Fixed mfinlke comment

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 554597 [details] [diff] [review]
Deactivate last selected tab when fennec in background/inactive state

Review of attachment 554597 [details] [diff] [review]:
-----------------------------------------------------------------

the application-foreground notification should probably be sent from onStart() rather than onRestart(), so adjust this accordingly.
r=blassey with that change, though I'd be happy to re-review if you'd like

(sorry for not catching that earlier)

Comment 31

[Oleg Romashin (:romaxa)]

Attachment: Deactivate last selected tab when fennec in background/inactive state.

Moved to OnStart.

Comment 32

[Oleg Romashin (:romaxa)]

Attachment: Deactivate last selected tab when fennec in background/inactive state.

Refreshed against latest trunk

Comment 33

[Ed Morley [:emorley]]

Sent to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=9a567d609584

Assuming all green, will push to inbound after :-)

Comment 34

[Oleg Romashin (:romaxa)]

Attachment: Deactivate last selected tab when fennec in background/inactive state.

Try will fail, because I again grab old patch with broken nsnull, param...
Here is the same patch but s/nsull/nsnull fix

Comment 35

[Ed Morley [:emorley]]

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=86f78f470d87

http://hg.mozilla.org/integration/mozilla-inbound/rev/45f8416ebe65

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/45f8416ebe65

Comment 37

[Oleg Romashin (:romaxa)]

Comment on attachment 554718 [details] [diff] [review]
Deactivate last selected tab when fennec in background/inactive state.

This patch is quite important for saving battery life with fennec running in background.
In additional on N9 with patch https://bugzilla.mozilla.org/attachment.cgi?id=554737&action=edit and https://bugzilla.mozilla.org/attachment.cgi?id=553994&action=edit - it will suspend plugins which without these changes can drain battery very quickly..

Without this fix, shell-active/inactive only applied for background tabs, with this fix it is also applied for last active tab while fennec app in background, so we just using old functionality in one additional case, and I believe risk should be minimal

Comment 38

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

Comment on attachment 554718 [details] [diff] [review]
Deactivate last selected tab when fennec in background/inactive state.

Leaving in fx9.