Provide NetworkChangeNotifierAndroid with the actual initial connection type.

Provide NetworkChangeNotifierAndroid with the actual initial connection type.                                                                                                                                                                                                                                                  

The Android NetworkChangeNotifier is initialized with connection_type =
CONNECTION_UNKNOWN.
This means that the result returned by NetworkChangeNotifier::IsOffline() is
inaccurate until the first network change happens.
This can happen for the whole initialization path if the user starts Chrome
after he explicitly disabled WiFi and 3G. 

This CL updates NetworkChangeNotifier.java to support multiple native
observers. This can happen in case NetworkChangeNotifierDelegateAndroid is
instantiated multiple times (e.g. when multiple factories are instantiated).

Still on the Java side, NetworkChangeNotifier.java was also changed to fetch
the actual connection type as soon as the auto-detector (interacting with the
Android platform) is enabled.

On the native side, NetworkChangeNotifierDelegateAndroid now fetches at
construction time the current connection type from the Java side singleton
through a direct JNI function call (possible since the delegate is constructed
on the JNI thread).
NetworkChangeNotifierAndroid::GetCurrentConnectionType() is now a simple
wrapper around the delegate's GetCurrentConnectionType() method (which is
thread-safe).

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

[Philippe, 2012-12-18 14:57:56]

On 2012/12/18 14:41:24, Philippe wrote:

Please ignore this CL for now. I will upload a new patch set soon.

[Philippe, 2012-12-18 15:38:18]

+ Ryan

I have uploaded a new patch set which should be ready for review.

FYI, I came across this issue when I tried to use the new
URLFetcher::SetAutomaticallyRetryOnNetworkChanges() method.
NetworkChangeNotifier::IsOffline() (used by this new feature) was returning
false although I had no connectivity on my device.

[Ryan Sleevi, 2012-12-18 18:48:48]

You have significantly altered the threading lifetime and guarantees here, and I
will need time to review to understand this in the larger picture.

Is there a reason you did this? It does not seem necessary to solve the bug, nor
does it seem to make the threading any simpler, so I'm trying to understand the
motivation.

The whole point of having the flow of messages go from NCNDA -> NCNA was to
provide safer, simpler threading. This mix of communication between NCNDA ->
NCNA (via observers) and NCNA -> NCNDA (via directly calling from any thread and
locking) seems to add more complexity.

[Philippe, 2012-12-19 10:00:48]

On 2012/12/18 18:48:48, Ryan Sleevi wrote:
> You have significantly altered the threading lifetime and guarantees here, and
I
> will need time to review to understand this in the larger picture.
> 
> Is there a reason you did this? It does not seem necessary to solve the bug,
nor
> does it seem to make the threading any simpler, so I'm trying to understand
the
> motivation.
> 
> The whole point of having the flow of messages go from NCNDA -> NCNA was to
> provide safer, simpler threading. This mix of communication between NCNDA ->
> NCNA (via observers) and NCNA -> NCNDA (via directly calling from any thread
and
> locking) seems to add more complexity.

I understand your concerns and I think I will come up with a new patch set
addressing the issues I pointed out below. 

My initial patch set (#1) was only changing the Java side to notify the
observers with the initial connection type as soon as the Java object is
constructed.
This was making all the *existing* native side delegates (NCNDA) (which
subscribed to the Java side) receive the initial connection type and forward it
to the notifiers (NCNA) that subscribed to them. This wasn't enough obviously.

The problem is that a new notifier created through the factory wouldn't get the
actual initial connection type if it doesn't fetch it through the delegate
(which initially received the actual connection type from the Java side).

Note that my latest change (patch set #2) is not ideal either since the Java
side fires the initial notification only when the singleton object is
constructed. This is a one time event. When this happens, there might be no
native observer (NCNDA) at all if the Java side singleton was used very early.
The best case is when there is already a NCNDA observing the Java side
singleton. In that case only the notifiers observing this delegate would get the
initial actual type. Any other notifier created after this one time event
through the same factory (i.e. sharing the same delegate) wouldn't receive this
event obviously. It's even worse if another factory (i.e. another delegate) is
created.

I will prepare a totally different patch set addressing these issues.

[Philippe, 2012-12-19 16:48:45]

On 2012/12/19 10:00:48, Philippe wrote:
> On 2012/12/18 18:48:48, Ryan Sleevi wrote:
> > You have significantly altered the threading lifetime and guarantees here,
and
> I
> > will need time to review to understand this in the larger picture.
> > 
> > Is there a reason you did this? It does not seem necessary to solve the bug,
> nor
> > does it seem to make the threading any simpler, so I'm trying to understand
> the
> > motivation.
> > 
> > The whole point of having the flow of messages go from NCNDA -> NCNA was to
> > provide safer, simpler threading. This mix of communication between NCNDA ->
> > NCNA (via observers) and NCNA -> NCNDA (via directly calling from any thread
> and
> > locking) seems to add more complexity.
> 
> I understand your concerns and I think I will come up with a new patch set
> addressing the issues I pointed out below. 
> 
> My initial patch set (#1) was only changing the Java side to notify the
> observers with the initial connection type as soon as the Java object is
> constructed.
> This was making all the *existing* native side delegates (NCNDA) (which
> subscribed to the Java side) receive the initial connection type and forward
it
> to the notifiers (NCNA) that subscribed to them. This wasn't enough obviously.
> 
> The problem is that a new notifier created through the factory wouldn't get
the
> actual initial connection type if it doesn't fetch it through the delegate
> (which initially received the actual connection type from the Java side).
> 
> Note that my latest change (patch set #2) is not ideal either since the Java
> side fires the initial notification only when the singleton object is
> constructed. This is a one time event. When this happens, there might be no
> native observer (NCNDA) at all if the Java side singleton was used very early.
> The best case is when there is already a NCNDA observing the Java side
> singleton. In that case only the notifiers observing this delegate would get
the
> initial actual type. Any other notifier created after this one time event
> through the same factory (i.e. sharing the same delegate) wouldn't receive
this
> event obviously. It's even worse if another factory (i.e. another delegate) is
> created.
> 
> I will prepare a totally different patch set addressing these issues.

Hi Ryan,

I uploaded a new patch set trying another approach. You might not like the use
of WeakPtr (I'm not even 100% sure it is correct in this case since it has its
subtleties). Unfortunately I didn't see ways other than using a WeakPtr or
making |NetworkChangeNotifierDelegateAndroid::connection_type_| be guarded by a
lock to address this (I'm excluding thread-safe ref-counting obviously :). The
lock might be a better approach since there shouldn't be any contention. I won't
be surprised if you come up with a clean and simple approach involving none of
these subtleties though :).

[Ryan Sleevi, 2012-12-19 17:38:15]

So, my biggest concern with your first approach is that by plumbing things
through, you

1) Introduced lock contention on NDNDA's lock when NCNA's are notified of
changes
2) Potentially caused events to be fired when the network hadn't been changed

T2: NCNDA receives change event (Network State == 1)
T2: NCNDA posts tasks to its NCNAs to wake them (Network State == 1,
Notification 1)
T2: NCNDA receives change event (Network State == 2)
T2: NCNDA posts tasks to its NCNAs to wake them (network state == 2,
Notification 2)
T1: Wakes up and processes the NCNDA->NCNA notification 1
T1: Calls NCNDA, sees Network State == 2
T1: Processes the NCNDA->NCNA notification 2
T1: Calls NCNDA, sees Network State == 2 <-- No change!


Can you just not have the AddObserver registration receive the initial state? If
you get NCNDA's state *before* calling NCNDA::observers_->AddObserver(), then
return that state from AddObserver, you shouldn't have any of the threading
complexity.

WDYT?

https://codereview.chromium.org/11628008/diff/11001/net/android/network_chang...
File net/android/network_change_notifier_delegate_android.h (right):

https://codereview.chromium.org/11628008/diff/11001/net/android/network_chang...
net/android/network_change_notifier_delegate_android.h:58: void
AddObserver(Observer* observer);
Can you not just have AddObserver return the initial state when adding?

[Philippe, 2012-12-19 17:52:23]

What do you think?

https://codereview.chromium.org/11628008/diff/11001/net/android/network_chang...
File net/android/network_change_notifier_delegate_android.h (right):

https://codereview.chromium.org/11628008/diff/11001/net/android/network_chang...
net/android/network_change_notifier_delegate_android.h:58: void
AddObserver(Observer* observer);
On 2012/12/19 17:38:16, Ryan Sleevi wrote:
> Can you not just have AddObserver return the initial state when adding?

I might be missing something but AddObserver() can be called from the I/O
thread. So if I read |connection_type_| in AddObserver(), ideally this should be
guarded by a lock. We might accept to read a possibly outdated value (without
using a lock) in practice. Is it what you meant?

[Ryan Sleevi, 2012-12-19 18:07:57]

In comparing Patchset 2 with Patchset 3, I think even with my concerns, it
(PS#2) is probably better/simpler. It does match our Linux & Mac
implementations, so if there is a problematic pattern, it's probably more
wide-spread then we've realized.

Having NCNA ask the NCNDA the ConnectionType doesn't seem to radically offensive
(ObserverListThreadSafe already makes use of Lock-guarded structures), so I
think going that route would be fine, and better than having AddObserver return
the current type (for the reason you point out regarding needing a lock)

Note: I wasn't asking you to rework things in my original comment, just provide
more explanation/justification for the issues, because it definitely changed
some message flows & timing when NCNA::GetConnectionType forwarded to
NCNDA::GetConnectionType

https://codereview.chromium.org/11628008/diff/4001/net/android/network_change...
File net/android/network_change_notifier_delegate_android.h (right):

https://codereview.chromium.org/11628008/diff/4001/net/android/network_change...
net/android/network_change_notifier_delegate_android.h:57: // any thread.
This comment is confusing and would need to be re-worded.

https://codereview.chromium.org/11628008/diff/11001/net/android/network_chang...
File net/android/network_change_notifier_delegate_android.h (right):

https://codereview.chromium.org/11628008/diff/11001/net/android/network_chang...
net/android/network_change_notifier_delegate_android.h:58: void
AddObserver(Observer* observer);
On 2012/12/19 17:52:23, Philippe wrote:
> On 2012/12/19 17:38:16, Ryan Sleevi wrote:
> > Can you not just have AddObserver return the initial state when adding?
> 
> I might be missing something but AddObserver() can be called from the I/O
> thread. So if I read |connection_type_| in AddObserver(), ideally this should
be
> guarded by a lock. We might accept to read a possibly outdated value (without
> using a lock) in practice. Is it what you meant?

Yeah, you'd still have to have a lock.

[Philippe, 2012-12-19 18:14:36]

On 2012/12/19 18:07:57, Ryan Sleevi wrote:
> In comparing Patchset 2 with Patchset 3, I think even with my concerns, it
> (PS#2) is probably better/simpler. It does match our Linux & Mac
> implementations, so if there is a problematic pattern, it's probably more
> wide-spread then we've realized.
> 
> Having NCNA ask the NCNDA the ConnectionType doesn't seem to radically
offensive
> (ObserverListThreadSafe already makes use of Lock-guarded structures), so I
> think going that route would be fine, and better than having AddObserver
return
> the current type (for the reason you point out regarding needing a lock)
> 
> Note: I wasn't asking you to rework things in my original comment, just
provide
> more explanation/justification for the issues, because it definitely changed
> some message flows & timing when NCNA::GetConnectionType forwarded to
> NCNDA::GetConnectionType
> 
>
https://codereview.chromium.org/11628008/diff/4001/net/android/network_change...
> File net/android/network_change_notifier_delegate_android.h (right):
> 
>
https://codereview.chromium.org/11628008/diff/4001/net/android/network_change...
> net/android/network_change_notifier_delegate_android.h:57: // any thread.
> This comment is confusing and would need to be re-worded.
> 
>
https://codereview.chromium.org/11628008/diff/11001/net/android/network_chang...
> File net/android/network_change_notifier_delegate_android.h (right):
> 
>
https://codereview.chromium.org/11628008/diff/11001/net/android/network_chang...
> net/android/network_change_notifier_delegate_android.h:58: void
> AddObserver(Observer* observer);
> On 2012/12/19 17:52:23, Philippe wrote:
> > On 2012/12/19 17:38:16, Ryan Sleevi wrote:
> > > Can you not just have AddObserver return the initial state when adding?
> > 
> > I might be missing something but AddObserver() can be called from the I/O
> > thread. So if I read |connection_type_| in AddObserver(), ideally this
should
> be
> > guarded by a lock. We might accept to read a possibly outdated value
(without
> > using a lock) in practice. Is it what you meant?
> 
> Yeah, you'd still have to have a lock.

Ok, thanks Ryan for the details. I will upload a new patch set based on patch
set #2 then. I will also probably include the changes on the Java side that I
made in patch set #3.

[Philippe, 2012-12-20 15:37:32]

On 2012/12/19 18:14:36, Philippe wrote:
> On 2012/12/19 18:07:57, Ryan Sleevi wrote:
> > In comparing Patchset 2 with Patchset 3, I think even with my concerns, it
> > (PS#2) is probably better/simpler. It does match our Linux & Mac
> > implementations, so if there is a problematic pattern, it's probably more
> > wide-spread then we've realized.
> > 
> > Having NCNA ask the NCNDA the ConnectionType doesn't seem to radically
> offensive
> > (ObserverListThreadSafe already makes use of Lock-guarded structures), so I
> > think going that route would be fine, and better than having AddObserver
> return
> > the current type (for the reason you point out regarding needing a lock)
> > 
> > Note: I wasn't asking you to rework things in my original comment, just
> provide
> > more explanation/justification for the issues, because it definitely changed
> > some message flows & timing when NCNA::GetConnectionType forwarded to
> > NCNDA::GetConnectionType
> > 
> >
>
https://codereview.chromium.org/11628008/diff/4001/net/android/network_change...
> > File net/android/network_change_notifier_delegate_android.h (right):
> > 
> >
>
https://codereview.chromium.org/11628008/diff/4001/net/android/network_change...
> > net/android/network_change_notifier_delegate_android.h:57: // any thread.
> > This comment is confusing and would need to be re-worded.
> > 
> >
>
https://codereview.chromium.org/11628008/diff/11001/net/android/network_chang...
> > File net/android/network_change_notifier_delegate_android.h (right):
> > 
> >
>
https://codereview.chromium.org/11628008/diff/11001/net/android/network_chang...
> > net/android/network_change_notifier_delegate_android.h:58: void
> > AddObserver(Observer* observer);
> > On 2012/12/19 17:52:23, Philippe wrote:
> > > On 2012/12/19 17:38:16, Ryan Sleevi wrote:
> > > > Can you not just have AddObserver return the initial state when adding?
> > > 
> > > I might be missing something but AddObserver() can be called from the I/O
> > > thread. So if I read |connection_type_| in AddObserver(), ideally this
> should
> > be
> > > guarded by a lock. We might accept to read a possibly outdated value
> (without
> > > using a lock) in practice. Is it what you meant?
> > 
> > Yeah, you'd still have to have a lock.
> 
> Ok, thanks Ryan for the details. I will upload a new patch set based on patch
> set #2 then. I will also probably include the changes on the Java side that I
> made in patch set #3.

I have just uploaded a new patch set ready for review.

[gone, 2012-12-20 19:37:31]

Just some random comments, for now.  I think rsleevi has a better handle on any
threading potential threading issues.

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:71: static void
reset(Context context) {
Maybe this should be called recreateInstance() to go with getInstance()?  Or
make it more explicit that this is for tests, like
MetricsService::StartRecordingForTests()?

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/javate...
File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/javate...
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:113:
noConnectivityIntent.putExtra(ConnectivityManager.EXTRA_NO_CONNECTIVITY, true);
Don't think you use this Extra anymore.

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/networ...
File net/android/network_change_notifier_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/networ...
net/android/network_change_notifier_delegate_android.cc:110:
NetworkChangeNotifierDelegateAndroid::GetJavaSideConnectivityState() {
What's the rule for indenting here?

[Ryan Sleevi, 2012-12-20 19:43:08]

So the C++ changes mostly LGTM, but I'm having trouble understanding the test &
Java side changes.

Your added test seems to be testing when there are multiple NCNDAs. When is that
possible?

I have some comment nits, but I think I should first understand that part.

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/networ...
File net/android/network_change_notifier_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/networ...
net/android/network_change_notifier_delegate_android.cc:110:
NetworkChangeNotifierDelegateAndroid::GetJavaSideConnectivityState() {
On 2012/12/20 19:37:31, dfalcantara wrote:
> What's the rule for indenting here?

This indent is fine.

[gone, 2012-12-20 21:52:20]

I'm confused about the need for multiple native observers, too.  Aren't these
classes meant to be singletons to avoid having multiple things communicating
across the JNI barrier?  Is the functionality needed just for the test?

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:22: * the
autoDetectConnectivityState function.
autoDetectConnectivityState > setAutoDetectConnectivityState

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:24: * WARNING:
This class is not thread-safe.
import javax.annotation.concurrent.NotThreadSafe;
@NotThreadSafe

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:111: *
(initializing the singleton) was created.
assert that the singleton isn't null instead of the comment

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:123:
!mConnectivityManagerDelegate.isConnected()) {
8 spaces I think

[Philippe, 2012-12-21 09:51:55]

Thanks!

To be clear, having multiple factories is not the general use case. The
"problem" is that we don't enforce the singleton property on the native side.
And I think this is good. Ideally singleton should only be used when having more
than one instance of a class is the end of the world.

Since nothing prevents the factory on the native side from being instantiated
twice (which is good again), I think the Java side should handle this properly
rather than crashing when assertions are enabled or silently failing when they
are disabled (leading to subtle bugs). Besides that I also use this property in
the newly added unit test which is quite convenient (although testing shouldn't
completely drive design).

What do you think?

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:22: * the
autoDetectConnectivityState function.
On 2012/12/20 21:52:20, dfalcantara wrote:
> autoDetectConnectivityState > setAutoDetectConnectivityState

Done.

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:24: * WARNING:
This class is not thread-safe.
On 2012/12/20 21:52:20, dfalcantara wrote:
> import javax.annotation.concurrent.NotThreadSafe;
> @NotThreadSafe

Nice! Unfortunately I think we are missing a dependency JAR for this. javac
can't resolve the import. Have you used this recently in Chromium?

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:71: static void
reset(Context context) {
On 2012/12/20 19:37:31, dfalcantara wrote:
> Maybe this should be called recreateInstance() to go with getInstance()?  Or
> make it more explicit that this is for tests, like
> MetricsService::StartRecordingForTests()?

I renamed it to resetInstanceForTests().

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:111: *
(initializing the singleton) was created.
On 2012/12/20 21:52:20, dfalcantara wrote:
> assert that the singleton isn't null instead of the comment

I removed the comment. I am now relying on the assert in getInstance().

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:123:
!mConnectivityManagerDelegate.isConnected()) {
On 2012/12/20 21:52:20, dfalcantara wrote:
> 8 spaces I think

Done.

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/javate...
File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/javate...
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:113:
noConnectivityIntent.putExtra(ConnectivityManager.EXTRA_NO_CONNECTIVITY, true);
On 2012/12/20 19:37:31, dfalcantara wrote:
> Don't think you use this Extra anymore.

Good catch!

[Ryan Sleevi, 2012-12-21 19:26:07]

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
File net/android/network_change_notifier_android_unittest.cc (right):

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
net/android/network_change_notifier_android_unittest.cc:79:
SetGlobalConnectivityStateAsOnline(true);
nit: I actually found the original code cleaner, in this case. It's not
immediately obvious from reading this that the bool here is used to control the
online/offline state, and not some secondary argument.

While there is more typing involved, I think it's clearer. An alternative is
just two helper methods "SetOffline()" & "SetOnline" [the
'globalconnectivitystate' is a bit redundant/unnecessary]

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
net/android/network_change_notifier_android_unittest.cc:103: // This addresses
http://crbug.com/166883.
I'd drop this comment. It'd be one thing if it was some highly specific
regression test, but this isn't - it's testing that Android implements the same
guarantees that (Linux, Mac, Win) do - which is that the initial state is
immediately acquired, rather than waiting for the first transition state.

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
net/android/network_change_notifier_android_unittest.cc:108:
delegate_.GetCurrentConnectionType());
Just from a naieve reading, these two lines combined with the comments
explaining the test (line 97-99) are quite confusing.

You're wanting to test that it's initialized with the right state, but then you
immediately test CONNECT_UNKNOWN after initializing, which you suggest is the
wrong state.

Hopefully you see the confusion here.

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
File net/android/network_change_notifier_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
net/android/network_change_notifier_delegate_android.cc:73:
&Observer::OnConnectionTypeChanged, actual_connection_type);
Considering that all of the NCNDA::Observers do not use
|actual_connection_type|, but instead query
delegate_->GetCurrentConnectionType(), supplying connection type perhaps
unnecessary/superflous?

[Philippe, 2012-12-26 10:33:25]

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
File net/android/network_change_notifier_android_unittest.cc (right):

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
net/android/network_change_notifier_android_unittest.cc:79:
SetGlobalConnectivityStateAsOnline(true);
On 2012/12/21 19:26:07, Ryan Sleevi wrote:
> nit: I actually found the original code cleaner, in this case. It's not
> immediately obvious from reading this that the bool here is used to control
the
> online/offline state, and not some secondary argument.
> 
> While there is more typing involved, I think it's clearer. An alternative is
> just two helper methods "SetOffline()" & "SetOnline" [the
> 'globalconnectivitystate' is a bit redundant/unnecessary]

I chose the SetOnline/SetOffline alternative. I had made this change initially
after I moved the ConnectivityState enum to the class' private section. I
removed this enum now.

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
net/android/network_change_notifier_android_unittest.cc:103: // This addresses
http://crbug.com/166883.
On 2012/12/21 19:26:07, Ryan Sleevi wrote:
> I'd drop this comment. It'd be one thing if it was some highly specific
> regression test, but this isn't - it's testing that Android implements the
same
> guarantees that (Linux, Mac, Win) do - which is that the initial state is
> immediately acquired, rather than waiting for the first transition state.

Done.

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
net/android/network_change_notifier_android_unittest.cc:108:
delegate_.GetCurrentConnectionType());
On 2012/12/21 19:26:07, Ryan Sleevi wrote:
> Just from a naieve reading, these two lines combined with the comments
> explaining the test (line 97-99) are quite confusing.
> 
> You're wanting to test that it's initialized with the right state, but then
you
> immediately test CONNECT_UNKNOWN after initializing, which you suggest is the
> wrong state.
> 
> Hopefully you see the confusion here.

I hope it's less confusing now.

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
File net/android/network_change_notifier_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/11628008/diff/23001/net/android/networ...
net/android/network_change_notifier_delegate_android.cc:73:
&Observer::OnConnectionTypeChanged, actual_connection_type);
On 2012/12/21 19:26:07, Ryan Sleevi wrote:
> Considering that all of the NCNDA::Observers do not use
> |actual_connection_type|, but instead query
> delegate_->GetCurrentConnectionType(), supplying connection type perhaps
> unnecessary/superflous?

Good point. Note that this implied a few changes in the unit tests.

[Philippe, 2013-01-03 12:04:49]

Ping :)

[Ryan Sleevi, 2013-01-03 18:28:25]

Some comment nits, but C++ LGTM, no need for re-review.

https://chromiumcodereview.appspot.com/11628008/diff/28002/net/android/networ...
File net/android/network_change_notifier_android_unittest.cc (right):

https://chromiumcodereview.appspot.com/11628008/diff/28002/net/android/networ...
net/android/network_change_notifier_android_unittest.cc:73: SetOffline();
nit: Add some comments (*above* line 73/78/83)

// Changing from online to offline should trigger a notification

https://chromiumcodereview.appspot.com/11628008/diff/28002/net/android/networ...
net/android/network_change_notifier_android_unittest.cc:78: SetOffline();
// No notification should be triggered when the offline state hasn't changed

https://chromiumcodereview.appspot.com/11628008/diff/28002/net/android/networ...
net/android/network_change_notifier_android_unittest.cc:83: SetOnline();
// Going from offline to online should trigger a notification

https://chromiumcodereview.appspot.com/11628008/diff/28002/net/android/networ...
File net/android/network_change_notifier_delegate_android.h (right):

https://chromiumcodereview.appspot.com/11628008/diff/28002/net/android/networ...
net/android/network_change_notifier_delegate_android.h:43: 
Pedantry, but I think it might be easier to group by "methods called on
particular threads" and "methods called on any thread" (eg: move this to ~ line
57/58). Hardly strong feelings about this, since I think the comment is clear.

[gone, 2013-01-03 18:47:12]

lgtm

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://chromiumcodereview.appspot.com/11628008/diff/20001/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:24: * WARNING:
This class is not thread-safe.
On 2012/12/21 09:51:55, Philippe wrote:
> On 2012/12/20 21:52:20, dfalcantara wrote:
> > import javax.annotation.concurrent.NotThreadSafe;
> > @NotThreadSafe
> 
> Nice! Unfortunately I think we are missing a dependency JAR for this. javac
> can't resolve the import. Have you used this recently in Chromium?

Ah, too bad.  We do it inside the android-specific source; I guess we don't link
against them here.

[Philippe, 2013-01-04 10:13:01]

Thanks guys!

https://codereview.chromium.org/11628008/diff/28002/net/android/network_chang...
File net/android/network_change_notifier_android_unittest.cc (right):

https://codereview.chromium.org/11628008/diff/28002/net/android/network_chang...
net/android/network_change_notifier_android_unittest.cc:73: SetOffline();
On 2013/01/03 18:28:25, Ryan Sleevi wrote:
> nit: Add some comments (*above* line 73/78/83)
> 
> // Changing from online to offline should trigger a notification

Thanks.

https://codereview.chromium.org/11628008/diff/28002/net/android/network_chang...
net/android/network_change_notifier_android_unittest.cc:78: SetOffline();
On 2013/01/03 18:28:25, Ryan Sleevi wrote:
> // No notification should be triggered when the offline state hasn't changed

Done.

https://codereview.chromium.org/11628008/diff/28002/net/android/network_chang...
net/android/network_change_notifier_android_unittest.cc:83: SetOnline();
On 2013/01/03 18:28:25, Ryan Sleevi wrote:
> // Going from offline to online should trigger a notification

Done.

https://codereview.chromium.org/11628008/diff/28002/net/android/network_chang...
File net/android/network_change_notifier_delegate_android.h (right):

https://codereview.chromium.org/11628008/diff/28002/net/android/network_chang...
net/android/network_change_notifier_delegate_android.h:43: 
On 2013/01/03 18:28:25, Ryan Sleevi wrote:
> Pedantry, but I think it might be easier to group by "methods called on
> particular threads" and "methods called on any thread" (eg: move this to ~
line
> 57/58). Hardly strong feelings about this, since I think the comment is clear.

Good point.

[commit-bot: I haz the power, 2013-01-04 12:02:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/11628008/36001