Refactor Android's NetworckChangeNotifier.

Refactor Android's NetworckChangeNotifier.

The current implementation of NetworkChangeNotifier on Android
assumes that the embedder has the Android ACCESS_NETWORK_STATE
platform permission, and will crash if it does not.

This change refactors the implementation such that the embedder
pushes state changes into the NetworkChangeNotfier, and makes
the NetworkChangeNotifier a singleton.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146100

[benm (inactive), 2012-07-02 15:19:48]

Hi guys,

A refactoring of the Android NetworkChangeNotifier that means Android platform
permissions are not required by the net/ layer. Embedders must now push network
state changes into the net/ layer and are responsible for acquiring any
permissions they require to do so.

Please take a look!

[benm (inactive), 2012-07-02 15:21:41]

+joth

[Ryan Sleevi, 2012-07-02 16:12:20]

We've been trying to remove as many singletons as possible in the network stack,
and there's a very high bar when introducing them.

From this CL, it's not clear what the benefit is. Embedders are still going to
require permissions to ACCESS_NETWORK_STATE with this, right? If so, I'm not
sure what the net win is, and there are clear net losses (requiring more
boilerplate code to be implemented by embedders, introducing singletons,
separating out logic from where it logically belongs).

As such, this would be a nack for now without understanding what the 'big
picture' is. This is part of why gcl warns you about an absence of BUG= lines -
for non-trivial changes such as this, it's good to have a tracking bug that can
explain the motivations and track the work involved in it.

http://codereview.chromium.org/10693068/diff/1/net/android/network_change_not...
File net/android/network_change_notifier_factory.h (right):

http://codereview.chromium.org/10693068/diff/1/net/android/network_change_not...
net/android/network_change_notifier_factory.h:20: //
NetworkChangeNotifierFactory creates a sinfleton Android-specific
sinfleton -> singleton

[benm (inactive), 2012-07-10 14:36:40]

Thanks for taking a look Ryan. I've refactored the refactoring. This time, we
keep the auto-detecting code in the tree, and allow embedders to enable it with
a simple API. By default we will use the "simple" approach rather than the
auto-detector to avoid permission problems. If an app does not have the
requisite permission and invokes the API to enable the auto-detector, I'm happy
with them crashing :)

I have also pushed the logic to the java side, so there are no substantive
native changes to singletons, etc.

Please take a look!

Thanks, Ben

[Ryan Sleevi, 2012-07-10 14:53:12]

Is there anything for me to review here? Looks like it's all java/jni-side (I
assume joth/yaron can stamp)

[joth, 2012-07-10 17:24:53]

lgtm with a couple small questions

http://codereview.chromium.org/10693068/diff/8001/net/android/java/src/org/ch...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

http://codereview.chromium.org/10693068/diff/8001/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:47: public
static void setAutoDetectConnectivityState(boolean shouldAutoDetect) {
i'm not sure where this gets called with setAutoDetectConnectivityState(true) ?

http://codereview.chromium.org/10693068/diff/8001/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:105: }
instead, could call setAutoDetectConnectivityStateInternal(false) here? I'm fine
with it either way though.

[benm (inactive), 2012-07-10 17:28:57]

Thanks joth!

http://codereview.chromium.org/10693068/diff/8001/net/android/java/src/org/ch...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

http://codereview.chromium.org/10693068/diff/8001/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:47: public
static void setAutoDetectConnectivityState(boolean shouldAutoDetect) {
On 2012/07/10 17:24:53, joth wrote:
> i'm not sure where this gets called with setAutoDetectConnectivityState(true)
?

That's up to the embedding application should they decide to use the
autodetecting code.

http://codereview.chromium.org/10693068/diff/8001/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:105: }
IMHO, I prefer this as it seems to be more explicit.

[benm (inactive), 2012-07-10 17:31:34]

+satish for OWNERS

[Satish, 2012-07-10 23:40:16]

lgtm

[commit-bot: I haz the power, 2012-07-11 09:37:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benm@chromium.org/10693068/8001

[commit-bot: I haz the power, 2012-07-11 10:48:39]

Change committed as 146100

[benm (inactive), 2012-07-20 15:41:14]

*sigh* please ignore patch set 3 :)