Change method of activity tracking done in ActivityStatus.

base/android/java/src/org/chromium/base/ActivityStatus.java

Index: base/android/java/src/org/chromium/base/ActivityStatus.java
diff --git a/base/android/java/src/org/chromium/base/ActivityStatus.java b/base/android/java/src/org/chromium/base/ActivityStatus.java
index 47472342b20946466c13268558b9616c12d1d625..56903a52bf30055f8733ed27a5480aa09c86c7f1 100644
--- a/base/android/java/src/org/chromium/base/ActivityStatus.java
+++ b/base/android/java/src/org/chromium/base/ActivityStatus.java
@@ -5,8 +5,14 @@
 package org.chromium.base;
 
 import android.app.Activity;
-import android.os.Handler;
-import android.os.Looper;
+import android.app.Application;
+import android.app.Application.ActivityLifecycleCallbacks;
+import android.os.Bundle;
+
+import com.google.common.annotations.VisibleForTesting;
+
+import java.util.HashMap;
+import java.util.Map;
 
 /**
  * Provides information about the current activity's status, and a way
@@ -28,9 +34,7 @@ public class ActivityStatus {
     // Current main activity, or null if none.

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

Current visible activity?  Last activity that was shown?

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

Done.

     private static Activity sActivity;
 
-    // Current main activity's state. This can be set even if sActivity is null, to simplify unit
-    // testing.
-    private static int sActivityState;
+    private static Map<Activity, Integer> sActivityStates = new HashMap<Activity, Integer>();

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

Nit: did you omit the 'final' so that this can fit on a single line? :)

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

Nope, that was an accident.  Added final

 
     private static final ObserverList<StateListener> sStateListeners =
             new ObserverList<StateListener>();
@@ -49,33 +53,113 @@ public class ActivityStatus {
     private ActivityStatus() {}
 
     /**
+     * Initializes the activity status for a specified application.
+     *
+     * @param application The application whose status you wish to monitor.
+     */
+    public static void initialize(Application application) {
+        application.registerActivityLifecycleCallbacks(new ActivityLifecycleCallbacks() {
+            @Override
+            public void onActivityCreated(Activity activity, Bundle savedInstanceState) {
+                onStateChange(activity, CREATED);
+            }
+
+            @Override
+            public void onActivityDestroyed(Activity activity) {
+                onStateChange(activity, DESTROYED);

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

should this call unregisterActivityLifecycleCallbacks(this); as well?

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

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

+            }
+
+            @Override
+            public void onActivityPaused(Activity activity) {
+                onStateChange(activity, PAUSED);
+            }
+
+            @Override
+            public void onActivityResumed(Activity activity) {
+                onStateChange(activity, RESUMED);
+            }
+
+            @Override
+            public void onActivitySaveInstanceState(Activity activity, Bundle outState) {}
+
+            @Override
+            public void onActivityStarted(Activity activity) {
+                onStateChange(activity, STARTED);
+            }
+
+            @Override
+            public void onActivityStopped(Activity activity) {
+                onStateChange(activity, STOPPED);
+            }
+        });
+    }
+
+    /**
      * Must be called by the main activity when it changes state.
+     *
+     * <p>
+     * This is exposed purely for testing as normal usage should rely on the internal behavior
+     * setup during {@link #initialize(Application)}
+     *
      * @param activity Current activity.
      * @param newState New state value.
      */
+    @VisibleForTesting
     public static void onStateChange(Activity activity, int newState) {

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

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?
:)

[aberent, 2013/09/13 15:25:44]

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]

Sounds good to me.  I added TODO (and marked it deprecated) to make it private
once the downstream usages change.
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).

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

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

+        if (activity == null) throw new IllegalArgumentException("null activity is not supported");
+
         if (sActivity != activity) {
             // ActivityStatus is notified with the CREATED event very late during the main activity
             // creation to avoid making startup performance worse than it is by notifying observers
             // that could do some expensive work. This can lead to non-CREATED events being fired
             // before the CREATED event which is problematic.
             // TODO(pliard): fix http://crbug.com/176837.
-            sActivity = activity;
+            if (sActivity == null
+                    || newState == CREATED || newState == RESUMED || newState == STARTED) {
+                sActivity = activity;
+            }
         }
-        sActivityState = newState;
-        for (StateListener listener : sStateListeners) {
-            listener.onActivityStateChange(newState);
+
+        if (newState != DESTROYED) {
+            sActivityStates.put(activity, newState);
+        } else {
+            sActivityStates.remove(activity);
         }
-        if (newState == DESTROYED) {
-            sActivity = null;
+
+        if (sActivity == activity) {
+            for (StateListener listener : sStateListeners) {
+                listener.onActivityStateChange(newState);
+            }
+            if (newState == DESTROYED) {
+                sActivity = null;
+            }
         }
     }
 
     /**
      * Indicates that the parent activity is currently paused.
+     *
+     * Use {@link #isActive()} instead.
      */
+    @Deprecated
     public static boolean isPaused() {
-        return sActivityState == PAUSED;
+        if (sActivity == null) return false;
+        Integer currentStatus = sActivityStates.get(sActivity);
+        return currentStatus != null && currentStatus.intValue() == PAUSED;
+    }
+
+    /**
+     * @return Whether the current active is considered to be "active".  An activity is only

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

Nit: s/active/activity?

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

Done.

+     *         determined to be active if the last observed state is {@link #RESUMED}.  Other
+     *         states imply the activity is either starting up {@link #CREATED} and
+     *         {@link #STARTED}, or that the activity is not the foreground activity
+     *         {@link #PAUSED} or {@link #STOPPED}, or that the activity is being destructed
+     *         {@link #DESTROYED}.
+     */
+    public static boolean isActive() {
+        if (sActivity == null) return false;
+        Integer currentStatus = sActivityStates.get(sActivity);
+        return currentStatus != null && currentStatus.intValue() == RESUMED;
     }
 
     /**
@@ -86,10 +170,23 @@ public class ActivityStatus {
     }
 
     /**
-     * Returns the current main application activity's state.
+     * Returns the current main application activity's state (if no activity is registered,
+     * then DESTROYED will be returned).
      */
     public static int getState() {
-        return sActivityState;
+        return getStateForActivity(sActivity);
+    }
+
+    /**
+     * Query the state for a given activity.  If the activity is being tracked, this will

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

Nit: s/being/not being?

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

Done.

+     * return {@link #DESTROYED}
+     *
+     * @param activity The activity whose state is to be returned.
+     * @return The state of the specified activity.

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

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

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

I tried to add documentation about expected lifecycles.  Let me know if it's
clear.
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?

+     */
+    public static int getStateForActivity(Activity activity) {
+        Integer currentStatus = sActivityStates.get(activity);
+        return currentStatus != null ? currentStatus.intValue() : DESTROYED;
     }
 
     /**