[Android] Introduce RegistrationPolicy for NetworkChangeNotifier.

[Android] Introduce RegistrationPolicy for NetworkChangeNotifier.

Currently the NetworkChangeNotifier.setAutoDetectConnectivityState(true) uses
ApplicationState to determine when to listen to network changes. However the
ApplicationStatus class does not work as such in the context of WebView. To make
the NetworkChangeNotifier more flexible this patch introduces a RegistrationPolicy
class which can be passed to the overloaded setAutoDetectConnectivityState method.

We make sure to keep the overall API compatible, such that existing users of the
NetworkChangeNotifier do not have to provide a policy, which is set behind the
screens instead. To this end this patch includes two concrete implementations of
the RegistrationPolicy.

BUG=520088
Committed: https://crrev.com/97997f341e5e7205b5311a5f1ed66f8cd0d66454
Cr-Commit-Position: refs/heads/master@{#353071}

[timvolodine, 2015-09-25 15:16:30]

timvolodine@chromium.org changed reviewers:
+ jkarlin@chromium.org, pauljensen@chromium.org

[timvolodine, 2015-09-29 16:30:08]

friendly ping ;)
pauljensen@ : would you be able to review?

we need this patch to make a customized implementation in case of webview.

[pauljensen, 2015-09-29 16:38:33]

I can review.

[pauljensen, 2015-10-01 12:06:37]

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:153: boolean
shouldAutoDetect, RegistrationPolicy policy) {
It looks like you refactored the body of this function.  I'm not sure I'm a big
fan of the refactor:  the two conditionals are now negated which is harder to
read/understand, and the two early returns make it harder to understand the
control flow.  Can we put it back at least for this CL?  Let's leave the
refactor for another CL.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/RegistrationPolicy.java (right):

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicy.java:11: public
abstract class RegistrationPolicy {
Can we move this to be a static subclass of NetworkChangeNotifier (NCN)?  I
think this makes the class's scope more clearly match what this class is used
for.  Then we could remove the Listener class which I find confusing, replace
mListener with a NCN pointer, and just have RegistrationPolicy.notifyRegister()
call private NCN functions directly.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicy.java:24: protected
RegistrationPolicy() {}
why do we need a constructor?

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicy.java:26: protected
final void notifyRegister() {
Can we rename notifyRegister to register? ditto for notifyUnregister

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicy.java:26: protected
final void notifyRegister() {
Please add a comments for these functions; they represent an API.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicy.java:37: mListener =
listener;
move this line of code to constructor and make this function abstract

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/RegistrationPolicyAlwaysRegister.java
(right):

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicyAlwaysRegister.java:11:
public RegistrationPolicyAlwaysRegister() {}
why do we need a constructor?

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
File
net/android/java/src/org/chromium/net/RegistrationPolicyApplicationStatus.java
(right):

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicyApplicationStatus.java:18:
public RegistrationPolicyApplicationStatus() {}
why do we need a constructor?

[timvolodine, 2015-10-05 17:26:46]

thanks for the comments! I think I've addressed most of them, PTAL.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:153: boolean
shouldAutoDetect, RegistrationPolicy policy) {
On 2015/10/01 12:06:36, pauljensen wrote:
> It looks like you refactored the body of this function.  I'm not sure I'm a
big
> fan of the refactor:  the two conditionals are now negated which is harder to
> read/understand, and the two early returns make it harder to understand the
> control flow.  Can we put it back at least for this CL?  Let's leave the
> refactor for another CL.

sure no problem, this is not essential anyway. done.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/RegistrationPolicy.java (right):

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicy.java:11: public
abstract class RegistrationPolicy {
On 2015/10/01 12:06:36, pauljensen wrote:
> Can we move this to be a static subclass of NetworkChangeNotifier (NCN)?  I
> think this makes the class's scope more clearly match what this class is used
> for.  Then we could remove the Listener class which I find confusing, replace
> mListener with a NCN pointer, and just have
RegistrationPolicy.notifyRegister()
> call private NCN functions directly.

made it inner static to NCNAD and removed the Listener interface.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicy.java:24: protected
RegistrationPolicy() {}
On 2015/10/01 12:06:37, pauljensen wrote:
> why do we need a constructor?

yup no need, removed. done.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicy.java:26: protected
final void notifyRegister() {
On 2015/10/01 12:06:36, pauljensen wrote:
> Can we rename notifyRegister to register? ditto for notifyUnregister

Done.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicy.java:26: protected
final void notifyRegister() {
On 2015/10/01 12:06:37, pauljensen wrote:
> Please add a comments for these functions; they represent an API.

Done.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicy.java:37: mListener =
listener;
On 2015/10/01 12:06:37, pauljensen wrote:
> move this line of code to constructor and make this function abstract

we should be able to pass the registration policy to the
NetworkChangeNotifierAutoDetect (NCNAD) (e.g. BackgroundSyncNetworkObserver
actually uses NCNAD on its own). so I introduced this method to solve the
chicken-egg problem regarding constructors.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/RegistrationPolicyAlwaysRegister.java
(right):

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicyAlwaysRegister.java:11:
public RegistrationPolicyAlwaysRegister() {}
On 2015/10/01 12:06:37, pauljensen wrote:
> why do we need a constructor?

no need. done.

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
File
net/android/java/src/org/chromium/net/RegistrationPolicyApplicationStatus.java
(right):

https://codereview.chromium.org/1358163004/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicyApplicationStatus.java:18:
public RegistrationPolicyApplicationStatus() {}
On 2015/10/01 12:06:37, pauljensen wrote:
> why do we need a constructor?

no need, done.

[pauljensen, 2015-10-06 12:50:01]

lgtm assuming my three small comments are addressed.  BTW you'll probably have
to fix some merge conflicts after my crrev.com/352414

https://codereview.chromium.org/1358163004/diff/40001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/RegistrationPolicyAlwaysRegister.java
(right):

https://codereview.chromium.org/1358163004/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicyAlwaysRegister.java:12:
protected void init(NetworkChangeNotifierAutoDetect notifier) {
@Override

https://codereview.chromium.org/1358163004/diff/40001/net/android/java/src/or...
File
net/android/java/src/org/chromium/net/RegistrationPolicyApplicationStatus.java
(right):

https://codereview.chromium.org/1358163004/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicyApplicationStatus.java:19:
protected void init(NetworkChangeNotifierAutoDetect notifier) {
@Override

https://codereview.chromium.org/1358163004/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicyApplicationStatus.java:22:
ApplicationStatus.registerApplicationStateListener(this);
Perhaps swap these two lines to avoid missing an application state change in
between them.  Probably not an issue as this is executed on UI thread so that
couldn't happen, but still a good idea.  The old code did this.

[timvolodine, 2015-10-07 15:12:12]

Thanks for the review pauljensen@!

https://codereview.chromium.org/1358163004/diff/40001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/RegistrationPolicyAlwaysRegister.java
(right):

https://codereview.chromium.org/1358163004/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicyAlwaysRegister.java:12:
protected void init(NetworkChangeNotifierAutoDetect notifier) {
On 2015/10/06 12:50:01, pauljensen wrote:
> @Override

Done.

https://codereview.chromium.org/1358163004/diff/40001/net/android/java/src/or...
File
net/android/java/src/org/chromium/net/RegistrationPolicyApplicationStatus.java
(right):

https://codereview.chromium.org/1358163004/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicyApplicationStatus.java:19:
protected void init(NetworkChangeNotifierAutoDetect notifier) {
On 2015/10/06 12:50:01, pauljensen wrote:
> @Override

Done.

https://codereview.chromium.org/1358163004/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/RegistrationPolicyApplicationStatus.java:22:
ApplicationStatus.registerApplicationStateListener(this);
On 2015/10/06 12:50:01, pauljensen wrote:
> Perhaps swap these two lines to avoid missing an application state change in
> between them.  Probably not an issue as this is executed on UI thread so that
> couldn't happen, but still a good idea.  The old code did this.

Done.

[pauljensen, 2015-10-07 15:14:03]

Looks like it needs another rebase; the patch fails to apply.

[timvolodine, 2015-10-07 15:22:31]

On 2015/10/07 15:14:03, pauljensen wrote:
> Looks like it needs another rebase; the patch fails to apply.

yeah hmm this is strange, looks like rietveld sees this issue as two patches

apply_issue.py found 1 dependent CLs.
They will be applied in the following order:
  #1 https://codereview.chromium.org/1358163004/#ps40001
  #2 https://codereview.chromium.org/1358163004/#ps80001

[timvolodine, 2015-10-07 17:08:41]

On 2015/10/07 15:22:31, timvolodine wrote:
> On 2015/10/07 15:14:03, pauljensen wrote:
> > Looks like it needs another rebase; the patch fails to apply.
> 
> yeah hmm this is strange, looks like rietveld sees this issue as two patches
> 
> apply_issue.py found 1 dependent CLs.
> They will be applied in the following order:
>   #1 https://codereview.chromium.org/1358163004/#ps40001
>   #2 https://codereview.chromium.org/1358163004/#ps80001

ok looks better now, was probably because of a different config on my new
workstation.. had to rebase more than I wanted to though ;)

[timvolodine, 2015-10-07 17:09:08]

+ jkarlin@ as owner for BackgroundSyncNetworkObserver.java

[jkarlin, 2015-10-07 17:20:15]

lgtm

[timvolodine, 2015-10-08 16:15:03]

On 2015/10/07 17:20:15, jkarlin wrote:
> lgtm

thanks for the reviews!

[timvolodine, 2015-10-08 16:18:01]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2015-10-08 16:18:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/1358163004/#ps120001
(title: "fix compile of tests")

[commit-bot: I haz the power, 2015-10-08 16:19:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358163004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358163004/120001

[commit-bot: I haz the power, 2015-10-08 16:25:45]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-08 16:26:40]

Patchset 7 (id:??) landed as
https://crrev.com/97997f341e5e7205b5311a5f1ed66f8cd0d66454
Cr-Commit-Position: refs/heads/master@{#353071}