Closed Bug 853366 Opened 8 years ago Closed 8 years ago

Clean up pause/resume/start/stop events

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached patch Part 2 - Big cleanups (obsolete) — Splinter Review
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.
Attached patch Part 2 - Big cleanups (obsolete) — Splinter Review
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
Attachment #727756 - Attachment is obsolete: true
Attachment #729012 - Flags: review?(cpeterson)
Attachment #727752 - Flags: review?(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/
Attachment #727752 - Flags: review?(cpeterson) → review+
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?)
Attachment #729012 - Flags: review?(cpeterson) → review+
Updated to address comments, carrying r+
Attachment #727752 - Attachment is obsolete: true
Attachment #729505 - Flags: review+
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.
Attachment #729012 - Attachment is obsolete: true
Attachment #729507 - Flags: review?(mark.finkle)
Attachment #729507 - Flags: review?(cpeterson)
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).
Attachment #729507 - Flags: review?(mark.finkle) → review+
Comment on attachment 729507 [details] [diff] [review]
Part 2 - Big cleanups

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

LGTM!
Attachment #729507 - Flags: review?(cpeterson) → review+
https://hg.mozilla.org/mozilla-central/rev/aef2b64391ea
https://hg.mozilla.org/mozilla-central/rev/3dacdf908700
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.