Currently, one can close the browser and the guest mode notification will persist in the Android system tray. -- Samsung Galaxy S5 (Android 4.4.4) Nightly (09/29)

There are two ways to hard-kill an app in Android. Force Stop kills everything -- processes, services, activities. Force Stop removes persistent notifications. No problem there. Swiping away an app in the task switcher kills activities (without invoking onDestroy, apparently). It does not remove persistent notifications. The only way to detect a swipe-death is to use Service#onTaskRemoved in a service that survives the kill. As far as I can tell, then, the only way to improve the situation in this bug is: * Stop GeckoApp/BrowserApp from managing the guest mode notification. This needs to be handled by a service. This will also handily stop us having to load the browser in order to handle a tap on the notification. * Have that service respond to onTaskRemoved to remove the notification. The alternative is that we decide to show a persistent notification only when the activity is in the foreground. This whole caboodle seems a little like a misuse of notifications, and only showing it during browsing perhaps even more so. Anyone have a strong opinion here?

Flagging some likely suspects to comment from their cells in Fennec Gaol.

I agree, it's weird to see the notification if you're not using the app, and especially when you use the Task Manager to quit out and it persists. You don't need a reminder that Guest Mode is running if you're not using it. I am for showing it only when the app is in the foreground.

It builds, but I haven't tested it. This patch: * Breaks out the show/hide logic into BrowserApp#onStart,onStop. This is the right pair of spots in the Android lifecycle. * Delays the onStart logic to make sure we don't early-init the profile. The notification should pop up shortly after startup. * Removes GuestSession#onDestroy, because we now always kill the notification in onStop, and either show or kill it in onStart, so the extra hook is unnecessary. * We also kill the notification immediately when hitting the "Exit" menu item, just for a little extra responsiveness (and belt-and-braces). There is a trivial race condition here: if you manage to stop the activity before the background runnable gets to fire, we'll try to hide the notification before it's added, and so we'll actually add it shortly after you leave the browser. I'm going to see if that's an issue in practice; there are a few straightforward fixes for that if we really care.

Ahh, nice. I swear I did this originally and saw the onStop fire when the notification tray was shown, but I can't reproduce that anymore. Yay :) ?

Comment on attachment 8499866 [details] [diff] [review] Dismiss guest mode notification on browser close. v1 Review of attachment 8499866 [details] [diff] [review]: ----------------------------------------------------------------- One thing that I noticed is that switching to Settings makes the icon go away. Alas, multiple activities! This should ideally be tied to the application, or the application service, but this is no worse than what we do now. Adding a note here for bug 1077583. I also noticed a lot unused imports in GuestSession - I won't be mad if you kill all of them. Also, it looks like quitting on 2.3 devices doesn't (always) clear this icon - you should make sure whichever pathway that is calls hideNotification. Otherwise, looks good! ::: mobile/android/base/BrowserApp.java @@ +799,5 @@ > + public void onStop() { > + super.onStop(); > + > + // We only show the guest mode notification when our activity is in the foreground. > + GuestSession.hideNotification(this); Is getting the Service and canceling the notification cheaper than checking inGuestMode()? I guess this is more of a failsafe as well though, in case we get into some inconsistent state of showing the notification even though we're not in guest mode.

(In reply to Chenxia Liu [:liuche] from comment #6) > One thing that I noticed is that switching to Settings makes the icon go > away. Alas, multiple activities! This should ideally be tied to the > application, or the application service, but this is no worse than what we > do now. The trick here is that the behavior we want is really "show this notification if GeckoApp is present on the current task's stack". "Task" is one of the concepts that Android doesn't reify, instead managing internally. We can't use the Application lifecycle instead, because Applications don't really finish. They stick around to support services. So the notification wouldn't go away. We could maaaaybe try to hack it into the onActivityPause/onActivityResume logic that we have in GeckoApplication, but that seems like it would involve a lot of complexity -- we'd be modeling the Android activity stack ourselves. What we'd really like is something between onStop and onDestroy that means "activity popped off the stack". (We can't use onDestroy because it's non-deterministic.) The reason I didn't actually bother to tackle this for Settings and for Sync is twofold: * We're not going to let you set up Sync, and perhaps not even access Settings at all, so this becomes an academic problem. Guest users simply can't push one of Fennec's other activities into the foreground. * Arguably you need the reminder most when you begin interacting with BrowserApp, so from an interaction perspective, showing a fresh notification when BrowserApp (re)starts might be a convenient accident! > I also noticed a lot unused imports in GuestSession - I won't be mad if you > kill all of them. \o/ > Also, it looks like quitting on 2.3 devices doesn't (always) clear this icon > - you should make sure whichever pathway that is calls hideNotification. Yeah, I made a mental note to find that path. Wasn't able to test it in Starbucks :D Thanks for checking on that! > Is getting the Service and canceling the notification cheaper than checking > inGuestMode()? If we *are* in guest mode, there's no redundant work, of course. (Indeed, diving straight in is the cheapest option!) If we're not, then: Getting the service instance should be ~free. The call ends up here: http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.0.2_r1/com/android/server/NotificationManagerService.java#NotificationManagerService.cancelNotification%28java.lang.String%2Cjava.lang.String%2Cint%2Cint%2Cint%2Cboolean%29 That is: write to the event log (should be cheap, Android does this a lot), enter a synchronized block, and fail to find our ID. In practice this should be too brief to measure. Further, we're calling this in onStop, probably during the interstitial activity transition. It shouldn't materially impact the user experience. > I guess this is more of a failsafe as well though, in case we > get into some inconsistent state of showing the notification even though > we're not in guest mode. I'm on the fence here. To check if we're in guest mode is cheaper (assuming GeckoProfile has already done its work), and strictly speaking we should be able to confidently determine which state we're in. But yes, this is the only remaining piece of belt-and-braces that we have, and it will catch and clean up things like the inverted order of operations in Comment 4 — we'd get a bug report like "guest mode notification sticks around if I really quickly quit, but gets cleaned up next time I leave my regular profile", instead of "guest mode notification sticks around".

Comments addressed -- I hit the Quit path, cleaned up imports.

Comment on attachment 8501502 [details] [diff] [review] Dismiss guest mode notification on browser close. v2 Review of attachment 8501502 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApp.java @@ +441,5 @@ > @Override > public boolean onOptionsItemSelected(MenuItem item) { > if (item.getItemId() == R.id.quit) { > + // Make sure the Guest Browsing notification goes away when > + // we quit. Don't wrap this comment.