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?

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

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.

Depends on bug 1359531 so the GAM is no longer encumbered by the demands of tab type activity switching.

Comment on attachment 8870523 [details] Bug 1346413 - Part 0 - Remove unneeded imports. https://reviewboard.mozilla.org/r/141968/#review145696

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.

Comment on attachment 8870524 [details] Bug 1346413 - Part 2 - Remove GeckoActivityMonitor onNewIntent handling. https://reviewboard.mozilla.org/r/141970/#review145752

Comment on attachment 8870525 [details] Bug 1346413 - Part 3 - Remove GeckoActivityStatus-based background detection. https://reviewboard.mozilla.org/r/141972/#review145754

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

https://hg.mozilla.org/mozilla-central/rev/232a22894983 https://hg.mozilla.org/mozilla-central/rev/34672acd7c14 https://hg.mozilla.org/mozilla-central/rev/828b3361dcd5 https://hg.mozilla.org/mozilla-central/rev/be8d550b1adc