Change method of activity tracking done in ActivityStatus.

Change method of activity tracking done in ActivityStatus.

Use the ability to track activity changes by registering a listener on
the application.

Also, this will manage the state of various activities on the java side.

BUG=286071
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223437

[Ted C, 2013-09-12 17:53:04]

Looking at trying to make activity status a bit easier to manage and less
clobbery.

Thoughts?

[David Trainor- moved to gerrit, 2013-09-12 22:12:32]

lgtm

https://chromiumcodereview.appspot.com/23522032/diff/1/base/android/java/src/...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/23522032/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:160: if (sActivity
== null) return DESTROYED;
Can an Activity be considered to be not paused and not destroyed?  Just looking
at isPaused() vs getState() in the case where sActivity is null.

https://chromiumcodereview.appspot.com/23522032/diff/1/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/23522032/diff/1/base/base.gyp#newcode1218
base/base.gyp:1218: '../third_party/guava/guava.gyp:guava_javalib',
?

[Ted C, 2013-09-12 23:14:54]

https://chromiumcodereview.appspot.com/23522032/diff/1/base/android/java/src/...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/23522032/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:160: if (sActivity
== null) return DESTROYED;
On 2013/09/12 22:12:32, David Trainor wrote:
> Can an Activity be considered to be not paused and not destroyed?  Just
looking
> at isPaused() vs getState() in the case where sActivity is null.

Chatted a bit about this an somewhat conflicted.  I think we decided that this
method probably isn't useful and I added an isActive that is more useful for
determining whether the activity is truly in the foreground.  Checking only
PAUSED and not STOPPED is probably wrong in almost all circumstances, so I think
we should just switch to isActive and remove this method entirely.

https://chromiumcodereview.appspot.com/23522032/diff/1/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/23522032/diff/1/base/base.gyp#newcode1218
base/base.gyp:1218: '../third_party/guava/guava.gyp:guava_javalib',
On 2013/09/12 22:12:32, David Trainor wrote:
> ?

Needed for VisibleForTesting

[gone, 2013-09-13 00:45:56]

LGTM, but I'd defer to pliard.

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:34: // Current main
activity, or null if none.
Current visible activity?  Last activity that was shown?

[Philippe, 2013-09-13 09:42:58]

Thanks Ted! This looks good to me overall although I'm  (maybe unnecessarily?)
worried about the possible side effects (see my comment) and the implications
without follow up CLs in the application repository. I'm CC'ing Tommy and
Anthony who might want to be aware/look into this if it makes any sense.

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:37: private static
Map<Activity, Integer> sActivityStates = new HashMap<Activity, Integer>();
Nit: did you omit the 'final' so that this can fit on a single line? :)

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:108: public static
void onStateChange(Activity activity, int newState) {
Ideally we should remove all the references to this method (which is very good
news IMO) in non-testing code to avoid clashes with the
ActivityLifecycleCallbacks, right? At which point of the application startup do
you intend to call ActivityStatus.initialize()? The first event that
ActivityLifecycleCallbacks will fire is STARTED if we call initialize() very
early right after creation, right? 

I'm wondering whether we could mark this method as deprecated and have a
onStateChangeForTesting method (I agree that the name is not super sexy :)) that
we would only use for testing. This would also let us not add the guava
dependency although I have nothing against it.

Wdyt?

With this CL it seems that we are also not deferring the CREATE event (that
would have been replaced with RESUMED by Tommy btw, right?) anymore thanks to
the ActivityLifecycleCallbacks. I think this is a really good change. But this
means that there could possibly be a small startup performance impact if clients
spawn some expensive work when the RESUMED event is fired? And could this even
lead to some clients calling native code while the native library is not loaded
yet? In my opinion, clients should just post a delayed task when they receive
the notification if they want to do some expensive work. I don't how this is
handled currently but if they depend on native code maybe we should also let
them subscribe to a native library loaded event.

So ideally I (I mean you if you don't mind :)) would file a bug (assigned to
aberent@?) to track this. Or am I completely wrong and just worrying too much?
:)

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:152: * @return
Whether the current active is considered to be "active".  An activity is only
Nit: s/active/activity?

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:181: * Query the
state for a given activity.  If the activity is being tracked, this will
Nit: s/being/not being?

[bulach, 2013-09-13 13:08:48]

thanks ted! lgtm, I think my comments are more in terms of documentation, so
I'll leave the final say to pliard.

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:69:
onStateChange(activity, DESTROYED);
should this call unregisterActivityLifecycleCallbacks(this); as well?

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:185: * @return The
state of the specified activity.
should we add some words about the ordering, to avoid confusion?

from:
http://developer.android.com/reference/android/app/Activity.html#onPause()
When activity B is launched in front of activity A, this callback will be
invoked on A. B will not be created until A's onPause() returns, so be sure to
not do anything lengthy here.


also: what happens if an Application has its own implementation for the
ActivityLifecycleCallbacks that gets called before this one?
there's a potential for inconsistent results..

[aberent, 2013-09-13 15:25:43]

https://codereview.chromium.org/23522032/diff/23001/base/android/java/src/org...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://codereview.chromium.org/23522032/diff/23001/base/android/java/src/org...
base/android/java/src/org/chromium/base/ActivityStatus.java:108: public static
void onStateChange(Activity activity, int newState) {
On 2013/09/13 09:42:58, Philippe wrote:
> Ideally we should remove all the references to this method (which is very good
> news IMO) in non-testing code to avoid clashes with the
> ActivityLifecycleCallbacks, right? At which point of the application startup
do
> you intend to call ActivityStatus.initialize()? The first event that
> ActivityLifecycleCallbacks will fire is STARTED if we call initialize() very
> early right after creation, right? 
> 
> I'm wondering whether we could mark this method as deprecated and have a
> onStateChangeForTesting method (I agree that the name is not super sexy :))
that
> we would only use for testing. This would also let us not add the guava
> dependency although I have nothing against it.
> 
> Wdyt?
> 
> With this CL it seems that we are also not deferring the CREATE event (that
> would have been replaced with RESUMED by Tommy btw, right?) anymore thanks to
> the ActivityLifecycleCallbacks. I think this is a really good change. But this
> means that there could possibly be a small startup performance impact if
clients
> spawn some expensive work when the RESUMED event is fired? And could this even
> lead to some clients calling native code while the native library is not
loaded
> yet? In my opinion, clients should just post a delayed task when they receive
> the notification if they want to do some expensive work. I don't how this is
> handled currently but if they depend on native code maybe we should also let
> them subscribe to a native library loaded event.
> 
> So ideally I (I mean you if you don't mind :)) would file a bug (assigned to
> aberent@?) to track this. Or am I completely wrong and just worrying too much?
> :)

I can't find any detailed documentation about when the
ActivityLifecycleCallbacks are called, and I am unclear how this is intended to
be used, but it looks dangerous. I would guess that the callbacks are called
sometime during what Android sees as the processing of the events.
Unfortunately, to speed up startup, and to get the UI interactive early, a
CREATE event is "processed" by queuing up lots of UI tasks, and starting non UI
threads, to do the real startup work. Early RESUME events (received while the
startup tasks are running) are processed by simply queuing up a further UI task
to do the actual resume. As such nothing should be making any assumptions about
the state of Chrome based on simply what Android events it has "processed".

In particular the C++ side of Chrome will not be fully configured until some
time after Chrome's OnCreate call has completed.

[Ted C, 2013-09-13 18:20:07]

Comments addressed (hopefully)

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:34: // Current main
activity, or null if none.
On 2013/09/13 00:45:56, dfalcantara wrote:
> Current visible activity?  Last activity that was shown?

Done.

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:37: private static
Map<Activity, Integer> sActivityStates = new HashMap<Activity, Integer>();
On 2013/09/13 09:42:58, Philippe wrote:
> Nit: did you omit the 'final' so that this can fit on a single line? :)

Nope, that was an accident.  Added final

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:69:
onStateChange(activity, DESTROYED);
On 2013/09/13 13:08:48, bulach wrote:
> should this call unregisterActivityLifecycleCallbacks(this); as well?

So this registers it on the application.  The lifetime of ActivityStatus is tied
to the application, so we shouldn't ever need to unregister (as it will only be
destroyed by android).

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:108: public static
void onStateChange(Activity activity, int newState) {
On 2013/09/13 09:42:58, Philippe wrote:
> Ideally we should remove all the references to this method (which is very good
> news IMO) in non-testing code to avoid clashes with the
> ActivityLifecycleCallbacks, right? At which point of the application startup
do
> you intend to call ActivityStatus.initialize()? The first event that
> ActivityLifecycleCallbacks will fire is STARTED if we call initialize() very
> early right after creation, right? 
> 
> I'm wondering whether we could mark this method as deprecated and have a
> onStateChangeForTesting method (I agree that the name is not super sexy :))
that
> we would only use for testing. This would also let us not add the guava
> dependency although I have nothing against it.
> 
> Wdyt?

Sounds good to me.  I added TODO (and marked it deprecated) to make it private
once the downstream usages change.

> 
> With this CL it seems that we are also not deferring the CREATE event (that
> would have been replaced with RESUMED by Tommy btw, right?) anymore thanks to
> the ActivityLifecycleCallbacks. I think this is a really good change. But this
> means that there could possibly be a small startup performance impact if
clients
> spawn some expensive work when the RESUMED event is fired? And could this even
> lead to some clients calling native code while the native library is not
loaded
> yet? In my opinion, clients should just post a delayed task when they receive
> the notification if they want to do some expensive work. I don't how this is
> handled currently but if they depend on native code maybe we should also let
> them subscribe to a native library loaded event.

Agreed, we should be very clear that this is specific to activity status and not
some hybrid quasi status kind of linked to activities.  If we need to come up
with a different on native initialized callback for everyone, then that seems
reasonable as well (and I think tommy added callback methods that can be added
onto the browser initialize process at anytime).

> 
> So ideally I (I mean you if you don't mind :)) would file a bug (assigned to
> aberent@?) to track this. Or am I completely wrong and just worrying too much?
> :)

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:108: public static
void onStateChange(Activity activity, int newState) {
On 2013/09/13 15:25:44, aberent wrote:
> On 2013/09/13 09:42:58, Philippe wrote:
> > Ideally we should remove all the references to this method (which is very
good
> > news IMO) in non-testing code to avoid clashes with the
> > ActivityLifecycleCallbacks, right? At which point of the application startup
> do
> > you intend to call ActivityStatus.initialize()? The first event that
> > ActivityLifecycleCallbacks will fire is STARTED if we call initialize() very
> > early right after creation, right? 
> > 
> > I'm wondering whether we could mark this method as deprecated and have a
> > onStateChangeForTesting method (I agree that the name is not super sexy :))
> that
> > we would only use for testing. This would also let us not add the guava
> > dependency although I have nothing against it.
> > 
> > Wdyt?
> > 
> > With this CL it seems that we are also not deferring the CREATE event (that
> > would have been replaced with RESUMED by Tommy btw, right?) anymore thanks
to
> > the ActivityLifecycleCallbacks. I think this is a really good change. But
this
> > means that there could possibly be a small startup performance impact if
> clients
> > spawn some expensive work when the RESUMED event is fired? And could this
even
> > lead to some clients calling native code while the native library is not
> loaded
> > yet? In my opinion, clients should just post a delayed task when they
receive
> > the notification if they want to do some expensive work. I don't how this is
> > handled currently but if they depend on native code maybe we should also let
> > them subscribe to a native library loaded event.
> > 
> > So ideally I (I mean you if you don't mind :)) would file a bug (assigned to
> > aberent@?) to track this. Or am I completely wrong and just worrying too
much?
> > :)
> 
> I can't find any detailed documentation about when the
> ActivityLifecycleCallbacks are called, and I am unclear how this is intended
to
> be used, but it looks dangerous.

Documentation?  Who needs documentation when you have the code!

Some semblance of this is dispatched at the end of all the Activity state
methods in the base Activity.java class.
getApplication().dispatchActivityCreated(this, savedInstanceState);

Each of these methods just loops over the list of state listeners and broadcasts
the notification.

By hooking into the same callback, we should be having the state activity state
awareness of the application, which to mean seems to be the most consistent.

 I would guess that the callbacks are called
> sometime during what Android sees as the processing of the events.
> Unfortunately, to speed up startup, and to get the UI interactive early, a
> CREATE event is "processed" by queuing up lots of UI tasks, and starting non
UI
> threads, to do the real startup work. Early RESUME events (received while the
> startup tasks are running) are processed by simply queuing up a further UI
task
> to do the actual resume. As such nothing should be making any assumptions
about
> the state of Chrome based on simply what Android events it has "processed".
> 
> In particular the C++ side of Chrome will not be fully configured until some
> time after Chrome's OnCreate call has completed.

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:152: * @return
Whether the current active is considered to be "active".  An activity is only
On 2013/09/13 09:42:58, Philippe wrote:
> Nit: s/active/activity?

Done.

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:181: * Query the
state for a given activity.  If the activity is being tracked, this will
On 2013/09/13 09:42:58, Philippe wrote:
> Nit: s/being/not being?

Done.

https://chromiumcodereview.appspot.com/23522032/diff/23001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:185: * @return The
state of the specified activity.
On 2013/09/13 13:08:48, bulach wrote:
> should we add some words about the ordering, to avoid confusion?
> 
> from:
> http://developer.android.com/reference/android/app/Activity.html#onPause%28)
> When activity B is launched in front of activity A, this callback will be
> invoked on A. B will not be created until A's onPause() returns, so be sure to
> not do anything lengthy here.

I tried to add documentation about expected lifecycles.  Let me know if it's
clear.

> 
> 
> also: what happens if an Application has its own implementation for the
> ActivityLifecycleCallbacks that gets called before this one?
> there's a potential for inconsistent results..

If someone else registers the callbacks, then they have all the necessary state
information and shouldn't be relying on this class at all though right?

[Philippe, 2013-09-16 08:40:41]

lgtm, thanks Ted! Nice cleanup too.

[bulach, 2013-09-16 11:13:11]

lgtm, thanks for the added documentation!! I have one tiny nit about that, but
no concrete suggestions this time... feel free to go ahead if others are happy.

https://chromiumcodereview.appspot.com/23522032/diff/37001/base/android/java/...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/23522032/diff/37001/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:177: * Returns the
current main application activity's state (if no activity is registered,
nit: 144 says "parent activity", 156 says "current activity" and here is
"current main application"... tbh, I'm not sure which one is right :)

sActivity can be set to _any_ activity (not just "main", 108), and then once
that is destroyed (131), it can have become anything again at 108...

not sure how to best explain this though... is there a real need for
"isActive()" ? wouldn't it be simpler to let callers do
getStateForActivity(fooo) == RESUMED ? it'd force understanding the
documentation on 185-222 and rely less on the internal implementation about
sActivity, wdyt?

[Ted C, 2013-09-16 16:00:09]

https://codereview.chromium.org/23522032/diff/37001/base/android/java/src/org...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://codereview.chromium.org/23522032/diff/37001/base/android/java/src/org...
base/android/java/src/org/chromium/base/ActivityStatus.java:177: * Returns the
current main application activity's state (if no activity is registered,
On 2013/09/16 11:13:12, bulach wrote:
> nit: 144 says "parent activity", 156 says "current activity" and here is
> "current main application"... tbh, I'm not sure which one is right :)
> 
> sActivity can be set to _any_ activity (not just "main", 108), and then once
> that is destroyed (131), it can have become anything again at 108...

I updated all these (that I could find) to simply say current in hopes that is
clearer.

> 
> not sure how to best explain this though... is there a real need for
> "isActive()" ? wouldn't it be simpler to let callers do
> getStateForActivity(fooo) == RESUMED ? it'd force understanding the
> documentation on 185-222 and rely less on the internal implementation about
> sActivity, wdyt?

Not all calls will keep a strong activity reference (and often they should not,
like the LocationProvider since it's a singleton), so some places will still
need to rely on getState().

That being said, I don't think we really need isActive() unless we come to see
that there is a common pattern used everywhere.

And I've thought more about this over the weekend, I suspect just checking
RESUMED is wrong in most cases.  The one nit that has bitten us multiple times
recently is that PAUSED means that our activity is still visible on the screen
but just covered up by some other activity (but not entirely, i.e. a dialog). 
This means that we disable things on pause, we might see odd behavior in the
background.

I'm fine with removing it for now though and we can revisit this later if we
need to.

[commit-bot: I haz the power, 2013-09-16 18:54:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/23522032/10002

[commit-bot: I haz the power, 2013-09-16 22:03:21]

Change committed as 223437