Refactor ActivityStatus to not store current activity

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 df43dbebfbb9bf95df55fbb72607ecd9a1f85ff2..93144bba6b12b0a2af91de3a93d4bd2297235782 100644
--- a/base/android/java/src/org/chromium/base/ActivityStatus.java
+++ b/base/android/java/src/org/chromium/base/ActivityStatus.java
@@ -7,9 +7,11 @@ package org.chromium.base;
 import android.app.Activity;
 import android.app.Application;
 import android.app.Application.ActivityLifecycleCallbacks;
+import android.content.Context;
 import android.os.Bundle;
 
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.Map;
 
 /**
@@ -29,24 +31,59 @@ public class ActivityStatus {
     public static final int STOPPED = ActivityState.STOPPED;
     public static final int DESTROYED = ActivityState.DESTROYED;
 
+    public static final int APP_RUNNING = ApplicationState.RUNNING;
+    public static final int APP_PAUSED = ApplicationState.PAUSED;
+    public static final int APP_STOPPED = ApplicationState.STOPPED;
+    public static final int APP_DESTROYED = ApplicationState.DESTROYED;
+
+    private static Application sApplication;
+
     // Last activity that was shown (or null if none or it was destroyed).
+    // TODO(dtrainor): Add window focus listener to update this.

[Ted C, 2014/02/13 04:38:44]

aren't you doing this down below?

[David Trainor- moved to gerrit, 2014/02/14 19:13:36]

Done.

     private static Activity sActivity;
 
+    /**
+     * A map of {@link Activity} to {@link ActivityState}.  If an {@link Activity} is not present it
+     * can be assumed that it was destroyed.
+     */
     private static final Map<Activity, Integer> sActivityStates =

[Ted C, 2014/02/13 04:38:44]

Hmm...I wonder if we should combine this and the state listeners map together.

Seeing two maps with the same key always worries me...but I guess that might be
just me.

[David Trainor- moved to gerrit, 2014/02/13 05:03:55]

No it's a good idea.  I thought about it but forgot.  Will do.

             new HashMap<Activity, Integer>();
 
-    private static final ObserverList<StateListener> sStateListeners =
-            new ObserverList<StateListener>();
+    /**
+     * A set of observers for each {@link Activity}.  Observers on the {@code null} key in this map
+     * will observer all changes for all {@link Activity}s.
+     */
+    private static final Map<Activity, ObserverList<ActivityStateListener>>
+            sActivityStateListeners = new HashMap<Activity, ObserverList<ActivityStateListener>>();
+
+    /**
+     * A list of observers to be notified when the visibility state of this {@link Application}
+     * changes.  See {@link #getStateForApplication()}.
+     */
+    private static final ObserverList<ApplicationStateListener> sApplicationStateListeners =
+            new ObserverList<ApplicationStateListener>();
 
     /**
      * Interface to be implemented by listeners.
      */
-    public interface StateListener {
+    public interface ApplicationStateListener {
+        /**
+         * Called when the application's state changes.
+         * @param newState The application state.
+         */
+        public void onApplicationStateChange(int newState);
+    }
+
+    /**
+     * Interface to be implemented by listeners.
+     */
+    public interface ActivityStateListener {
         /**
          * Called when the activity's state changes.
+         * @param activity The activity that had a state change.
          * @param newState New activity state.
          */
-        public void onActivityStateChange(int newState);
+        public void onActivityStateChange(Activity activity, int newState);
     }
 
     private ActivityStatus() {}
@@ -56,10 +93,28 @@ public class ActivityStatus {
      *
      * @param application The application whose status you wish to monitor.
      */
-    public static void initialize(Application application) {
+    public static void initialize(BaseChromiumApplication application) {
+        sApplication = application;
+
+        application.registerWindowFocusChangedListener(
+                new BaseChromiumApplication.WindowFocusChangedListener() {
+            @Override
+            public void onWindowFocusChanged(Activity activity, boolean hasFocus) {
+                if (!hasFocus || activity == sActivity) return;
+
+                int state = getStateForActivity(activity);
+
+                if (state != DESTROYED && state != STOPPED) {
+                    sActivity = activity;
+                }
+
+                // TODO(dtrainor): Notify of active activity change?
+            }
+        });
+
         application.registerActivityLifecycleCallbacks(new ActivityLifecycleCallbacks() {
             @Override
-            public void onActivityCreated(Activity activity, Bundle savedInstanceState) {
+            public void onActivityCreated(final Activity activity, Bundle savedInstanceState) {
                 onStateChange(activity, CREATED);
             }
 
@@ -102,32 +157,44 @@ public class ActivityStatus {
     private static void onStateChange(Activity activity, int newState) {
         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.
-            if (sActivity == null
-                    || newState == CREATED || newState == RESUMED || newState == STARTED) {
-                sActivity = activity;
-            }
+        if (sActivity == null
+                || newState == CREATED || newState == RESUMED || newState == STARTED) {
+            sActivity = activity;
         }
 
-        if (newState != DESTROYED) {
-            sActivityStates.put(activity, newState);
-        } else {
-            sActivityStates.remove(activity);
+        int oldApplicationState = getStateForApplication();
+        sActivityStates.put(activity, newState);
+
+        // Notify all state observers that are specifically listening to this activity.
+        ObserverList<ActivityStateListener> listeners = sActivityStateListeners.get(activity);
+        if (listeners != null) {
+            for (ActivityStateListener listener : listeners) {
+                listener.onActivityStateChange(activity, newState);
+            }
         }
 
-        if (sActivity == activity) {
-            for (StateListener listener : sStateListeners) {
-                listener.onActivityStateChange(newState);
+        // Notify all state observers that are listening globally for all activity state
+        // changes.
+        ObserverList<ActivityStateListener> globalListeners = sActivityStateListeners.get(null);
+        if (globalListeners != null) {
+            for (ActivityStateListener listener : globalListeners) {
+                listener.onActivityStateChange(activity, newState);
             }
-            if (newState == DESTROYED) {
-                sActivity = null;
+        }
+
+        int applicationState = getStateForApplication();
+        if (applicationState != oldApplicationState) {
+            for (ApplicationStateListener listener : sApplicationStateListeners) {
+                listener.onApplicationStateChange(applicationState);
             }
         }
+
+        if (newState == DESTROYED) {
+            sActivityStateListeners.remove(activity);
+            sActivityStates.remove(activity);
+
+            if (activity == sActivity) sActivity = null;
+        }
     }
 
     /**
@@ -138,18 +205,18 @@ public class ActivityStatus {
     }
 
     /**
-     * @return The current activity.
+     * @return The first visible {@link Activity} tracked by this class.  Being visible is defined

[bulach, 2014/02/13 11:17:48]

hmm.. I wish we could call this "focused", but I suppose the "tracked by this
class" part would make it a lie.. :)

my concern is that "getVisibleActivity" should ideally return a list..
perhaps being more explicit? getFirstVisibleActivity would be better?

or how about "getLastTrackedFocusedActivity()", would that be wrong / too
mouthful?

[David Trainor- moved to gerrit, 2014/02/14 19:13:36]

Yeah I used to return a list from this.  But I found that every consumer either
just wanted the first element (which means they really should be refactored...).
 Ideally this method could go away in the future.  I'll rename it for now since
it's somewhat temporary.

+     * as being in a state of created, started, or resumed.

[Ted C, 2014/02/13 04:38:44]

indent to be aligned with "The" above

Also since you are listening for window focus, this comment seems to be a bit
off.

This will actually return the most visible activity: The activity with window
focus -> the visible activity w/o focus -> the activity that was last visible.

[David Trainor- moved to gerrit, 2014/02/14 19:13:36]

I updated the comment.  I think I understand your flow but let me know if it
still doesn't capture what you want.

      */
-    public static Activity getActivity() {
+    public static Activity getVisibleActivity() {
         return sActivity;
     }
 
     /**
-     * @return The current activity's state (if no activity is registered, then DESTROYED will
-     *         be returned).
+     * @return The {@link Context} for the {@link Application}.
      */
-    public static int getState() {
-        return getStateForActivity(sActivity);
+    public static Context getApplicationContext() {
+        return sApplication != null ? sApplication.getApplicationContext() : null;
     }
 
     /**
@@ -189,7 +256,7 @@ public class ActivityStatus {
      * </ul>
      *
      * @param activity The activity whose state is to be returned.
-     * @return The state of the specified activity.
+     * @return The state of the specified activity (see {@link ActivityState}).
      */
     public static int getStateForActivity(Activity activity) {
         Integer currentStatus = sActivityStates.get(activity);
@@ -197,19 +264,92 @@ public class ActivityStatus {
     }
 
     /**
-     * Registers the given listener to receive activity state changes.
+     * @return The state of the application (see {@link ApplicationState}).
+     */
+    public static int getStateForApplication() {

[bulach, 2014/02/13 11:17:48]

hmm, I'm not sure, but I think it's bit unnatural to aggregate the state of the
activities as an "application state"...
again, would it be possible to be more explicit?
hasAnyRunningActivity, hasOnlyPausedActivities, has onlyStoppedActivities?

[David Trainor- moved to gerrit, 2014/02/14 19:13:36]

Sounds reasonable.  I changed the states to match what you recommended natively.

+        boolean running = false;
+        boolean paused = false;
+        boolean stopped = false;
+
+        for (Integer state : sActivityStates.values()) {
+            running |= state != PAUSED && state != STOPPED && state != DESTROYED;

[Ted C, 2014/02/13 04:38:44]

Seems like you can drop running and just return APP_RUNNING in this case (and
early exit the function).

Maybe something like this (checks down on the number of comparisons and looks to
accomplish the same):

boolean hasPausedActivity = false;
boolean hasStoppedActivity = false;
for (Integer state : sActivityStates.values()) {
    if (state != PAUSED && state != STOPPED && state != DESTROYED) return
APP_RUNNING;
    else if (state == PAUSED) hasPausedActivity = true;
    else if (state == STOPPED) hasStoppedActivity = true;
}

Also, how often does this get called?  Should we cache the value and mark it
dirty when we receive a new activity state changed?

[David Trainor- moved to gerrit, 2014/02/14 19:13:36]

Done.

+            paused |= state != STOPPED && state != DESTROYED;
+            stopped |= state != DESTROYED;
+        }
+
+        if (running) return APP_RUNNING;
+        if (paused) return APP_PAUSED;
+        if (stopped) return APP_STOPPED;
+        return APP_DESTROYED;
+    }
+
+    /**
+     * Checks whether or not any Activity in this Application is visible to the user.  Note that
+     * this includes the PAUSED state, which can happen when the Activity is temporarily covered
+     * by another Activity's Fragment (e.g.).
+     * @return True if the any Activity under this Application is visible, false otherwise.

[Ted C, 2014/02/13 04:38:44]

Whether any Activity ....

Then drop the "false otherwise" bit.

[David Trainor- moved to gerrit, 2014/02/14 19:13:36]

Done.

+     */
+    public static boolean isApplicationVisible() {

[bulach, 2014/02/13 11:17:48]

how about "hasAnyVisibleActivities" ?

[David Trainor- moved to gerrit, 2014/02/14 19:13:36]

Done.

+        int state = getStateForApplication();
+        return state == APP_RUNNING || state == APP_PAUSED;
+    }
+
+    /**
+     * Registers the given listener to receive state changes for all activities.
+     * @param listener Listener to receive state changes.
+     */
+    public static void registerStateListenerForAllActivities(ActivityStateListener listener) {

[Ted C, 2014/02/13 04:38:44]

Does anyone actually need to listen for all activities?

Just wondering if you should drop this support until it's needed.

[David Trainor- moved to gerrit, 2014/02/13 05:03:55]

There were one or two consumers as of when I moved everything over.  If I looked
closer I could probably rework them to listen to either a specific activity or
the application.  I'll take another look.

+        registerStateListenerForActivity(listener, null);
+    }
+
+    /**
+     * Registers the given listener to receive state changes for {@code activity}.  After a call to
+     * {@link ActivityStateListener#onActivityStateChange(Activity, int)} with
+     * {@link ActivityState#DESTROYED} all listeners associated with that particular
+     * {@link Activity} are removed.
      * @param listener Listener to receive state changes.
+     * @param activity Activity to track or {@code null} to track all activities.
      */
-    public static void registerStateListener(StateListener listener) {
-        sStateListeners.addObserver(listener);
+    public static void registerStateListenerForActivity(ActivityStateListener listener,
+            Activity activity) {
+        assert getStateForActivity(activity) != DESTROYED;
+        ObserverList<ActivityStateListener> list = sActivityStateListeners.get(activity);
+        if (list == null) {

[Ted C, 2014/02/13 04:38:44]

Personally, I would change this to always initialize the list when you see
CREATE for an activity.  Then the list would never be null and you wouldn't need
to check for it.

[David Trainor- moved to gerrit, 2014/02/14 19:13:36]

Done.

+            list = new ObserverList<ActivityStateListener>();
+            sActivityStateListeners.put(activity, list);
+        }
+        list.addObserver(listener);
     }
 
     /**
      * Unregisters the given listener from receiving activity state changes.
      * @param listener Listener that doesn't want to receive state changes.
      */
-    public static void unregisterStateListener(StateListener listener) {
-        sStateListeners.removeObserver(listener);
+    public static void unregisterActivityStateListener(ActivityStateListener listener) {
+        // Loop through all observer lists for all activities and remove the listener.  If the
+        // list is empty after removing the listener remove the activity entry as well.
+        for (Iterator<Map.Entry<Activity, ObserverList<ActivityStateListener>>> it =
+                sActivityStateListeners.entrySet().iterator(); it.hasNext();) {
+            Map.Entry<Activity, ObserverList<ActivityStateListener>> entry = it.next();
+            entry.getValue().removeObserver(listener);
+            if (!entry.getValue().iterator().hasNext()) it.remove();
+        }
+    }
+
+    /**
+     * Registers the given listener to receive state changes for the application.
+     * @param listener Listener to receive state state changes.
+     */
+    public static void registerApplicationStateListener(ApplicationStateListener listener) {
+        sApplicationStateListeners.addObserver(listener);
+    }
+
+    /**
+     * Unregisters the given listener from receiving state changes.
+     * @param listener Listener that doesn't want to receive state changes.
+     */
+    public static void unregisterApplicationStateListener(ApplicationStateListener listener) {
+        sApplicationStateListeners.removeObserver(listener);
     }
 
     /**
@@ -219,15 +359,14 @@ public class ActivityStatus {
      * side, hence lifecycle management is greatly simplified.
      */
     @CalledByNative
-    private static void registerThreadSafeNativeStateListener() {
+    private static void registerThreadSafeNativeApplicationStateListener() {
         ThreadUtils.runOnUiThread(new Runnable () {
             @Override
             public void run() {
-                // Register a new listener that calls nativeOnActivityStateChange.
-                sStateListeners.addObserver(new StateListener() {
+                registerApplicationStateListener(new ApplicationStateListener() {
                     @Override
-                    public void onActivityStateChange(int newState) {
-                        nativeOnActivityStateChange(newState);
+                    public void onApplicationStateChange(int newState) {
+                        nativeOnApplicationStateChange(newState);
                     }
                 });
             }
@@ -236,17 +375,18 @@ public class ActivityStatus {
 
     // Called to notify the native side of state changes.
     // IMPORTANT: This is always called on the main thread!
-    private static native void nativeOnActivityStateChange(int newState);
+    private static native void nativeOnApplicationStateChange(int newState);
 
     /**
-     * Checks whether or not the Application's current Activity is visible to the user.  Note that
-     * this includes the PAUSED state, which can happen when the Activity is temporarily covered
-     * by another Activity's Fragment (e.g.).
-     * @return True if the Activity is visible, false otherwise.
+     * Checks wehther or not the Application is active.  This means Activity#onStart() has been
+     * called but Activity#onStop() has not.
+     * @return Whether or not the application is active.
      */
-    public static boolean isApplicationVisible() {
-        int state = getState();
-        return state != STOPPED && state != DESTROYED;
+    public static boolean isApplicationActive() {

[Ted C, 2014/02/13 04:38:44]

Active is still an odd sounding name.

Maybe Foreground is better?

[David Trainor- moved to gerrit, 2014/02/13 05:03:55]

Oops this might be an old function I can erase... sorry.

[David Trainor- moved to gerrit, 2014/02/14 19:13:36]

Done.

+        for (Integer state : sActivityStates.values()) {
+            if (state == RESUMED || state == STARTED) return true;

[Ted C, 2014/02/13 04:38:44]

can you not use the application state here?

Also, move up to isApplicationVisible?

[David Trainor- moved to gerrit, 2014/02/14 19:13:36]

Done.

+        }
+        return false;
     }
 
     /**