Closed Bug 679923 Opened 8 years ago Closed 8 years ago

Fennec must deactivate last selected tab when goes to background

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 9

People

(Reporter: romaxa, Assigned: romaxa)

References

()

Details

(Whiteboard: QA?[not-fennec-11])

Attachments

(1 file, 15 obsolete files)

8.28 KB, patch
Details | Diff | Splinter Review
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...
Attached patch Frontend part (obsolete) — Splinter Review
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #553947 - Flags: review?(azakai)
Why Util.timeout and not just setTimeout(..., 0) ?
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.
Attachment #554111 - Flags: review?(mark.finkle)
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.
Attachment #553947 - Flags: review?(azakai)
Attached patch Android ts notifications (obsolete) — Splinter Review
Blind patch for android port... from api
http://developer.android.com/reference/android/app/Activity.html
it seems should be working
Attachment #553947 - Attachment is obsolete: true
Attachment #554144 - Flags: review?(mwu)
Audio playing is not affected by this...
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/
(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...)
Addressed Alon comments..
Attachment #554111 - Attachment is obsolete: true
Attachment #554111 - Flags: review?(mark.finkle)
Attachment #554156 - Flags: review?(mark.finkle)
Comment on attachment 554156 [details] [diff] [review]
Deactivate last selected tab on background/inactive signals

err syntax here is wrong
Attachment #554156 - Flags: review?(mark.finkle)
Ok, seems all visibility, issues was fixed., tested works fine
Attachment #554156 - Attachment is obsolete: true
Attachment #554198 - Flags: review?(mark.finkle)
Duplicate of this bug: 668271
(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.
OS: Linux → All
Hardware: x86 → All
Comment on attachment 554144 [details] [diff] [review]
Android ts notifications

mbrubek suggested you to review this patch
Attachment #554144 - Flags: review?(mwu) → review?(azakai)
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.
Attachment #554198 - Flags: review?(mark.finkle) → review+
Attachment #554144 - Flags: review?(blassey.bugs)
Fixed reviewer
Attachment #554198 - Attachment is obsolete: true
Attachment #554250 - Attachment is obsolete: true
Attachment #554258 - Flags: review+
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
Attachment #554144 - Flags: review?(blassey.bugs) → review-
Attached patch Android ts, active notifications (obsolete) — Splinter Review
ACTIVITY_PAUSING - system-idle
ACTIVITY_RESUME - system-active
ACTIVITY_STOPPING - ts-background
ACTIVITY_RESTART - ts-foreground
Attachment #554144 - Attachment is obsolete: true
Attachment #554144 - Flags: review?(azakai)
Attachment #554307 - Flags: review?(blassey.bugs)
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?
Attachment #554325 - Flags: review?(blassey.bugs)
(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
Attachment #554307 - Attachment is obsolete: true
Attachment #554325 - Attachment is obsolete: true
Attachment #554307 - Flags: review?(blassey.bugs)
Attachment #554325 - Flags: review?(blassey.bugs)
Attachment #554442 - Flags: review?(blassey.bugs)
Checked on try, and found null vs nsnull problem... fixed here
Attachment #554442 - Attachment is obsolete: true
Attachment #554442 - Flags: review?(blassey.bugs)
Attachment #554474 - Flags: review?(blassey.bugs)
where did the "ts-" prefix come from?
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
Maybe we could use "application-foreground" and "application-background"

What do you think? "ts-" just isn't very readable
Attachment #554258 - Attachment is obsolete: true
Attachment #554474 - Attachment is obsolete: true
Attachment #554474 - Flags: review?(blassey.bugs)
Attachment #554569 - Flags: review?(mark.finkle)
Attachment #554569 - Flags: review?(mark.finkle) → review?(blassey.bugs)
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.
Attachment #554569 - Flags: review+
Fixed mfinlke comment
Attachment #554569 - Attachment is obsolete: true
Attachment #554569 - Flags: review?(blassey.bugs)
Attachment #554597 - Flags: review?(blassey.bugs)
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)
Attachment #554597 - Flags: review?(blassey.bugs) → review+
Moved to OnStart.
Attachment #554597 - Attachment is obsolete: true
Attachment #554666 - Flags: review?(blassey.bugs)
Refreshed against latest trunk
Attachment #554666 - Attachment is obsolete: true
Attachment #554666 - Flags: review?(blassey.bugs)
Attachment #554668 - Flags: review?(blassey.bugs)
Attachment #554668 - Flags: review?(blassey.bugs) → review+
Keywords: checkin-needed
Sent to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=9a567d609584

Assuming all green, will push to inbound after :-)
Keywords: checkin-needed
Try will fail, because I again grab old patch with broken nsnull, param...
Here is the same patch but s/nsull/nsnull fix
Attachment #554668 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/45f8416ebe65
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 679733
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
Attachment #554718 - Flags: approval-mozilla-aurora?
Comment on attachment 554718 [details] [diff] [review]
Deactivate last selected tab when fennec in background/inactive state.

Leaving in fx9.
Attachment #554718 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Whiteboard: QA?
Depends on: 696732
Depends on: 702823
Depends on: 693930
Depends on: 712517
Depends on: 712076
Whiteboard: QA? → QA?[not-fennec-11]
You need to log in before you can comment on or make changes to this bug.