Refactor ActivityStatus to not store current activity

Refactor ActivityStatus to not store current

- Refactor ActivityStatus to expose application level visibility over activity visbility.
- Add a listener for the visibility of the Application (are any Activities visible?)

BUG=341231
NOTRY=true

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

[David Trainor- moved to gerrit, 2014-02-11 04:14:51]

ptal for initial comments on the change.

[David Trainor- moved to gerrit, 2014-02-12 18:03:09]

Updated based on in person comments.

[Ted C, 2014-02-13 04:38:43]

+bulach for an extra set of eyes and for ensure my nits/comments are not
draconian

https://codereview.chromium.org/159173002/diff/130001/base/android/activity_s...
File base/android/activity_status.h (right):

https://codereview.chromium.org/159173002/diff/130001/base/android/activity_s...
base/android/activity_status.h:40: //    void OnApplicationStateChange(bool
foreground, bool active) {
update since it's no longer two booleans

same in the above comment

https://codereview.chromium.org/159173002/diff/130001/base/android/activity_s...
base/android/activity_status.h:53: class BASE_EXPORT ActivityStatus {
This should probably be called ApplicationStatus, but I guess I don't want to
rock the boat too much.

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:42: //
TODO(dtrainor): Add window focus listener to update this.
aren't you doing this down below?

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:49: private static
final Map<Activity, Integer> sActivityStates =
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.

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:209: * as being in a
state of created, started, or resumed.
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.

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:275: running |=
state != PAUSED && state != STOPPED && state != DESTROYED;
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?

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:290: * @return True
if the any Activity under this Application is visible, false otherwise.
Whether any Activity ....

Then drop the "false otherwise" bit.

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:301: public static
void registerStateListenerForAllActivities(ActivityStateListener listener) {
Does anyone actually need to listen for all activities?

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

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:317: if (list ==
null) {
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.

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:385: public static
boolean isApplicationActive() {
Active is still an odd sounding name.

Maybe Foreground is better?

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:387: if (state ==
RESUMED || state == STARTED) return true;
can you not use the application state here?

Also, move up to isApplicationVisible?

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/159173002/diff/130001/base/android/java/src/o...
base/android/java/src/org/chromium/base/BaseChromiumApplication.java:59: public
void onActivityResumed(Activity activity) { }
I would add some asserts in a couple of these methods to ensure no one has
overridden the window callbacks with something you don't expect.

Right now, there is nothing to keep someone from calling the setCallback and us
never hearing about window focus again.

https://codereview.chromium.org/159173002/diff/130001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java
(right):

https://codereview.chromium.org/159173002/diff/130001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:147:
if (!(context instanceof Activity)) {
combine this with the null check above since null isn't an instanceof Activity
either.

https://codereview.chromium.org/159173002/diff/130001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:153:

one too many blank lines

https://codereview.chromium.org/159173002/diff/130001/chrome/browser/ui/andro...
File chrome/browser/ui/android/ssl_client_certificate_request.cc (right):

https://codereview.chromium.org/159173002/diff/130001/chrome/browser/ui/andro...
chrome/browser/ui/android/ssl_client_certificate_request.cc:49: if (!window) {
Since you DCHECK window below, you should be able to remove this.  If you have a
DCHECK, you aren't supposed to try and handle the failure case (despite that
being my thinking several months ago)

https://codereview.chromium.org/159173002/diff/130001/ui/base/android/window_...
File ui/base/android/window_android.h (right):

https://codereview.chromium.org/159173002/diff/130001/ui/base/android/window_...
ui/base/android/window_android.h:27: base::android::ScopedJavaLocalRef<jobject>
GetJavaObject() const;
Hmm...why is this const?

Allowing const to access java seems like a really easy way to avoid const guards
(and not to mention java has no concept of const so you can get the java object
and call any sort of mutators you'd like).

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

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:49: private static
final Map<Activity, Integer> sActivityStates =
On 2014/02/13 04:38:44, Ted C wrote:
> 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.

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

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:301: public static
void registerStateListenerForAllActivities(ActivityStateListener listener) {
On 2014/02/13 04:38:44, Ted C wrote:
> Does anyone actually need to listen for all activities?
> 
> Just wondering if you should drop this support until it's needed.

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.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:385: public static
boolean isApplicationActive() {
On 2014/02/13 04:38:44, Ted C wrote:
> Active is still an odd sounding name.
> 
> Maybe Foreground is better?

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

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
(right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/BaseChromiumApplication.java:59: public
void onActivityResumed(Activity activity) { }
On 2014/02/13 04:38:44, Ted C wrote:
> I would add some asserts in a couple of these methods to ensure no one has
> overridden the window callbacks with something you don't expect.
> 
> Right now, there is nothing to keep someone from calling the setCallback and
us
> never hearing about window focus again.

Ah yes I forgot about this!  Good idea.

https://chromiumcodereview.appspot.com/159173002/diff/130001/chrome/browser/u...
File chrome/browser/ui/android/ssl_client_certificate_request.cc (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/chrome/browser/u...
chrome/browser/ui/android/ssl_client_certificate_request.cc:49: if (!window) {
On 2014/02/13 04:38:44, Ted C wrote:
> Since you DCHECK window below, you should be able to remove this.  If you have
a
> DCHECK, you aren't supposed to try and handle the failure case (despite that
> being my thinking several months ago)

Yeah I thought about that too, but was trying to follow the same behavior as I
saw elsewhere in this file (although now I can't find what I was mirroring). 
Will change.

https://chromiumcodereview.appspot.com/159173002/diff/130001/ui/base/android/...
File ui/base/android/window_android.h (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/ui/base/android/...
ui/base/android/window_android.h:27: base::android::ScopedJavaLocalRef<jobject>
GetJavaObject() const;
On 2014/02/13 04:38:44, Ted C wrote:
> Hmm...why is this const?
> 
> Allowing const to access java seems like a really easy way to avoid const
guards
> (and not to mention java has no concept of const so you can get the java
object
> and call any sort of mutators you'd like).

Dogh!  I passed a const WindowAndroid* through in
ssl_client_certificate_request.  I'll remove const there and remove it from
here.  That does makes more sense.  I don't know why I added const there to
begin with :/.

Although semantically I think this is fine as long as the method doesn't change
anything in WindowAndroid (which the get method does not).  If we wanted to
protect the returned object wouldn't we just prepend const to the return
parameter?  A const getter doesn't have to return an immutable object.  Although
like you said since Java could in theory be used to mutate this object and
they're supposedly representing the same thing you have a good point.

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

good stuff, I think we're very close!

I have just some suggestions below but except for the activitystatus class name,
the rest I don't feel too strongly..

whatever you two folks think about the "application state" suggestion (including
ignoring it) would work for me. ;)

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/act...
File base/android/activity_status.h (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/act...
base/android/activity_status.h:53: class BASE_EXPORT ActivityStatus {
On 2014/02/13 04:38:44, Ted C wrote:
> This should probably be called ApplicationStatus, but I guess I don't want to
> rock the boat too much.

yes, ApplicationStatus please :)
imho, either:
- keep the ActivityStatus temporarily whilst introducing the new
ApplicationState (i.e., N-side patches) 
- go all the way and rename this class, this file and its tests (which I'd
prefer!).


while at it... when we implemented the MemoryPressureListener:
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/memory...

we changed the relationship slightly:
rather than having these inner-listener notified by an outer singleton, there's
only one "public" level for listeners and the singleton became an internal
detail just in the .cc...

since we'll have to change everything ;) perhaps this would be a good time?

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/app...
File base/android/application_state_list.h (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/app...
base/android/application_state_list.h:11: DEFINE_APPLICATION_STATE(RUNNING, 1)
see below, perhaps it'd be better to have something like:
HAS_ANY_RUNNING_ACTIVITIES
HAS_ONLY_PAUSED_ACTIVITIES
HAS_ONLY_STOPPED_ACTIVITIES
DESTROYED

to be more explicit this is not an application state per se, but rather a
derivation of the underlying activities? wdyt?

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:208: * @return The
first visible {@link Activity} tracked by this class.  Being visible is defined
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?

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:269: public static
int getStateForApplication() {
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?

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:292: public static
boolean isApplicationVisible() {
how about "hasAnyVisibleActivities" ?

https://chromiumcodereview.appspot.com/159173002/diff/130001/net/disk_cache/s...
File net/disk_cache/simple/simple_index.cc (left):

https://chromiumcodereview.appspot.com/159173002/diff/130001/net/disk_cache/s...
net/disk_cache/simple/simple_index.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
trigger happy! :)

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

Addressed comments.  ptal

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/act...
File base/android/activity_status.h (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/act...
base/android/activity_status.h:40: //    void OnApplicationStateChange(bool
foreground, bool active) {
On 2014/02/13 04:38:44, Ted C wrote:
> update since it's no longer two booleans
> 
> same in the above comment

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/app...
File base/android/application_state_list.h (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/app...
base/android/application_state_list.h:11: DEFINE_APPLICATION_STATE(RUNNING, 1)
On 2014/02/13 11:17:48, bulach wrote:
> see below, perhaps it'd be better to have something like:
> HAS_ANY_RUNNING_ACTIVITIES
> HAS_ONLY_PAUSED_ACTIVITIES
> HAS_ONLY_STOPPED_ACTIVITIES
> DESTROYED
> 
> to be more explicit this is not an application state per se, but rather a
> derivation of the underlying activities? wdyt?

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:42: //
TODO(dtrainor): Add window focus listener to update this.
On 2014/02/13 04:38:44, Ted C wrote:
> aren't you doing this down below?

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:208: * @return The
first visible {@link Activity} tracked by this class.  Being visible is defined
On 2014/02/13 11:17:48, bulach wrote:
> 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?

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.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:209: * as being in a
state of created, started, or resumed.
On 2014/02/13 04:38:44, Ted C wrote:
> 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.

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

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:269: public static
int getStateForApplication() {
On 2014/02/13 11:17:48, bulach wrote:
> 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?

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

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:275: running |=
state != PAUSED && state != STOPPED && state != DESTROYED;
On 2014/02/13 04:38:44, Ted C wrote:
> 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?

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:290: * @return True
if the any Activity under this Application is visible, false otherwise.
On 2014/02/13 04:38:44, Ted C wrote:
> Whether any Activity ....
> 
> Then drop the "false otherwise" bit.

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:292: public static
boolean isApplicationVisible() {
On 2014/02/13 11:17:48, bulach wrote:
> how about "hasAnyVisibleActivities" ?

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:317: if (list ==
null) {
On 2014/02/13 04:38:44, Ted C wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:385: public static
boolean isApplicationActive() {
On 2014/02/13 05:03:55, David Trainor wrote:
> On 2014/02/13 04:38:44, Ted C wrote:
> > Active is still an odd sounding name.
> > 
> > Maybe Foreground is better?
> 
> Oops this might be an old function I can erase... sorry.

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:387: if (state ==
RESUMED || state == STARTED) return true;
On 2014/02/13 04:38:44, Ted C wrote:
> can you not use the application state here?
> 
> Also, move up to isApplicationVisible?

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
(right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/base/android/jav...
base/android/java/src/org/chromium/base/BaseChromiumApplication.java:59: public
void onActivityResumed(Activity activity) { }
On 2014/02/13 05:03:55, David Trainor wrote:
> On 2014/02/13 04:38:44, Ted C wrote:
> > I would add some asserts in a couple of these methods to ensure no one has
> > overridden the window callbacks with something you don't expect.
> > 
> > Right now, there is nothing to keep someone from calling the setCallback and
> us
> > never hearing about window focus again.
> 
> Ah yes I forgot about this!  Good idea.

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java
(right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:147:
if (!(context instanceof Activity)) {
On 2014/02/13 04:38:44, Ted C wrote:
> combine this with the null check above since null isn't an instanceof Activity
> either.

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:153:

On 2014/02/13 04:38:44, Ted C wrote:
> one too many blank lines

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/chrome/browser/u...
File chrome/browser/ui/android/ssl_client_certificate_request.cc (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/chrome/browser/u...
chrome/browser/ui/android/ssl_client_certificate_request.cc:49: if (!window) {
On 2014/02/13 05:03:55, David Trainor wrote:
> On 2014/02/13 04:38:44, Ted C wrote:
> > Since you DCHECK window below, you should be able to remove this.  If you
have
> a
> > DCHECK, you aren't supposed to try and handle the failure case (despite that
> > being my thinking several months ago)
> 
> Yeah I thought about that too, but was trying to follow the same behavior as I
> saw elsewhere in this file (although now I can't find what I was mirroring). 
> Will change.

Done.

https://chromiumcodereview.appspot.com/159173002/diff/130001/net/disk_cache/s...
File net/disk_cache/simple/simple_index.cc (left):

https://chromiumcodereview.appspot.com/159173002/diff/130001/net/disk_cache/s...
net/disk_cache/simple/simple_index.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/02/13 11:17:48, bulach wrote:
> trigger happy! :)

DELETE DELETE DELETE OMNOMNOM

https://chromiumcodereview.appspot.com/159173002/diff/130001/ui/base/android/...
File ui/base/android/window_android.h (right):

https://chromiumcodereview.appspot.com/159173002/diff/130001/ui/base/android/...
ui/base/android/window_android.h:27: base::android::ScopedJavaLocalRef<jobject>
GetJavaObject() const;
On 2014/02/13 05:03:55, David Trainor wrote:
> On 2014/02/13 04:38:44, Ted C wrote:
> > Hmm...why is this const?
> > 
> > Allowing const to access java seems like a really easy way to avoid const
> guards
> > (and not to mention java has no concept of const so you can get the java
> object
> > and call any sort of mutators you'd like).
> 
> Dogh!  I passed a const WindowAndroid* through in
> ssl_client_certificate_request.  I'll remove const there and remove it from
> here.  That does makes more sense.  I don't know why I added const there to
> begin with :/.
> 
> Although semantically I think this is fine as long as the method doesn't
change
> anything in WindowAndroid (which the get method does not).  If we wanted to
> protect the returned object wouldn't we just prepend const to the return
> parameter?  A const getter doesn't have to return an immutable object. 
Although
> like you said since Java could in theory be used to mutate this object and
> they're supposedly representing the same thing you have a good point.

Done.

[bulach, 2014-02-17 12:04:00]

lgtm, good stuff! I'm happy with the overall API, but please make sure ted is
happy too!

just some minor nits.
also, it was not clear if ActivityStatus.java is going to be renamed soon, but I
hope that's part of the plan? :)

to be clear, it's totally fine to do in N patches, I just want to ensure that
application_status_listener.cc is only calling temporarily into
ActivityStatus_jni.h and they will soon be reunited again..

https://codereview.chromium.org/159173002/diff/320001/base/android/applicatio...
File base/android/application_state_list.h (right):

https://codereview.chromium.org/159173002/diff/320001/base/android/applicatio...
base/android/application_state_list.h:14:
DEFINE_APPLICATION_STATE(HAS_ONLY_DESTROYED_ACTIVITIES, 3)
is it intentional to have the same value as above?

https://codereview.chromium.org/159173002/diff/320001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://codereview.chromium.org/159173002/diff/320001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:21: public class
ActivityStatus {
is this class here just for the N side patch? perhaps add a comment saying it'll
be removed?

https://codereview.chromium.org/159173002/diff/320001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:39:
ApplicationState.HAS_ONLY_DESTROYED_ACTIVITIES;
are these needed? can't we use ApplicationState directly?

https://codereview.chromium.org/159173002/diff/320001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityStatus.java:254: * return {@link
#DESTROYED}.
nit: perhaps add a comment saying something like:

Please note that chrome can have multiple activities running 
simultaneously.
Please also look at "getStateForApplication()" below for more details.

https://codereview.chromium.org/159173002/diff/320001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
(left):

https://codereview.chromium.org/159173002/diff/320001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:127:
if (SyncStatusHelper.get(mContext).isSyncEnabled()) {
nit: what about this condition?

[Ted C, 2014-02-18 18:18:29]

lgtm (w/ bulach's comments as well)

https://codereview.chromium.org/159173002/diff/320001/base/android/applicatio...
File base/android/application_state_list.h (right):

https://codereview.chromium.org/159173002/diff/320001/base/android/applicatio...
base/android/application_state_list.h:12:
DEFINE_APPLICATION_STATE(HAS_ONLY_PAUSED_ACTIVITIES, 2)
I would drop the _ONLY_ as that is not usually correct.  It's actually the most
visible activity state, so I would just leave it as (or maybe even drop the
HAS_):

HAS_RESUMED_ACTIVITIES
HAS_PAUSED_ACTIVITES
HAS_STOPPED_ACTIVITIES
HAS_DESTROYED_ACTIVITIES

Sadly, I think this will only be clear if you read the documentation.  I don't
think we want enum names:

MOST_VISIBLE_ACTIVITY_IS_RUNNING
MOST_VISIBLE_ACTIVITY_IS_PAUSED
MOST_VISIBLE_ACTIVITY_IS_STOPPED
MOST_VISIBLE_ACTIVITY_IS_DESTROYED

https://codereview.chromium.org/159173002/diff/320001/base/android/base_jni_r...
File base/android/base_jni_registrar.cc (right):

https://codereview.chromium.org/159173002/diff/320001/base/android/base_jni_r...
base/android/base_jni_registrar.cc:31: { "ApplicationStatus",
ApplicationStatusListener

https://codereview.chromium.org/159173002/diff/320001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ActivityState.template (right):

https://codereview.chromium.org/159173002/diff/320001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ActivityState.template:10: interface
ActivityState {
TODO for deleting this file?

[David Trainor- moved to gerrit, 2014-02-18 19:54:15]

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/app...
File base/android/application_state_list.h (right):

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/app...
base/android/application_state_list.h:12:
DEFINE_APPLICATION_STATE(HAS_ONLY_PAUSED_ACTIVITIES, 2)
On 2014/02/18 18:18:29, Ted C wrote:
> I would drop the _ONLY_ as that is not usually correct.  It's actually the
most
> visible activity state, so I would just leave it as (or maybe even drop the
> HAS_):
> 
> HAS_RESUMED_ACTIVITIES
> HAS_PAUSED_ACTIVITES
> HAS_STOPPED_ACTIVITIES
> HAS_DESTROYED_ACTIVITIES
> 
> Sadly, I think this will only be clear if you read the documentation.  I don't
> think we want enum names:
> 
> MOST_VISIBLE_ACTIVITY_IS_RUNNING
> MOST_VISIBLE_ACTIVITY_IS_PAUSED
> MOST_VISIBLE_ACTIVITY_IS_STOPPED
> MOST_VISIBLE_ACTIVITY_IS_DESTROYED

Done.

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/app...
base/android/application_state_list.h:14:
DEFINE_APPLICATION_STATE(HAS_ONLY_DESTROYED_ACTIVITIES, 3)
On 2014/02/17 12:04:01, bulach wrote:
> is it intentional to have the same value as above?

Done.

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/bas...
File base/android/base_jni_registrar.cc (right):

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/bas...
base/android/base_jni_registrar.cc:31: { "ApplicationStatus",
On 2014/02/18 18:18:29, Ted C wrote:
> ApplicationStatusListener

Done.

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/jav...
File base/android/java/src/org/chromium/base/ActivityState.template (right):

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityState.template:10: interface
ActivityState {
On 2014/02/18 18:18:29, Ted C wrote:
> TODO for deleting this file?

Done.

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/jav...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:21: public class
ActivityStatus {
On 2014/02/17 12:04:01, bulach wrote:
> is this class here just for the N side patch? perhaps add a comment saying
it'll
> be removed?

Done.

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:39:
ApplicationState.HAS_ONLY_DESTROYED_ACTIVITIES;
On 2014/02/17 12:04:01, bulach wrote:
> are these needed? can't we use ApplicationState directly?

Done.

https://chromiumcodereview.appspot.com/159173002/diff/320001/base/android/jav...
base/android/java/src/org/chromium/base/ActivityStatus.java:254: * return {@link
#DESTROYED}.
On 2014/02/17 12:04:01, bulach wrote:
> nit: perhaps add a comment saying something like:
> 
> Please note that chrome can have multiple activities running 
> simultaneously.
> Please also look at "getStateForApplication()" below for more details.

Done.

https://chromiumcodereview.appspot.com/159173002/diff/320001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
(left):

https://chromiumcodereview.appspot.com/159173002/diff/320001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:127:
if (SyncStatusHelper.get(mContext).isSyncEnabled()) {
On 2014/02/17 12:04:01, bulach wrote:
> nit: what about this condition?

Ah messed this up in a rebase.  Will fix thanks.

[David Trainor- moved to gerrit, 2014-02-19 23:01:41]

boliu@: Can you review android_webview changes?
darin@: Can you review base.gyp/base.gypi?
yfriedman@: Can you review net/android and sync/android changes?
pasko@: Can you review net/disk_cache/simple changes?

Thanks!

[boliu, 2014-02-19 23:13:06]

android_webview lgtm

[Yaron, 2014-02-20 05:48:46]

net/android and sync/android lgtm. I trust Marcus/Ted for the others

[David Trainor- moved to gerrit, 2014-02-20 19:02:04]

Dogh forgot to add you to the reviewer list!

darin@: Can you review base.gyp(i) changes?
gavinp@: Can you review the simple_index changes?  Just saw you actually touched
the relevant code last.  Thanks!

[darin (slow to review), 2014-02-20 19:03:59]

base/base.gyp* LGTM

[Ryan Hamilton, 2014-02-21 18:49:58]

net/ LGTM

[David Trainor- moved to gerrit, 2014-02-21 19:04:43]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-02-21 19:04:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/159173002/1180001

[commit-bot: I haz the power, 2014-02-21 19:16:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/159173002/1180001

[commit-bot: I haz the power, 2014-02-21 21:40:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 21:40:23]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[David Trainor- moved to gerrit, 2014-02-21 22:57:27]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-02-21 23:01:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/159173002/1180001

[commit-bot: I haz the power, 2014-02-21 23:04:11]

Change committed as 252679

[pasko, 2014-02-24 14:43:30]

sorry for late response, was on vacation.

https://chromiumcodereview.appspot.com/159173002/diff/1180001/net/disk_cache/...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/159173002/diff/1180001/net/disk_cache/...
net/disk_cache/simple/simple_index.cc:463: if (state ==
base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) {
app_on_background_ should be set to false when HAS_PAUSED_ACTIVITIES because in
this case we are still on foreground. This would match hasVisibleActivities().

I have a more general question: why do we ever need to subscribe to
onPause()/onResume() in chrome?