android: Improve ActivityStatus and add ChromiumActivity.

android: Improve ActivityStatus and add ChromiumActivity.

The ActivityStatus class used to track activity state changes is too basic
for some usage scenarios.

This patch does the following:

- Augment ActivityStatus with a new StateListener type that can be
  registered to listen to all activity state changes.

  Also add getActivity(), getState(), registerStateListener()
  and unregisterStateListener() as static method.

- Add a new ChromiumActivity class that all Chromium main activities
  should sub-class, to ensure that the ActivityStatus state is updated
  appropriately.

- Modify all main activities in the Chromium code base to extend
  from ChromiumActivity instead of Activity.

BUG=none


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

[digit1, 2012-12-04 15:36:07]

ping

[digit1, 2012-12-04 15:41:55]

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

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:76: public void
onActivityStatusChanged(boolean isPaused);
Note that I'm pretty sure that the Android convention is to avoid the past tense
here, i.e. this should be called "onActivityStatusChange" instead. However, I
didn't want to update this here for backwards compatibility.

https://chromiumcodereview.appspot.com/11419287/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java
(right):

https://chromiumcodereview.appspot.com/11419287/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java:53:
if (state == ActivityStatus.PAUSED) {
I suspect this should really be STOPPED / STARTED instead. The PAUSED state
means the activity is still visible, e.g. there is a dialog on top of it.

The feature I'm working on (SSL client certificate support) does exactly this,
i.e. it prompts the user through a system UI dialog to select a certificate.
This will definitely put the activity in the paused state. However, I don't see
why we should stop the LocationListener in this case.

The STOPPED state corresponds to the case where the activity is not visible
anymore (and even there, do we want to stop the listener if, say, we display the
settings activity instead of the main one).

I didn't want to address this in this patch, but I'm curious about your opinions
on this topic.

https://chromiumcodereview.appspot.com/11419287/diff/1/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://chromiumcodereview.appspot.com/11419287/diff/1/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:76:
if (ActivityStatus.getState() != ActivityStatus.PAUSED) {
Same remark here, do we really want to stop this on pause?

[digit1, 2012-12-04 15:45:34]

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

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:36: // List of
deprecated pause/resume listeners.
Ignore the "deprecated" here, I'll remove it.

[Philippe, 2012-12-04 16:12:42]

Thanks David and sorry for the nits :)

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

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:18: //
StateListener.onStateChange
Nit: I think this can fit on the line above. Generally we tend to put as much
content as possible on lines (100 char limit in Java). I don't know if your
editor can do this automatically.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:37: private
ArrayList<Listener> mListeners = new ArrayList<Listener>();
Nit: you can make this field and the one below 'final'.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:49: sActivity =
activity;
Nit: 4 space indentation here and in some other places (e.g. line 55).

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:71: public interface
Listener {
Nit: Maybe add a comment here to say that this listener is kept for backwards
compatibility or even that it is deprecated if clients can later switch to the
StateListener change interface. Note that there is a @Deprecated annotation in
case this can be considered as deprecated.

This would help the reader understand why there are two similar listener
interfaces.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:83: * @param
activity Current activity.
Nit: It seems that you got rid of |activity| and |oldState| after you wrote this
comment.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:94: sInstance = new
ActivityStatus();
Nit: as a bonus you could add a protected constructor so that clients don't
instantiate ActivityStatus without subclassing it if this class is not meant to
be used directly (needs to be confirmed though).

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:123: static public
Activity getActivity() {
Nit: I think 'public static' is more common.

https://chromiumcodereview.appspot.com/11419287/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java
(right):

https://chromiumcodereview.appspot.com/11419287/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java:53:
if (state == ActivityStatus.PAUSED) {
On 2012/12/04 15:41:55, digit1 wrote:
> I suspect this should really be STOPPED / STARTED instead. The PAUSED state
> means the activity is still visible, e.g. there is a dialog on top of it.
> 
> The feature I'm working on (SSL client certificate support) does exactly this,
> i.e. it prompts the user through a system UI dialog to select a certificate.
> This will definitely put the activity in the paused state. However, I don't
see
> why we should stop the LocationListener in this case.
> 
> The STOPPED state corresponds to the case where the activity is not visible
> anymore (and even there, do we want to stop the listener if, say, we display
the
> settings activity instead of the main one).
> 
> I didn't want to address this in this patch, but I'm curious about your
opinions
> on this topic.

This seems too aggressive indeed. Let's see what the others think.

[digit1, 2012-12-04 17:31:22]

thanks for the comments, I've addressed them in the latest patch

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

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:18: //
StateListener.onStateChange
On 2012/12/04 16:12:42, Philippe wrote:
> Nit: I think this can fit on the line above. Generally we tend to put as much
> content as possible on lines (100 char limit in Java). I don't know if your
> editor can do this automatically.

Done.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:36: // List of
deprecated pause/resume listeners.
On 2012/12/04 15:45:34, digit1 wrote:
> Ignore the "deprecated" here, I'll remove it.

Done.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:37: private
ArrayList<Listener> mListeners = new ArrayList<Listener>();
On 2012/12/04 16:12:42, Philippe wrote:
> Nit: you can make this field and the one below 'final'.

Done.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:49: sActivity =
activity;
On 2012/12/04 16:12:42, Philippe wrote:
> Nit: 4 space indentation here and in some other places (e.g. line 55).

Done.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:71: public interface
Listener {
On 2012/12/04 16:12:42, Philippe wrote:
> Nit: Maybe add a comment here to say that this listener is kept for backwards
> compatibility or even that it is deprecated if clients can later switch to the
> StateListener change interface. Note that there is a @Deprecated annotation in
> case this can be considered as deprecated.
> 
> This would help the reader understand why there are two similar listener
> interfaces.

Done.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:83: * @param
activity Current activity.
On 2012/12/04 16:12:42, Philippe wrote:
> Nit: It seems that you got rid of |activity| and |oldState| after you wrote
this
> comment.

Done.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:94: sInstance = new
ActivityStatus();
On 2012/12/04 16:12:42, Philippe wrote:
> Nit: as a bonus you could add a protected constructor so that clients don't
> instantiate ActivityStatus without subclassing it if this class is not meant
to
> be used directly (needs to be confirmed though).

Done.

https://chromiumcodereview.appspot.com/11419287/diff/1/base/android/java/src/...
base/android/java/src/org/chromium/base/ActivityStatus.java:123: static public
Activity getActivity() {
On 2012/12/04 16:12:42, Philippe wrote:
> Nit: I think 'public static' is more common.

Done.

[Philippe, 2012-12-04 17:49:21]

lgtm

Thanks David!

https://chromiumcodereview.appspot.com/11419287/diff/1014/base/android/java/s...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/11419287/diff/1014/base/android/java/s...
base/android/java/src/org/chromium/base/ActivityStatus.java:53: sActivity =
activity;
Nit: I don't know what the general consensus is about single statement
if/for/while statements and the use of braces in Chromium Java code.

Personally I prefer omitting the braces as you did. The only thing is that this
file is now using the two ways of doing.

https://chromiumcodereview.appspot.com/11419287/diff/1014/base/android/java/s...
base/android/java/src/org/chromium/base/ActivityStatus.java:74: // This
interface can only be used to listen to PAUSED and RESUMED
Nit: I think that Javadoc comments are preferred at the class/interface/method
level. I'm not sure this is true in Chromium though. The other reviewers can
clarify this.

[joth, 2012-12-04 22:42:08]

LG for the files I commented on. (didn't read it all)

https://chromiumcodereview.appspot.com/11419287/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java
(right):

https://chromiumcodereview.appspot.com/11419287/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java:53:
if (state == ActivityStatus.PAUSED) {
On 2012/12/04 16:12:42, Philippe wrote:
> On 2012/12/04 15:41:55, digit1 wrote:
> > I suspect this should really be STOPPED / STARTED instead. The PAUSED state
> > means the activity is still visible, e.g. there is a dialog on top of it.
> > 
> > The feature I'm working on (SSL client certificate support) does exactly
this,
> > i.e. it prompts the user through a system UI dialog to select a certificate.
> > This will definitely put the activity in the paused state. However, I don't
> see
> > why we should stop the LocationListener in this case.
> > 
> > The STOPPED state corresponds to the case where the activity is not visible
> > anymore (and even there, do we want to stop the listener if, say, we display
> the
> > settings activity instead of the main one).
> > 
> > I didn't want to address this in this patch, but I'm curious about your
> opinions
> > on this topic.
> 
> This seems too aggressive indeed. Let's see what the others think.

+1 ISTM that STOPPED makes more sense here. copying in bulach & jknotten for
opinions.

https://chromiumcodereview.appspot.com/11419287/diff/1014/base/android/java/s...
File base/android/java/src/org/chromium/base/ActivityStatus.java (right):

https://chromiumcodereview.appspot.com/11419287/diff/1014/base/android/java/s...
base/android/java/src/org/chromium/base/ActivityStatus.java:53: sActivity =
activity;
On 2012/12/04 17:49:21, Philippe wrote:
> Nit: I don't know what the general consensus is about single statement
> if/for/while statements and the use of braces in Chromium Java code.
> 
> Personally I prefer omitting the braces as you did. The only thing is that
this
> file is now using the two ways of doing.

In java (contrary to c++), an if() statement must either be 1 line, or 3 or
more. i.e. here it must either be all be on one line, or use braces.

This is following android java conventions (AIUI)

https://chromiumcodereview.appspot.com/11419287/diff/1014/base/android/java/s...
base/android/java/src/org/chromium/base/ActivityStatus.java:74: // This
interface can only be used to listen to PAUSED and RESUMED
On 2012/12/04 17:49:21, Philippe wrote:
> Nit: I think that Javadoc comments are preferred at the class/interface/method
> level. I'm not sure this is true in Chromium though. The other reviewers can
> clarify this.

yes,
Java DOC if it's something the caller needs to know, // comment for an
implementation note that's more of interest to the future maintainer of the
method itself

as this is deprecated, kind of moot.

[Yaron, 2012-12-04 23:08:41]

lgtm

[John Knottenbelt, 2012-12-05 11:19:52]

lgtm

https://chromiumcodereview.appspot.com/11419287/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java
(right):

https://chromiumcodereview.appspot.com/11419287/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/LocationProvider.java:53:
if (state == ActivityStatus.PAUSED) {
This sounds reasonable to me, too.

[commit-bot: I haz the power, 2012-12-05 13:36:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/11419287/12001

[commit-bot: I haz the power, 2012-12-05 16:32:25]

Retried try job too often on linux_rel for step(s) content_unittests

[commit-bot: I haz the power, 2012-12-07 15:23:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/11419287/12001

[commit-bot: I haz the power, 2012-12-07 16:59:39]

Change committed as 171789