Closed Bug 1346413 Opened 7 years ago Closed 7 years ago

Going back from a CustomTabsActivity doesn't trigger application-background notification

Categories

(Firefox for Android Graveyard :: General, enhancement)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(4 files)

Bug 1346413 - Part 1 - GeckoActivityMonitor/onStop-based application-background/foreground tracking.
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1346413 - Part 0 - Remove unneeded imports.
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1346413 - Part 2 - Remove GeckoActivityMonitor onNewIntent handling.
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1346413 - Part 3 - Remove GeckoActivityStatus-based background detection.
59 bytes, text/x-review-board-request
jchen
: review+
Details 
even though we're leaving the app.
This is not good, because this is the Gecko extension of onPause(), which is the last guaranteed chance to do stuff before being sent into the background and possibly killed. [1]

This is because here we're finishing the activity (compare https://dxr.mozilla.org/mozilla-central/rev/528e9dbbb882db0b32792d44b5be9cc539afa1a8/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#110), which we used to do only while navigating *within* our app (e.g. our preferences), but not when leaving it.

[1] E.g. Gecko's cache doesn't like being killed unceremoniously, so we shut it down cleanly when going into the background.
Dylan, we'll need to handle this somehow with the GV-based custom tabs. We should add the setActive() method like we had talked about before, and then I think we can infer the foreground/background status of Gecko based on the presence of any active <browsers>. This should interact well with Fennec too. Jim, do you have any ideas on this?
Assignee: nobody → droeh
Flags: needinfo?(nchen)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #1)
> We should add the setActive() method like we had talked about before, and then
> I think we can infer the foreground/background status of Gecko based on the
> presence of any active <browsers>.

Doesn't this just move the problem onto "when do we set a browser to active or not"? The easy bit is for each activity containing a GeckoView to track its foreground state via onPause/onResume and set its browser state based on that, but
- we ideally want to handle this synchronously in response to Android's onPause notification (because afterwards the OS can randomly kill us), while at the same time
- switching between different activities that are both displaying a <browser> shouldn't trigger a background notification that is immediately going to be followed by a foreground notification for the new activity anyway
- switching to some other non-Gecko based activity within our app (our settings) probably shouldn't trigger a notification either

and unfortunately Android doesn't tell us in onPause whether it's just going to launch some other activity of ours or else send the whole app into the background, so we still need additional logic for that just as today.
(In reply to Jan Henning [:JanH] from comment #2)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #1)
> > We should add the setActive() method like we had talked about before, and then
> > I think we can infer the foreground/background status of Gecko based on the
> > presence of any active <browsers>.
> 
> Doesn't this just move the problem onto "when do we set a browser to active
> or not"?

It does, but the application itself is the only thing that really knows about that state.

> The easy bit is for each activity containing a GeckoView to track
> its foreground state via onPause/onResume and set its browser state based on
> that, but

Right.

> - we ideally want to handle this synchronously in response to Android's
> onPause notification (because afterwards the OS can randomly kill us), while
> at the same time
> - switching between different activities that are both displaying a
> <browser> shouldn't trigger a background notification that is immediately
> going to be followed by a foreground notification for the new activity anyway
> - switching to some other non-Gecko based activity within our app (our
> settings) probably shouldn't trigger a notification either
> 
> and unfortunately Android doesn't tell us in onPause whether it's just going
> to launch some other activity of ours or else send the whole app into the
> background, so we still need additional logic for that just as today.

Yeah, I think we just need to handle quick background/foreground transitions as best we can. Nothing much we can do beyond that.
Oh, something we can do to make this easier is offer a GeckoViewFragment which does the background/foreground stuff automatically.
I think dumb GeckoThread.onPause/onResume calls from the activity are enough for the vast majority of cases. Maybe add an optimization for CustomTabsActivity > BrowserApp.

We can also consider calling GeckoThread.onPause from Activity.onStop, instead of Activity.onPause. In that case, when we switch activities, the new activity's onStart will be called _before_ the old activity's onStop, and it'd be easy to handle activity switching.
Flags: needinfo?(nchen)
I was a bit weary of using onStop() because of the ominous warning that it's not guaranteed to be called, but if we're okay with going down that route, the above patch is approximately how this could work (might need a little polishing plus removal of our current GeckoActivityStatus logic). A quick test seems to indicate it's working fine, although I've no idea if there might not be some modified Android versions floating around whose onStop() handling might be more unreliable.

The beauty of this approach however would indeed be that it doesn't require any further support from any activities along the lines of today's GeckoActivityStatus - instead, everything's covered automatically by the ActivityLifecycleCallbacks.
Dylan, can I steal this from you if we want to go for an onStop-based approach in the end?
(In reply to Jan Henning [:JanH] from comment #8)
> Dylan, can I steal this from you if we want to go for an onStop-based
> approach in the end?

Sure, the onStop-based approach looks good to me, so the bug is yours if you want it.
Assignee: droeh → jh+bugzilla
Depends on bug 1359531 so the GAM is no longer encumbered by the demands of tab type activity switching.
Depends on: 1359531
Comment on attachment 8870523 [details]
Bug 1346413 - Part 0 - Remove unneeded imports.

https://reviewboard.mozilla.org/r/141968/#review145696
Attachment #8870523 - Flags: review?(nchen) → review+
Comment on attachment 8869247 [details]
Bug 1346413 - Part 1 - GeckoActivityMonitor/onStop-based application-background/foreground tracking.

https://reviewboard.mozilla.org/r/140814/#review145700

Awesome! Thanks!

::: mobile/android/base/java/org/mozilla/gecko/GeckoActivityMonitor.java:41
(Diff revision 2)
> -        currentActivity = activity;
> +            return;
> +        }
> +
> +        Context appContext = context.getApplicationContext();
> +
> +        if (appContext instanceof GeckoApplication) {

Don't need this check because the cast to `GeckoApplication` will throw an exception for you if it's not the right type.
Attachment #8869247 - Flags: review?(nchen) → review+
Comment on attachment 8870524 [details]
Bug 1346413 - Part 2 - Remove GeckoActivityMonitor onNewIntent handling.

https://reviewboard.mozilla.org/r/141970/#review145752
Attachment #8870524 - Flags: review?(nchen) → review+
Comment on attachment 8870525 [details]
Bug 1346413 - Part 3 - Remove GeckoActivityStatus-based background detection.

https://reviewboard.mozilla.org/r/141972/#review145754
Attachment #8870525 - Flags: review?(nchen) → review+
Comment on attachment 8869247 [details]
Bug 1346413 - Part 1 - GeckoActivityMonitor/onStop-based application-background/foreground tracking.

https://reviewboard.mozilla.org/r/140814/#review145700

> Don't need this check because the cast to `GeckoApplication` will throw an exception for you if it's not the right type.

I originally stored that reference as a `Context`, so it might have made sense to fail early that way, but you're right, it's no longer needed this way.
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/232a22894983
Part 0 - Remove unneeded imports. r=jchen
https://hg.mozilla.org/integration/autoland/rev/34672acd7c14
Part 1 - GeckoActivityMonitor/onStop-based application-background/foreground tracking. r=jchen
https://hg.mozilla.org/integration/autoland/rev/828b3361dcd5
Part 2 - Remove GeckoActivityMonitor onNewIntent handling. r=jchen
https://hg.mozilla.org/integration/autoland/rev/be8d550b1adc
Part 3 - Remove GeckoActivityStatus-based background detection. r=jchen
Depends on: 1373224
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: