853366 - Clean up pause/resume/start/stop events

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Right now the names of the ACTIVITY_* events in GeckoEvent don't really correspond to when they get fired. We need to audit some of this code and rename stuff to be more correct and/or meaningful.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Small cleanups

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Big cleanups

This one should have no functional change against we have now, but hopefully is easier to follow. Still needs testing, will request review once I'm satisfied.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Big cleanups

So based on experimentation it looks like:
1) when GeckoApp.onStop is called, isApplicationInBackground() returns true if the app is in the background (i.e. not in another activity like the awesomescreen). Since the nsAppShell code for ACTIVITY_STOPPING only ever does stuff when isApplicationInBackground() returns true, that code can be rolled into the ACTIVITY_PAUSING code (now called APP_BACKGROUNDING).
2) when GeckoApp.onStart is called, isApplicationInBackground() *always* returns true, regardless of whether you are coming in to GeckoApp from the android homescreen or returning to it from the awesome screen. The nsAppShell code for ACTIVITY_START therefore always fires application-foreground, which seems silly because it doesn't map 1:1 with application-background. I got rid of this and just let it fire that event in ACTIVITY_RESUMING (now called APP_FOREGROUNDING).
3) ACTIVITY_PAUSE and ACTIVITY_RESUME do seem to get fired at the right times, and I renamed them to APP_FOREGROUNDING and APP_BACKGROUNDING to better reflect what they actually mean. They always passed in a hard-coded value of true for the isApplicationInBackground parameter so I removed that parameter and updated the code accordingly.
4) I put back an APP_EXITING event but I'm not really sure we want to keep this around since it's so unreliable. Thoughts?


Try push at https://tbpl.mozilla.org/?tree=Try&rev=d1b4ffacc878

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 727752 [details] [diff] [review]
Part 1 - Small cleanups

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

LGTM with nits

::: mobile/android/base/GeckoActivity.java.in
@@ +68,1 @@
>          // Whenever we call our own activity, the component and it's package name is set.

1. I would make checkIfGeckoActivity() a static method because it is a pure function that only depends on its parameters, but it's up to you. :)

2. In the comment, s/it's/its/

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 729012 [details] [diff] [review]
Part 2 - Big cleanups

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

LGTM

::: widget/android/nsAppShell.cpp
@@ +407,5 @@
> +        // we want to continue doing like run downloads. Here, we should do aggressive
> +        // memory minimization, and possibly do a full shutdown if we're not doing
> +        // anything we need to stay alive for
> +
> +        // XXX how much of this do we want to do?

1. Please file a follow up bug so we can track this shutdown question OR get someone to sr+ deleting all this Quit code for good. :)

2. Please add a comment stating that the APP_EXITING event is not actually raised! (Right?)

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Small cleanups

Updated to address comments, carrying r+

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Big cleanups

I decided to just axe the entire APP_EXITING thing, because going forward the plan is to do less stuff on shutdown, and the event will only get generated on some devices anyway. Adding mfinkle to the review but I don't see a super-review flag anywhere.

Comment 8

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

Comment on attachment 729507 [details] [diff] [review]
Part 2 - Big cleanups

Overall I think this should be a fine approach. I am trying to think if add-on restarts (we still support them) or restarts for profile changes (we might need them) will be affected. I think things should be OK.

Honestly, we'd like to make Gecko restarts for things like add-ons and profile changes, only restart Gecko, not the entire app (the Java part).

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 729507 [details] [diff] [review]
Part 2 - Big cleanups

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

LGTM!

Comment 10

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aef2b64391ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dacdf908700

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aef2b64391ea
https://hg.mozilla.org/mozilla-central/rev/3dacdf908700