Add base/android/activity_status.cc

Add base/android/activity_status.cc

This patch adds a small thread-safe wrapper around the Java
org.chromium.base.ActivityStatus that can be used to listen
to activity state changes from native code.

This uses a base::ObserverListThreadSafe to guarantee that
listeners can be registered on any threads, and that their
OnActivityStateChange() method will always be called on the
thread that created them.

+ Ensure both C++ and Java always use the same constants
  when it comes to ActivityState values.

R=felipeg@chromium.org, gavinp@chromium.org, pasko@google.com, pliard@chromium.org
BUG=233536

[digit1, 2013-04-25 13:36:20]

Note: I'm working on adding a decent unit test for this, but please take a look.

By the way, what would the relevant BUG number here?

[gavinp, 2013-04-25 13:52:37]

On 2013/04/25 13:36:20, digit1 wrote:
> Note: I'm working on adding a decent unit test for this, but please take a
look.
> 
> By the way, what would the relevant BUG number here?

https://code.google.com/p/chromium/issues/detail?id=233536 .

I updated the description.

[digit1, 2013-04-25 17:00:47]

Thanks, the second patch adds a single-threaded unit test that actually found a
real bug :-p. Hopefully will add a multi-threaded test tomorrow, but please take
a look.

[felipeg_google, 2013-04-25 18:16:08]

Are you sure that we want to use a singleton ?
I mean, I know that the ActivityStatus is never instantiated more than once.
But I just think that a leaky singleton is a poor solution for the problem
on the destructor race condition

Anyway,
Besides that, I much prefer this solution of having the generic native
ActivityStatus than my old solution of having an specific native listener
for our cache.



On Thu, Apr 25, 2013 at 7:00 PM, <digit@chromium.org> wrote:

> Thanks, the second patch adds a single-threaded unit test that actually
> found a
> real bug :-p. Hopefully will add a multi-threaded test tomorrow, but
> please take
> a look.
>
>
>
>
https://chromiumcodereview.**appspot.com/14373019/<https://chromiumcodereview...
>

[gavinp, 2013-04-26 07:37:53]

All nits.

LGTM.

You'll have to add a base/android/OWNER or base/OWNER reviewer to land this.

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
File base/android/activity_status.cc (right):

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status.cc:9: #include "base/memory/singleton.h"
Not needed, the header already has this.

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status.cc:29: ActivityStatus::ActivityStatus()
Method order: methods in .cc files should be in the same order as in the .h
file.

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
File base/android/activity_status.h (right):

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status.h:12: #include "base/callback.h"
Not needed. Favour "base/callback_forward.h" in headers if it is needed.

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status.h:33: // will always ne called from the thread that
created them.
nit "always be called"

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status.h:56: ~ActivityStatus();
~ActivityStatus() isn't defined anywhere. It's not instantiated either, as it's
leaky, but would it make sense to use an empty inline body? Our style guide
cautions against this, and the scoped_refptr cautions me too.

If we don't define a body for this, perhaps we should just comment on that?

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
File base/android/activity_status_unittest.cc (right):

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status_unittest.cc:12: namespace base {
Blank line here.

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status_unittest.cc:21: namespace {
Blank line here.

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status_unittest.cc:50: }
Blank line above this, and comment: // namespace

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status_unittest.cc:53: JNIEnv* env =
base::android::AttachCurrentThread();
#include <jni.h> ?

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status_unittest.cc:54: DCHECK(env != NULL);
Don't DCHECK in unit tests. Either ASSERT or EXPECT. This looks like an ASSERT.
It's best to ASSERT from the top level (due to gtest annoyingness), so perhaps
this function could return a bool, which could be ASSERT_TRUE in the caller?

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status_unittest.cc:76: typedef
scoped_ptr<StoringStateListener> ScopedStoringStateListener;
This is never used. I prefer the naked scoped_ptr definition, anyway, so I
suggest dropping this.

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status_unittest.cc:89: ASSERT_TRUE(listener.get() !=
NULL);
ASSERT_TRUE(listener);

Looks cleaner to me, but that may just be taste.

https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
base/android/activity_status_unittest.cc:100: }  // namespace android
Blank line after this.

[felipeg, 2013-04-26 09:29:41]

LGTM

On 2013/04/26 07:37:53, gavinp wrote:
> All nits.
> 
> LGTM.
> 
> You'll have to add a base/android/OWNER or base/OWNER reviewer to land this.
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> File base/android/activity_status.cc (right):
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status.cc:9: #include "base/memory/singleton.h"
> Not needed, the header already has this.
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status.cc:29: ActivityStatus::ActivityStatus()
> Method order: methods in .cc files should be in the same order as in the .h
> file.
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> File base/android/activity_status.h (right):
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status.h:12: #include "base/callback.h"
> Not needed. Favour "base/callback_forward.h" in headers if it is needed.
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status.h:33: // will always ne called from the thread
that
> created them.
> nit "always be called"
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status.h:56: ~ActivityStatus();
> ~ActivityStatus() isn't defined anywhere. It's not instantiated either, as
it's
> leaky, but would it make sense to use an empty inline body? Our style guide
> cautions against this, and the scoped_refptr cautions me too.
> 
> If we don't define a body for this, perhaps we should just comment on that?
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> File base/android/activity_status_unittest.cc (right):
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status_unittest.cc:12: namespace base {
> Blank line here.
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status_unittest.cc:21: namespace {
> Blank line here.
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status_unittest.cc:50: }
> Blank line above this, and comment: // namespace
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status_unittest.cc:53: JNIEnv* env =
> base::android::AttachCurrentThread();
> #include <jni.h> ?
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status_unittest.cc:54: DCHECK(env != NULL);
> Don't DCHECK in unit tests. Either ASSERT or EXPECT. This looks like an
ASSERT.
> It's best to ASSERT from the top level (due to gtest annoyingness), so perhaps
> this function could return a bool, which could be ASSERT_TRUE in the caller?
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status_unittest.cc:76: typedef
> scoped_ptr<StoringStateListener> ScopedStoringStateListener;
> This is never used. I prefer the naked scoped_ptr definition, anyway, so I
> suggest dropping this.
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status_unittest.cc:89: ASSERT_TRUE(listener.get() !=
> NULL);
> ASSERT_TRUE(listener);
> 
> Looks cleaner to me, but that may just be taste.
> 
>
https://codereview.chromium.org/14373019/diff/4001/base/android/activity_stat...
> base/android/activity_status_unittest.cc:100: }  // namespace android
> Blank line after this.

[digit1, 2013-04-26 12:32:40]

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
File base/android/activity_status.cc (right):

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status.cc:9: #include "base/memory/singleton.h"
On 2013/04/26 07:37:53, gavinp wrote:
> Not needed, the header already has this.

Done.

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status.cc:29: ActivityStatus::ActivityStatus()
On 2013/04/26 07:37:53, gavinp wrote:
> Method order: methods in .cc files should be in the same order as in the .h
> file.

Done.

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
File base/android/activity_status.h (right):

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status.h:12: #include "base/callback.h"
Good point, thanks. Done.

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status.h:33: // will always ne called from the thread that
created them.
On 2013/04/26 07:37:53, gavinp wrote:
> nit "always be called"

Done.

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status.h:56: ~ActivityStatus();
Ah, I've added an empty body. I guess this won't matter in the end, the function
will be removed by the linker.

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
File base/android/activity_status_unittest.cc (right):

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status_unittest.cc:12: namespace base {
Are you sure about that? I'm asking because _all_ the other Android-specific
code I've seen tacks the lines together.

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status_unittest.cc:21: namespace {
On 2013/04/26 07:37:53, gavinp wrote:
> Blank line here.

Done.

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status_unittest.cc:50: }
I think you're mistaken, this is the end of a function (it was copy-pasted from
auto-generated output, but I've reformatted it in a later patch).

The line you're looking for is lower :-)

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status_unittest.cc:53: JNIEnv* env =
base::android::AttachCurrentThread();
Added #include "base/android/jni_android.h"

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status_unittest.cc:54: DCHECK(env != NULL);
Replaced with an ASSERT_TRUE(env). Thanks.

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status_unittest.cc:76: typedef
scoped_ptr<StoringStateListener> ScopedStoringStateListener;
On 2013/04/26 07:37:53, gavinp wrote:
> This is never used. I prefer the naked scoped_ptr definition, anyway, so I
> suggest dropping this.

Done.

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status_unittest.cc:89: ASSERT_TRUE(listener.get() !=
NULL);
On 2013/04/26 07:37:53, gavinp wrote:
> ASSERT_TRUE(listener);
> 
> Looks cleaner to me, but that may just be taste.

Done.

[digit1, 2013-04-26 12:35:04]

Hello Marcus and Jonathan, can I ask you to review the base/android/ changes in
this patch.

Hello Brett, can I ask you to review the changes under base/base.gyp[i]

Thanks in advance.

[gavinp, 2013-04-26 12:38:48]

My old LGTM still stands. Thanks!

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
File base/android/activity_status_unittest.cc (right):

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status_unittest.cc:12: namespace base {
On 2013/04/26 12:32:40, digit1 wrote:
> Are you sure about that? I'm asking because _all_ the other Android-specific
> code I've seen tacks the lines together.

I just checked. It's not required. It is allowed.

I think you should leave this consistant with the other android code. Thanks for
educating me.

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status_unittest.cc:54: DCHECK(env != NULL);
On 2013/04/26 12:32:40, digit1 wrote:
> Replaced with an ASSERT_TRUE(env). Thanks.

Since the ASSERT is inside a function, I think it won't work, so the test will
still crash. Not ideal. :(

[bulach, 2013-04-26 12:57:06]

lgtm, truly just nits.. thanks!

https://chromiumcodereview.appspot.com/14373019/diff/23001/base/android/activ...
File base/android/activity_status.h (right):

https://chromiumcodereview.appspot.com/14373019/diff/23001/base/android/activ...
base/android/activity_status.h:59: Listener(const StateChangeCallback&
callback);
nit: explicit

https://chromiumcodereview.appspot.com/14373019/diff/23001/base/android/activ...
base/android/activity_status.h:60: ~Listener();
nit: add a \n

https://chromiumcodereview.appspot.com/14373019/diff/23001/base/android/activ...
base/android/activity_status.h:65: callback_.Run(state);
nit: I think the preference is to just have decl in the header, move the impl to
the cc..

[digit1, 2013-04-26 13:25:55]

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
File base/android/activity_status_unittest.cc (right):

https://chromiumcodereview.appspot.com/14373019/diff/4001/base/android/activi...
base/android/activity_status_unittest.cc:54: DCHECK(env != NULL);
Yes, I believe that's why I wanted to DCHECK() in the first place. I've replaced
this with EXPECT_() calls and returning success/false flags from some functions.

https://chromiumcodereview.appspot.com/14373019/diff/23001/base/android/activ...
File base/android/activity_status.h (right):

https://chromiumcodereview.appspot.com/14373019/diff/23001/base/android/activ...
base/android/activity_status.h:59: Listener(const StateChangeCallback&
callback);
On 2013/04/26 12:57:06, bulach wrote:
> nit: explicit

Done.

https://chromiumcodereview.appspot.com/14373019/diff/23001/base/android/activ...
base/android/activity_status.h:60: ~Listener();
On 2013/04/26 12:57:06, bulach wrote:
> nit: add a \n

Done.

https://chromiumcodereview.appspot.com/14373019/diff/23001/base/android/activ...
base/android/activity_status.h:65: callback_.Run(state);
On 2013/04/26 12:57:06, bulach wrote:
> nit: I think the preference is to just have decl in the header, move the impl
to
> the cc..

Done.

[Philippe, 2013-04-26 13:29:02]

lgtm, thanks David!

[joth, 2013-04-26 20:29:09]

https://codereview.chromium.org/14373019/diff/37002/base/android/activity_sta...
File base/android/activity_state_list.h (right):

https://codereview.chromium.org/14373019/diff/37002/base/android/activity_sta...
base/android/activity_state_list.h:4: 
there's some standard "no mutliple include guard" comment we normally put here

https://codereview.chromium.org/14373019/diff/37002/base/android/activity_sta...
File base/android/activity_status.cc (right):

https://codereview.chromium.org/14373019/diff/37002/base/android/activity_sta...
base/android/activity_status.cc:17: : callback_(callback) {
nit: indent

https://codereview.chromium.org/14373019/diff/37002/base/android/activity_sta...
File base/android/activity_status.h (right):

https://codereview.chromium.org/14373019/diff/37002/base/android/activity_sta...
base/android/activity_status.h:16: template <typename T> struct
LeakySingletonTraits;
is this needed?

https://codereview.chromium.org/14373019/diff/37002/base/android/activity_sta...
base/android/activity_status.h:29: // A native helper class to listen to
activity state changes from
maybe helpful to spell out that these are Android "Activity" states.

also, *which* activity is this reporting? A process can host multiple
activities, or none at all.

(those comments apply to the java counterpart class too, I guess)

https://codereview.chromium.org/14373019/diff/37002/base/android/activity_sta...
base/android/activity_status.h:35: // that the callback will always be called on
the same thread than
nit: on the same thread _that was_ the one that created the lisener (?)

https://codereview.chromium.org/14373019/diff/37002/base/android/activity_sta...
File base/android/activity_status_unittest.cc (right):

https://codereview.chromium.org/14373019/diff/37002/base/android/activity_sta...
base/android/activity_status_unittest.cc:21: // This is equivalent to what the
JNI generator would output if
At first I thought this comment (specifically, "This") referred just to the
'using' statement :)

maybe move inside namespace {} block?

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

https://codereview.chromium.org/14373019/diff/37002/base/android/java/src/org...
base/android/java/src/org/chromium/base/ActivityStatus.java:122: new
Handler(mainLooper).post(new Runnable() {
simpler to just use
ThreadUtils.runOnUiThread(new Runnable() .... )

[brettw, 2013-04-27 05:16:41]

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/base.gyp#newc...
base/base.gyp:1157: 'target_name': 'base_java_activity_state',
This adds a new target. It isn't clear from the code here or from the CL
description why this is necessary. (probably a comment in the gyp would be best)

[digit1, 2013-04-29 12:22:10]

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/activ...
File base/android/activity_state_list.h (right):

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/activ...
base/android/activity_state_list.h:4: 
I've added them, but generally speaking this kind of header doesn't use guard
since it can be included multiple times by the same source file to generate
things slightly differently (e.g. once to define enums, another time to define
enum -> strings tables, etc...)

This is not the case here though.

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/activ...
File base/android/activity_status.cc (right):

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/activ...
base/android/activity_status.cc:17: : callback_(callback) {
I've applied clang-format to the new sources.

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/activ...
File base/android/activity_status.h (right):

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/activ...
base/android/activity_status.h:16: template <typename T> struct
LeakySingletonTraits;
Good catch, no it isn't, it's a remnant of a previous implementation. Removed.

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/activ...
base/android/activity_status.h:29: // A native helper class to listen to
activity state changes from
On 2013/04/26 20:29:10, joth wrote:
> maybe helpful to spell out that these are Android "Activity" states.
> 
> also, *which* activity is this reporting? A process can host multiple
> activities, or none at all.
> 
it's the current one (when active), since there can only be one per process.

> (those comments apply to the java counterpart class too, I guess)

I've updated the comments in both the Java and C++ sources.

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/activ...
base/android/activity_status.h:35: // that the callback will always be called on
the same thread than
On 2013/04/26 20:29:10, joth wrote:
> nit: on the same thread _that was_ the one that created the lisener (?)

Done.

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/activ...
File base/android/activity_status_unittest.cc (right):

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/activ...
base/android/activity_status_unittest.cc:21: // This is equivalent to what the
JNI generator would output if
Yes, I've refactored this a bit. Thanks.

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

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/android/java/...
base/android/java/src/org/chromium/base/ActivityStatus.java:122: new
Handler(mainLooper).post(new Runnable() {
Good idea, I've changed the code to use that instead.

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/14373019/diff/37002/base/base.gyp#newc...
base/base.gyp:1157: 'target_name': 'base_java_activity_state',
Thanks for the suggestion, I've added a comment to the file.

[joth, 2013-04-29 16:41:23]

replies to my comments lg2m

https://chromiumcodereview.appspot.com/14373019/diff/51001/base/android/activ...
File base/android/activity_state_list.h (right):

https://chromiumcodereview.appspot.com/14373019/diff/51001/base/android/activ...
base/android/activity_state_list.h:5: #define BASE_ANDROID_ACTIVITY_STATE_H
yeah rather than add them I was more suggesting putting in the standard
disclaimer why they're not here:

// This file intentionally does not have header guards, it's included
// inside a macro to generate enum.

[digit1, 2013-05-02 16:06:52]

I've updated the file according to joth's and brett's recommendations. Please
take a look. Thanks.

[joth, 2013-05-02 17:16:01]

base/android lgtm, although I'm seeing diff error on some files in the
most-recent patchset
https://chromiumcodereview.appspot.com/14373019/diff/58001/base/android/java/...

[digit1, 2013-05-02 17:57:42]

Thanks, I can't reproduce the multi-threaded unit test freeze locally. I've
removed a non-necessary nested RunLoop from the test. I hope this explains the
flakiness, but frankly don't know about this enough to be sure. Anyone has a
good idea?

[brettw, 2013-05-02 20:26:12]

owners lgtm

[Philippe, 2013-05-03 14:05:55]

On 2013/05/02 20:26:12, brettw wrote:
> owners lgtm

The failures we are seeing on the bots are due to the fact that the unit tests
indirectly push some callbacks to the Java UI message loop but the UI thread
never yields therefore these callbacks are never executed (which causes a
deadlock since the second thread used for testing is never notified).
Unfortunately there is no equivalent of RunLoop for the Android Java side
(AFAICT).

I uploaded a separate CL fixing this issue there (note that I'm not David :)):
https://codereview.chromium.org/14767011

The first patch set is the same as this CL (patch set #11). The second patch set
fixes the issue by bypassing the Java side for testing.

[Philippe, 2013-05-03 14:06:44]

On 2013/05/03 14:05:55, Philippe wrote:
> On 2013/05/02 20:26:12, brettw wrote:
> > owners lgtm
> 
> The failures we are seeing on the bots are due to the fact that the unit tests
> indirectly push some callbacks to the Java UI message loop but the UI thread
> never yields therefore these callbacks are never executed (which causes a
> deadlock since the second thread used for testing is never notified).
> Unfortunately there is no equivalent of RunLoop for the Android Java side
> (AFAICT).
> 
> I uploaded a separate CL fixing this issue there (note that I'm not David :)):
> https://codereview.chromium.org/14767011
> 
> The first patch set is the same as this CL (patch set #11). The second patch
set
> fixes the issue by bypassing the Java side for testing.

Also, note that I will submit the other CL shortly :)

[tfarina, 2013-05-06 14:22:16]

https://codereview.chromium.org/14373019/diff/69002/base/android/activity_sta...
File base/android/activity_status.h (right):

https://codereview.chromium.org/14373019/diff/69002/base/android/activity_sta...
base/android/activity_status.h:52: BASE_EXPORT class ActivityStatus {
I think it is more common class BASE_EXPORT, no?

https://codereview.chromium.org/14373019/diff/69002/base/android/activity_sta...
base/android/activity_status.h:56: class Listener {
could you move this into its own header file? ActivityStatusListener? We
generally try to avoid nested classes for this kind of interface.

[Philippe, 2013-05-06 14:40:02]

https://codereview.chromium.org/14373019/diff/69002/base/android/activity_sta...
File base/android/activity_status.h (right):

https://codereview.chromium.org/14373019/diff/69002/base/android/activity_sta...
base/android/activity_status.h:52: BASE_EXPORT class ActivityStatus {
On 2013/05/06 14:22:16, tfarina wrote:
> I think it is more common class BASE_EXPORT, no?

Indeed, that's why I recently landed  https://codereview.chromium.org/14948004
:)

It broke the android component build downstream.

https://codereview.chromium.org/14373019/diff/69002/base/android/activity_sta...
base/android/activity_status.h:56: class Listener {
On 2013/05/06 14:22:16, tfarina wrote:
> could you move this into its own header file? ActivityStatusListener? We
> generally try to avoid nested classes for this kind of interface.

Sure. This will be a good change. Clients don't need to pull  all these things
since they just have to instantiate Listener to listen for events. I will send
you the CL.

[Philippe, 2013-05-06 15:03:24]

On 2013/05/06 14:40:02, Philippe wrote:
>
https://codereview.chromium.org/14373019/diff/69002/base/android/activity_sta...
> File base/android/activity_status.h (right):
> 
>
https://codereview.chromium.org/14373019/diff/69002/base/android/activity_sta...
> base/android/activity_status.h:52: BASE_EXPORT class ActivityStatus {
> On 2013/05/06 14:22:16, tfarina wrote:
> > I think it is more common class BASE_EXPORT, no?
> 
> Indeed, that's why I recently landed  https://codereview.chromium.org/14948004
> :)
> 
> It broke the android component build downstream.
> 
>
https://codereview.chromium.org/14373019/diff/69002/base/android/activity_sta...
> base/android/activity_status.h:56: class Listener {
> On 2013/05/06 14:22:16, tfarina wrote:
> > could you move this into its own header file? ActivityStatusListener? We
> > generally try to avoid nested classes for this kind of interface.
> 
> Sure. This will be a good change. Clients don't need to pull  all these things
> since they just have to instantiate Listener to listen for events. I will send
> you the CL.

Please also note that this CL was already submitted as r198124. Therefore I'm
closing this issue.