Added ChromeOS-specific NetworkChangeNotifier specialization that is wired directly to flimflam.

Added ChromeOS-specific NetworkChangeNotifier specialization that is wired directly to flimflam.
This implementation will help us detect more networking issues than the regular Linux version
that was based on netlink. In addition to service/ip changes, we can now raise network change 
notification for connectivity changes (i.e. captive portal state changes). 

BUG=chromium-os:15327
TEST=make sure SPDY and Proxy settings are properly invalidated once we resume ChromeOS session
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85779

[rtc, 2011-05-16 21:49:14]

+pstew

On Mon, May 16, 2011 at 2:46 PM, <zelidrag@chromium.org> wrote:

> Reviewers: sleffler, rtc, oshima,
>
> Description:
> Added ChromeOS-specific NetworkChangeNotifier specialization that is wired
> directly to flimflam.
> This implementation will help us detect more networking issues than the
> regular
> Linux version
> that was based on netlink. In addition to service/ip changes, we can now
> raise
> network change
> notification for connectivity changes (i.e. captive portal state changes).
>
> BUG=chromium-os:15327
> TEST=make sure SPDY and Proxy settings are properly invalidated once we
> resume
> ChromeOS session
>
> Please review this at http://codereview.chromium.org/7029019/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     net/base/network_change_notifier.cc
>  A     net/base/network_change_notifier_chromeos.h
>  A     net/base/network_change_notifier_chromeos.cc
>  M     net/net.gyp
>
>
>

[Sam Leffler, 2011-05-17 02:34:28]

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:1: // Copyright
(c) 2010 The Chromium Authors. All rights reserved.
it's 2011

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:21: const int
kInvalidSocket = -1;
don't see this used

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:67: ip_address_
= lib->IPAddress();
I'm not familiar with this stuff but how do these conditions imply the ip
address changed?  Or is this notification mis-named?

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:70: if ((network
&& network->connectivity_state() != connectivity_state_) &&
Again, not familiar with semantics here, but flimflam delivers a boolean that
defines online/offline status; not sure why notification happens based on this
condition (no comments).  Also don't see how offline status ever gets delivered
via the notifier (network must always be != NULL)

[oshima, 2011-05-17 17:33:04]

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:10: #include
"base/compiler_specific.h"
move this to header (for OVERRIDE)

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:12: #include
"base/task.h"
is this used?

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:59:
lib->IPAddress() != ip_address_) {
since the condition is a bit complicated, we need a test case to verify this.

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:70: if ((network
&& network->connectivity_state() != connectivity_state_) &&
If network can be null, don't we have to notify offline state when the state
becomes connected to network being null?

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:74:
NotifyObserversOfOnlineStateChange();
I believe NetworkChangeNotifier clients expect to be called in IO thread. Is
this IO thread?

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.h (right):

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.h:10: #include
"base/memory/scoped_ptr.h"
not used?

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.h:16: class
NetworkChangeNotifierCros
NetworkChangeNotifierChromeos
(to be sync with filename)

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.h:27: std::string
ip_address_;
data member after methods. Please add description.
Do we need both connected and connectivity_state_?

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.h:32: virtual bool
IsCurrentlyOffline() const;
OVERRIDE

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.h:35: virtual void
OnNetworkManagerChanged(chromeos::NetworkLibrary* obj);
OVERRIDE

[zel, 2011-05-18 00:43:33]

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:10: #include
"base/compiler_specific.h"
On 2011/05/17 17:33:05, oshima wrote:
> move this to header (for OVERRIDE)

Done.

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:12: #include
"base/task.h"
On 2011/05/17 17:33:05, oshima wrote:
> is this used?

Done.

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:21: const int
kInvalidSocket = -1;
On 2011/05/17 02:34:28, Sam Leffler wrote:
> don't see this used

Done.

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:67: ip_address_
= lib->IPAddress();
On 2011/05/17 02:34:28, Sam Leffler wrote:
> I'm not familiar with this stuff but how do these conditions imply the ip
> address changed?  Or is this notification mis-named?

Yes, on ChromeOS side this notification should be interpreted as 'significant
connectivity change that requires invalidation of cached network-related browser
resources'. Linux version deals only with IP address change notifications from
netlink, hence the name.

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:70: if ((network
&& network->connectivity_state() != connectivity_state_) &&
On 2011/05/17 17:33:05, oshima wrote:
> If network can be null, don't we have to notify offline state when the state
> becomes connected to network being null?

This part is rewritten now. Please take a look at new set of network callbacks
routines.

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:74:
NotifyObserversOfOnlineStateChange();
On 2011/05/17 17:33:05, oshima wrote:
> I believe NetworkChangeNotifier clients expect to be called in IO thread. Is
> this IO thread?

Done.

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.h (right):

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.h:10: #include
"base/memory/scoped_ptr.h"
On 2011/05/17 17:33:05, oshima wrote:
> not used?

Done.

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.h:32: virtual bool
IsCurrentlyOffline() const;
On 2011/05/17 17:33:05, oshima wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.h:35: virtual void
OnNetworkManagerChanged(chromeos::NetworkLibrary* obj);
On 2011/05/17 17:33:05, oshima wrote:
> OVERRIDE

Done.

[oshima, 2011-05-18 01:28:45]

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/7029019/diff/1008/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:59:
lib->IPAddress() != ip_address_) {
On 2011/05/17 17:33:05, oshima wrote:
> since the condition is a bit complicated, we need a test case to verify this.

The code looks correct, but I'm still concerned about lack of test as this is
last minute change. It's your call but I'd like to have some test at least
before m12 ships.

http://codereview.chromium.org/7029019/diff/6006/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7029019/diff/6006/chrome/browser/browser_main....
chrome/browser/browser_main.cc:536: #if defined(OS_CHROMEOS)
this looks wrong?

http://codereview.chromium.org/7029019/diff/6006/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1707: RegisterFileSystemURLRequestJobFactory();
These wasn't here before. Is this intentional?
They're not ifdef'ed so will be enabled on non chromeos build as well.

[zel, 2011-05-18 01:34:00]

http://codereview.chromium.org/7029019/diff/6006/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7029019/diff/6006/chrome/browser/browser_main....
chrome/browser/browser_main.cc:536: #if defined(OS_CHROMEOS)
On 2011/05/18 01:28:45, oshima wrote:
> this looks wrong?

Yes, pressed ctrl+z too many times here. Done.

http://codereview.chromium.org/7029019/diff/6006/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1707: RegisterFileSystemURLRequestJobFactory();
On 2011/05/18 01:28:45, oshima wrote:
> These wasn't here before. Is this intentional?
> They're not ifdef'ed so will be enabled on non chromeos build as well.

Hmm. I have no idea where that comes from. Let me take it out.

[oshima, 2011-05-18 01:42:44]

http://codereview.chromium.org/7029019/diff/8010/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/7029019/diff/8010/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:46:
network->ip_address() != ip_address_))) {
All right, i think this is easier to follow, then LGTM

if (!network && has_active_network_ ||
    (network && (!has_active_network ||
                 network->service_path() != service_path_ ||
                 network->ip_address() != ip_address)) {

[zel, 2011-05-18 01:48:27]

http://codereview.chromium.org/7029019/diff/8010/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/7029019/diff/8010/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:46:
network->ip_address() != ip_address_))) {
On 2011/05/18 01:42:44, oshima wrote:
> All right, i think this is easier to follow, then LGTM
> 
> if (!network && has_active_network_ ||
>     (network && (!has_active_network ||
>                  network->service_path() != service_path_ ||
>                  network->ip_address() != ip_address)) {

Done.

[oshima, 2011-05-18 17:16:40]

LGTM

can you run try bot with -c (clobber) again?

On 2011/05/18 01:48:27, zel wrote:
>
http://codereview.chromium.org/7029019/diff/8010/chrome/browser/chromeos/net/...
> File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):
> 
>
http://codereview.chromium.org/7029019/diff/8010/chrome/browser/chromeos/net/...
> chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:46:
> network->ip_address() != ip_address_))) {
> On 2011/05/18 01:42:44, oshima wrote:
> > All right, i think this is easier to follow, then LGTM
> > 
> > if (!network && has_active_network_ ||
> >     (network && (!has_active_network ||
> >                  network->service_path() != service_path_ ||
> >                  network->ip_address() != ip_address)) {
> 
> Done.

[Sam Leffler, 2011-05-18 17:31:31]

LGTM