Add native-side unit test for Android NetworkChangeNotifier

Add native-side unit test for Android NetworkChangeNotifier

* Renames some classes to move them out of the android namespace.
* Changes singleton handling in the Java NetworkChangeNotifier. The singleton
  is unfortunately still required for Android to turn the AutoDetector on.
* Adds a native-side unit test to track that the JNI calls are correctly plumbed
  to alert native-side observers of connection changes.

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

[gone, 2012-09-13 22:47:08]

Er, take 2:
Hey guys,
This CL further expands the Java NetworkChangeNotifier so that it can alert
Java-side observers, which we'll need for upstreaming some MetricsService
changes on Android.  It also brings over one Java unit test and adds a new
native side unit test.  No need to rush on the review; it's kind of large-ish.

If you want to punt to someone else, let me know.  Thanks!

[gone, 2012-09-13 23:03:56]

On 2012/09/13 22:47:08, dfalcantara wrote:
> Er, take 2:
> Hey guys,
> This CL further expands the Java NetworkChangeNotifier so that it can alert
> Java-side observers, which we'll need for upstreaming some MetricsService
> changes on Android.  It also brings over one Java unit test and adds a new
> native side unit test.  No need to rush on the review; it's kind of large-ish.
> 
> If you want to punt to someone else, let me know.  Thanks!

Apologies if you've already started on this one; seems like this should be split
apart into two different CLs.  Doing so now.

[gone, 2012-09-14 00:48:25]

On 2012/09/13 23:03:56, dfalcantara wrote:
> On 2012/09/13 22:47:08, dfalcantara wrote:
> > Er, take 2:
> > Hey guys,
> > This CL further expands the Java NetworkChangeNotifier so that it can alert
> > Java-side observers, which we'll need for upstreaming some MetricsService
> > changes on Android.  It also brings over one Java unit test and adds a new
> > native side unit test.  No need to rush on the review; it's kind of
large-ish.
> > 
> > If you want to punt to someone else, let me know.  Thanks!
> 
> Apologies if you've already started on this one; seems like this should be
split
> apart into two different CLs.  Doing so now.

Alright, I split off the Java-side stuff and left the unit test.  Please have a
look when you get a chance.

[szym, 2012-09-14 19:07:29]

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
File net/android/network_change_notifier_android.cc (right):

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
net/android/network_change_notifier_android.cc:29: void
NetworkChangeNotifier::NotifyObserversOfConnectionTypeChange(JNIEnv* env,
watch out for conflict with http://codereview.chromium.org/10905264

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
File net/android/network_change_notifier_android.h (right):

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
net/android/network_change_notifier_android.h:16: class NetworkChangeNotifier :
public net::NetworkChangeNotifier {
Hmm, I'm not sure that using namespace android and overloading names like that
is best for readability. You are within namespace net so NetworkChangeNotifier
should be net::NetworkChangeNotifier. This should likely be
NetworkChangeNotifierAndroid to match Chromium naming style.

Perhaps another CL to avoid merge conflicts, and this applies to
net::android::NetworkChangeNotifierFactory too.

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
net/android/network_change_notifier_android.h:23: void
ForceConnectivityState(bool state);
What is this for? If this is for tests only, at least add a comment, but
preferably hide it in the private section and friend the test suite.

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
net/android/network_change_notifier_android.h:28: // NetworkChangeNotifier:
since you're already here, make it "net::NetworkChangeNotifier:" unless you add
the "Android" suffix to the top-level class.

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
File net/android/network_change_notifier_android_unittest.cc (right):

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
net/android/network_change_notifier_android_unittest.cc:41:
net::NetworkChangeNotifier::ConnectionType current_connection_;
nit: DISALLOW_COPY_AND_ASSIGN(TestConnectionTypeObserver)

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
net/android/network_change_notifier_android_unittest.cc:51: virtual void SetUp()
{
Everything from here down can be protected:

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
net/android/network_change_notifier_android_unittest.cc:52: notifier_.reset(new
net::android::NetworkChangeNotifier());
You don't really need "net::android::" here. I understand if you want to avoid
confusion with net::NetworkChangeNotifier and I strongly suggest renaming it to
NetworkChangeNotifierAndroid.

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
net/android/network_change_notifier_android_unittest.cc:58:
net::android::NetworkChangeNotifier* notifier() {
Rather than the getter I suggest a forwarding ForceConnectivityState method.
This would allow you to make the NCN method private and just friend
NetworkChangeNotifierAndroidTest.

http://codereview.chromium.org/10928193/diff/3003/net/android/network_change_...
net/android/network_change_notifier_android_unittest.cc:62:
TestConnectionTypeObserver* observer() {
nit: const ... const?

http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
net/base/network_change_notifier.h:20: #if defined(OS_ANDROID)
You shouldn't need to forward declare for friend declaration. See
NetworkChangeNotifierWinTest.

http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
net/base/network_change_notifier.h:206: #if defined(OS_ANDROID)
No need for #if here.

[szym, 2012-09-14 19:26:34]

http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
net/base/network_change_notifier.h:20: #if defined(OS_ANDROID)
On 2012/09/14 19:07:29, szym wrote:
> You shouldn't need to forward declare for friend declaration. See
> NetworkChangeNotifierWinTest.

:( Apparently this is not true if you want to friend a class from another
namespace.

http://stackoverflow.com/questions/4492062/why-does-a-c-friend-class-need-a-f...

[szym, 2012-09-14 19:28:38]

http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
net/base/network_change_notifier.h:20: #if defined(OS_ANDROID)
On 2012/09/14 19:26:34, szym wrote:
> On 2012/09/14 19:07:29, szym wrote:
> > You shouldn't need to forward declare for friend declaration. See
> > NetworkChangeNotifierWinTest.
> 
> :( Apparently this is not true if you want to friend a class from another
> namespace.
> 
>
http://stackoverflow.com/questions/4492062/why-does-a-c-friend-class-need-a-f...

Still, I wonder if you need #if defined.

[Ryan Sleevi, 2012-09-14 19:43:08]

On 2012/09/14 19:28:38, szym wrote:
>
http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
> File net/base/network_change_notifier.h (right):
> 
>
http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
> net/base/network_change_notifier.h:20: #if defined(OS_ANDROID)
> On 2012/09/14 19:26:34, szym wrote:
> > On 2012/09/14 19:07:29, szym wrote:
> > > You shouldn't need to forward declare for friend declaration. See
> > > NetworkChangeNotifierWinTest.
> > 
> > :( Apparently this is not true if you want to friend a class from another
> > namespace.
> > 
> >
>
http://stackoverflow.com/questions/4492062/why-does-a-c-friend-class-need-a-f...
> 
> Still, I wonder if you need #if defined.

I believe you do, unless you're also moving the forward declaration of the
namespace out of the #if defined as well.

Otherwise, the pre-processor won't know whether you're attempting to friend a
nested class of a class called "android" or a class within the namespace.
Because it's unable to resolve the ambiguity of the symbol, it should generate
an error (since it cannot infer the correct type to forward declare, like it can
with non-namespace qualified identifiers)

[szym, 2012-09-14 19:47:11]

On 2012/09/14 19:43:08, Ryan Sleevi wrote:
> On 2012/09/14 19:28:38, szym wrote:
> >
>
http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
> > File net/base/network_change_notifier.h (right):
> > 
> >
>
http://codereview.chromium.org/10928193/diff/3003/net/base/network_change_not...
> > net/base/network_change_notifier.h:20: #if defined(OS_ANDROID)
> > On 2012/09/14 19:26:34, szym wrote:
> > > On 2012/09/14 19:07:29, szym wrote:
> > > > You shouldn't need to forward declare for friend declaration. See
> > > > NetworkChangeNotifierWinTest.
> > > 
> > > :( Apparently this is not true if you want to friend a class from another
> > > namespace.
> > > 
> > >
> >
>
http://stackoverflow.com/questions/4492062/why-does-a-c-friend-class-need-a-f...
> > 
> > Still, I wonder if you need #if defined.
> 
> I believe you do, unless you're also moving the forward declaration of the
> namespace out of the #if defined as well.
> 
> Otherwise, the pre-processor won't know whether you're attempting to friend a
> nested class of a class called "android" or a class within the namespace.
> Because it's unable to resolve the ambiguity of the symbol, it should generate
> an error (since it cannot infer the correct type to forward declare, like it
can
> with non-namespace qualified identifiers)

Right. I was suggesting to forward declare non-existing class to avoid the #if
defineds.

[gone, 2012-09-15 01:01:55]

Ended up changing the namespace in this CL, which fixes and cleans up a few
things.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
File net/android/network_change_notifier_android.cc (right):

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
net/android/network_change_notifier_android.cc:29: void
NetworkChangeNotifier::NotifyObserversOfConnectionTypeChange(JNIEnv* env,
On 2012/09/14 19:07:29, szym wrote:
> watch out for conflict with http://codereview.chromium.org/10905264

Keeping an eye on it.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
File net/android/network_change_notifier_android.h (right):

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
net/android/network_change_notifier_android.h:16: class NetworkChangeNotifier :
public net::NetworkChangeNotifier {
On 2012/09/14 19:07:29, szym wrote:
> Hmm, I'm not sure that using namespace android and overloading names like that
> is best for readability. You are within namespace net so NetworkChangeNotifier
> should be net::NetworkChangeNotifier. This should likely be
> NetworkChangeNotifierAndroid to match Chromium naming style.
> 
> Perhaps another CL to avoid merge conflicts, and this applies to
> net::android::NetworkChangeNotifierFactory too.

Moved it over for this CL; it'll likely land after pliard's change.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
net/android/network_change_notifier_android.h:23: void
ForceConnectivityState(bool state);
On 2012/09/14 19:07:29, szym wrote:
> What is this for? If this is for tests only, at least add a comment, but
> preferably hide it in the private section and friend the test suite.

Done.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
net/android/network_change_notifier_android.h:28: // NetworkChangeNotifier:
On 2012/09/14 19:07:29, szym wrote:
> since you're already here, make it "net::NetworkChangeNotifier:" unless you
add
> the "Android" suffix to the top-level class.

Appended the Android suffix.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
File net/android/network_change_notifier_android_unittest.cc (right):

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
net/android/network_change_notifier_android_unittest.cc:41:
net::NetworkChangeNotifier::ConnectionType current_connection_;
On 2012/09/14 19:07:29, szym wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(TestConnectionTypeObserver)

Done.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
net/android/network_change_notifier_android_unittest.cc:51: virtual void SetUp()
{
On 2012/09/14 19:07:29, szym wrote:
> Everything from here down can be protected:

Done.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
net/android/network_change_notifier_android_unittest.cc:52: notifier_.reset(new
net::android::NetworkChangeNotifier());
On 2012/09/14 19:07:29, szym wrote:
> You don't really need "net::android::" here. I understand if you want to avoid
> confusion with net::NetworkChangeNotifier and I strongly suggest renaming it
to
> NetworkChangeNotifierAndroid.

Renamed.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
net/android/network_change_notifier_android_unittest.cc:58:
net::android::NetworkChangeNotifier* notifier() {
On 2012/09/14 19:07:29, szym wrote:
> Rather than the getter I suggest a forwarding ForceConnectivityState method.
> This would allow you to make the NCN method private and just friend
> NetworkChangeNotifierAndroidTest.

Done.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/android/network...
net/android/network_change_notifier_android_unittest.cc:62:
TestConnectionTypeObserver* observer() {
On 2012/09/14 19:07:29, szym wrote:
> nit: const ... const?

Done.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/base/network_ch...
File net/base/network_change_notifier.h (right):

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/base/network_ch...
net/base/network_change_notifier.h:20: #if defined(OS_ANDROID)
On 2012/09/14 19:28:38, szym wrote:
> On 2012/09/14 19:26:34, szym wrote:
> > On 2012/09/14 19:07:29, szym wrote:
> > > You shouldn't need to forward declare for friend declaration. See
> > > NetworkChangeNotifierWinTest.
> > 
> > :( Apparently this is not true if you want to friend a class from another
> > namespace.
> > 
> >
>
http://stackoverflow.com/questions/4492062/why-does-a-c-friend-class-need-a-f...
> 
> Still, I wonder if you need #if defined.

Seems like it's fine to remove.  Had some issues with the android namespace
prior to this, so getting rid of it was kind of a boon.

https://chromiumcodereview.appspot.com/10928193/diff/3003/net/base/network_ch...
net/base/network_change_notifier.h:206: #if defined(OS_ANDROID)
On 2012/09/14 19:07:29, szym wrote:
> No need for #if here.

Done.

[benm (inactive), 2012-09-17 10:35:43]

https://chromiumcodereview.appspot.com/10928193/diff/4003/net/android/network...
File net/android/network_change_notifier_android_unittest.cc (right):

https://chromiumcodereview.appspot.com/10928193/diff/4003/net/android/network...
net/android/network_change_notifier_android_unittest.cc:75:
TEST_F(NetworkChangeNotifierAndroidTest, ObserverNotified) {
Perhaps a comment here that this is exercising calls over JNI, as it's not
obvious.

https://chromiumcodereview.appspot.com/10928193/diff/4003/net/android/network...
net/android/network_change_notifier_android_unittest.cc:85:
observer()->current_connection());
Maybe worth checking we can set it to a connected state as well?

[szym, 2012-09-17 22:08:54]

lgtm

[gone, 2012-09-17 23:34:21]

https://chromiumcodereview.appspot.com/10928193/diff/4003/net/android/network...
File net/android/network_change_notifier_android_unittest.cc (right):

https://chromiumcodereview.appspot.com/10928193/diff/4003/net/android/network...
net/android/network_change_notifier_android_unittest.cc:75:
TEST_F(NetworkChangeNotifierAndroidTest, ObserverNotified) {
On 2012/09/17 10:35:43, benm wrote:
> Perhaps a comment here that this is exercising calls over JNI, as it's not
> obvious.

Done.

https://chromiumcodereview.appspot.com/10928193/diff/4003/net/android/network...
net/android/network_change_notifier_android_unittest.cc:85:
observer()->current_connection());
On 2012/09/17 10:35:43, benm wrote:
> Maybe worth checking we can set it to a connected state as well?

Done.

[gone, 2012-09-17 23:40:16]

Adding additional OWNERS for the newly touched directories.

[benm (inactive), 2012-09-18 10:23:53]

lgtm

[jochen (gone - plz use gerrit), 2012-09-18 13:25:43]

https://chromiumcodereview.appspot.com/10928193/diff/13004/content/shell/shel...
File content/shell/shell_browser_main_parts.cc (right):

https://chromiumcodereview.appspot.com/10928193/diff/13004/content/shell/shel...
content/shell/shell_browser_main_parts.cc:88: new
net::NetworkChangeNotifierFactoryAndroid());
why is this needed?

ShellBrowserMainPartsAndroid already sets it, no?

[gone, 2012-09-18 17:05:46]

https://chromiumcodereview.appspot.com/10928193/diff/13004/content/shell/shel...
File content/shell/shell_browser_main_parts.cc (right):

https://chromiumcodereview.appspot.com/10928193/diff/13004/content/shell/shel...
content/shell/shell_browser_main_parts.cc:88: new
net::NetworkChangeNotifierFactoryAndroid());
On 2012/09/18 13:25:43, jochen wrote:
> why is this needed?
> 
> ShellBrowserMainPartsAndroid already sets it, no?

AFAICT, there is no ShellBrowserMainPartsAndroid in the tree.

[jochen (gone - plz use gerrit), 2012-09-18 19:21:05]

content/shell lgtm

https://chromiumcodereview.appspot.com/10928193/diff/13004/content/shell/shel...
File content/shell/shell_browser_main_parts.cc (right):

https://chromiumcodereview.appspot.com/10928193/diff/13004/content/shell/shel...
content/shell/shell_browser_main_parts.cc:88: new
net::NetworkChangeNotifierFactoryAndroid());
On 2012/09/18 17:05:46, dfalcantara wrote:
> On 2012/09/18 13:25:43, jochen wrote:
> > why is this needed?
> > 
> > ShellBrowserMainPartsAndroid already sets it, no?
> 
> AFAICT, there is no ShellBrowserMainPartsAndroid in the tree.

Argh, I misread ChromeBrowserMainPartsAndroid as Shell... sorry

[gone, 2012-09-19 17:29:09]

Adding another OWNER for the chrome/browser change.  PTAL if you can.

[Yaron, 2012-09-19 23:23:11]

lgtm for net/android

I think you can just tbr the chrome/browser change as it's a trivial rename.
Just add TBR=thakis@chromium.org to your change description.

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

https://chromiumcodereview.appspot.com/10928193/diff/13004/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:58: static
NetworkChangeNotifier createInstance(Context context, int nativeChangeNotifier)
{
This can be private too, right? Only called via jni?

[gone, 2012-09-19 23:26:26]

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

https://chromiumcodereview.appspot.com/10928193/diff/13004/net/android/java/s...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:58: static
NetworkChangeNotifier createInstance(Context context, int nativeChangeNotifier)
{
On 2012/09/19 23:23:11, Yaron wrote:
> This can be private too, right? Only called via jni?

I need this guy to be package private for the Java-side unit test that's in the
follow-up CL.  Should I make it private for now?

[Nico, 2012-09-19 23:38:23]

chrome lgtm

[commit-bot: I haz the power, 2012-09-20 00:01:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/10928193/20002

[Philippe, 2012-09-20 01:27:00]

On 2012/09/20 00:01:49, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/dfalcantara%40chromium.org/10928193/20002

I see that net::android::AndroidNetworkChangeNotifier was renamed to
net::AndroidNetworkChangeNotifierAndroid which is good I think to be consistent
with other implementations.
However I see it's still under net/android/ rather than net/base/ like other
platforms.
Do we have strong reasons to keep it there?

[szym, 2012-09-20 01:58:09]

On 2012/09/20 01:27:00, Philippe wrote:
> On 2012/09/20 00:01:49, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
>
https://chromium-status.appspot.com/cq/dfalcantara%2540chromium.org/10928193/...
> 
> I see that net::android::AndroidNetworkChangeNotifier was renamed to
> net::AndroidNetworkChangeNotifierAndroid which is good I think to be
consistent
> with other implementations.
> However I see it's still under net/android/ rather than net/base/ like other
> platforms.
> Do we have strong reasons to keep it there?

I think it makes sense for it to remain in net/android. The general trend should
be to move things *out* of net/base rather than into. However, you'll find that
within net/ sub-directories don't map to sub-namespaces.

In this case the naming NetworkChangeNotifierXXXX is preferred since those are
implementations of the NetworkChangeNotifier interface. Using net::android::XXXX
to override net::XXXX hinders readability and requires namespace qualifications
anyway.

[commit-bot: I haz the power, 2012-09-20 02:27:15]

Change committed as 157687