Implement new network change notifier that uses NetworkStateHandler

Implement new network change notifier that uses NetworkStateHandler

[This is a reland of 174127 which failed clang bots due to missing virtual
keyword on OVERRIDEs.]

This implements the new network change notifier that informs the Chrome
network stack of connectivity changes.

It's currently behind the --enable-new-network-handlers command line flag.

BUG=chromium-os:159647
TEST=unit tests, gmerged on a device and ensured offline->online and
     online->offline transitions are correctly detected.


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

[gauravsh, 2012-12-09 23:20:17]

Here's an initial patch which I think is ready for review and feedback. There
are a couple of things to note:

1) The name ConnectionChangeNotifier is just a placeholder. I didn't want
conflict with the existing NetworkChangeNotifierChromeos. What's the recommended
way when rewriting a new implementation with the intention of eventually
removing the old one?

2) checkdeps currently complains about depending on content/ (used for
PostTask). I was under the impression that we are trying to take as much out of
chrome/browser/chromeos as possible. So is there a better place for this? Or
should I just make the change to the chromeos/DEPS file?

3) Due to the late processing of about:flags - this new change handler only
works if --enable-new-network-handlers is passed in at the command line. The
network change notifier factory decision needs to be made before about:flags is
processed.

[Greg Spencer (Chromium), 2012-12-10 19:25:03]

On 2012/12/09 23:20:17, gauravsh wrote:
> Here's an initial patch which I think is ready for review and feedback. There
> are a couple of things to note:
> 
> 1) The name ConnectionChangeNotifier is just a placeholder. I didn't want
> conflict with the existing NetworkChangeNotifierChromeos. What's the
recommended
> way when rewriting a new implementation with the intention of eventually
> removing the old one?

I don't think there's a prescribed way of doing it.  I think it's perfectly fine
to use another name and just put a TODO in there to make it clear what it's
going to eventually replace.  If you're worried about it, you could do something
like NewNetworkChangeNotifierChromeos, and then when it replaces the other one,
rename it to NetworkChangeNotifierChromeos again.

> 2) checkdeps currently complains about depending on content/ (used for
> PostTask). I was under the impression that we are trying to take as much out
of
> chrome/browser/chromeos as possible. So is there a better place for this? Or
> should I just make the change to the chromeos/DEPS file?


Seems to me like PostTask is a perfectly reasonable dependency here, but perhaps
content needs a low-level and high-level split.  That's probably out of scope of
this CL though.

> 3) Due to the late processing of about:flags - this new change handler only
> works if --enable-new-network-handlers is passed in at the command line. The
> network change notifier factory decision needs to be made before about:flags
is
> processed.

Eventually, this will be the default, and we can make the decision early enough.
 The alternative would be to look at an env var or something, which feels kind
of messy.

[Greg Spencer (Chromium), 2012-12-10 19:30:17]

On 2012/12/10 19:25:03, Greg Spencer (Chromium) wrote:
> Eventually, this will be the default, and we can make the decision early
enough.
>  The alternative would be to look at an env var or something, which feels kind
> of messy.

I see what you mean now: it's only the about:flags, sorry, I was thinking you
were saying it was command line flags that weren't processed yet.  I'm not sure
what the solution is here for non-developers.  Is there any way to move
about:flags processing sooner?

[stevenjb, 2012-12-10 19:34:22]

My suggestion is to rename the existing class
chromeos::NetworkChangeNotifierNetworkLibrary (long, but we're planning to
deprecate it anyway) and use chromeos::NetworkChangeNotifierChromeOS for the new
class. We also discussed moving the new class to src/net.

Re: about:flags, I am investigating moving the about:flags processing sooner, I
think that it would be generally worth the effort.

[Greg Spencer (Chromium), 2012-12-10 22:54:55]

LGTM, modulo the arraysize thing below.

https://codereview.chromium.org/11469044/diff/4001/chromeos/network/connectio...
File chromeos/network/connection_change_notifier_unittest.cc (right):

https://codereview.chromium.org/11469044/diff/4001/chromeos/network/connectio...
chromeos/network/connection_change_notifier_unittest.cc:54: for (size_t i = 0; i
< ARRAYSIZE_UNSAFE(type_mappings); ++i) {
Why can't you use arraysize() here?

[gauravsh, 2012-12-11 00:39:18]

I would hold off on reviewing this patch. As per my discussion with Steven, I am
working on moving the notifier to net/base (where the linux, mac and windows)
implementations also live.

https://codereview.chromium.org/11469044/diff/4001/chromeos/network/connectio...
File chromeos/network/connection_change_notifier_unittest.cc (right):

https://codereview.chromium.org/11469044/diff/4001/chromeos/network/connectio...
chromeos/network/connection_change_notifier_unittest.cc:54: for (size_t i = 0; i
< ARRAYSIZE_UNSAFE(type_mappings); ++i) {
On 2012/12/10 22:54:55, Greg Spencer (Chromium) wrote:
> Why can't you use arraysize() here?

arraysize() doesn't work on anonymous/structs defined inside functions.

[gauravsh, 2012-12-11 03:17:13]

I moved the notifier code to net/base.

szym: For OWNER reviewing everything under net/. This is a new version of the
NetworkChangeNotifier for Chrome OS. In particular, please let me know if it's
ok to change net/base/DEPS to pull in low-level Chrome OS specific dependencies
under chromeos/ and third_party/cros_system_api.

[szym, 2012-12-12 20:14:08]

willchan: PTAL at the change to net/base/DEPS
Is it okay for net/ to depend on chromeos/?

[willchan no longer on Chromium, 2012-12-12 21:09:18]

I think that's probably a layering violation. I don't know for sure. I'm not
familiar enough with the chromeos package to tell, but I suspect this is not the
right way to do it.

[szym, 2012-12-12 21:11:19]

On 2012/12/12 21:09:18, willchan wrote:
> I think that's probably a layering violation. I don't know for sure. I'm not
> familiar enough with the chromeos package to tell, but I suspect this is not
the
> right way to do it.

Indeed, chromeos/DEPS includes +net so this introduces a dependency cycle.

[stevenjb, 2012-12-13 19:06:25]

On 2012/12/12 21:11:19, szym wrote:
> On 2012/12/12 21:09:18, willchan wrote:
> > I think that's probably a layering violation. I don't know for sure. I'm not
> > familiar enough with the chromeos package to tell, but I suspect this is not
> the
> > right way to do it.
> 
> Indeed, chromeos/DEPS includes +net so this introduces a dependency cycle.

Ah, interesting, I didn't realize that chromeos/ already depends on net/.
I think we will need to move NetworkChangeNotifierChromeos into src/chromeos
(which probably makes sense anyway).

[stevenjb, 2012-12-13 19:28:22]

https://codereview.chromium.org/11469044/diff/12001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/11469044/diff/12001/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:395: LOG(ERROR) <<
"Using new connection change notifier.";
Let's make this a WARNING instead of an ERROR, it will still show up in the
logs.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
File net/base/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:50: dns_changed = false;
Use a separate line for each of these.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:69: {}
{ on line 68

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:72: {}
{ on line 71

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:90: *connection_type_changed =
*ip_address_changed = *dns_changed = false;
Use separate lines for each, here and below.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:92: //   support is hooked into
NetworkStateHandler. For now,
nit: No extra WS indentation in comment

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:154: return CONNECTION_2G;  //
Default cellular type is 2G.
{} around each of these clauses for multi-line if()

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
File net/base/network_change_notifier_chromeos.h (right):

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:27: // changes from the network
state handler.
Simplify to: Starts observing changes from the network state handler.
(Initialize is obvious from the name).

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:31: // changes from the network
state handler.
dito. s/Stop/Stops

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:47: class DnsConfigService : public
net::internal::DnsConfigServicePosix {
We can define this only in the .cc file and forward declare it here. Also it
should have a comment describing the class.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:63: void UpdateActiveNetwork(const
chromeos::NetworkState* active_network,
It looks like this doesn't actually do any updating, just sets *changed? We
should probably rename this to something like "GetChangesForActiveNetwork".

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:78:
NetworkChangeNotifier::ConnectionType connection_type_;
We should be consistent with whether or not we use the NetworkChangeNotifier::
qualifier here. We should either use it fin front of
NetworkChangeCalculatorParams, or remove it before ConnectionType here and
elsewhere. (I think that removing it is fine and more common).

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:87: ConnectionTypeFromShill);
FRIEND* should be just below private

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
File net/base/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos_unittest.cc:23: } type_mappings[] = {
nit: define named struct separately

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos_unittest.cc:24:
{flimflam::kTypeEthernet, "", NetworkChangeNotifier::CONNECTION_ETHERNET},
' ' after { and before }

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos_unittest.cc:84: // owns the returned
pointer and must delete it.
Update comment (no returned pointer)

[gauravsh, 2012-12-15 00:42:45]

PTAL

https://codereview.chromium.org/11469044/diff/12001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/11469044/diff/12001/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:395: LOG(ERROR) <<
"Using new connection change notifier.";
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> Let's make this a WARNING instead of an ERROR, it will still show up in the
> logs.

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
File net/base/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:50: dns_changed = false;
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> Use a separate line for each of these.

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:69: {}
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> { on line 68

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:72: {}
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> { on line 71

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:90: *connection_type_changed =
*ip_address_changed = *dns_changed = false;
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> Use separate lines for each, here and below.

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:92: //   support is hooked into
NetworkStateHandler. For now,
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> nit: No extra WS indentation in comment

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.cc:154: return CONNECTION_2G;  //
Default cellular type is 2G.
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> {} around each of these clauses for multi-line if()

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
File net/base/network_change_notifier_chromeos.h (right):

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:27: // changes from the network
state handler.
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> Simplify to: Starts observing changes from the network state handler.
> (Initialize is obvious from the name).

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:31: // changes from the network
state handler.
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> dito. s/Stop/Stops

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:47: class DnsConfigService : public
net::internal::DnsConfigServicePosix {
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> We can define this only in the .cc file and forward declare it here. Also it
> should have a comment describing the class.

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:63: void UpdateActiveNetwork(const
chromeos::NetworkState* active_network,
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> It looks like this doesn't actually do any updating, just sets *changed? We
> should probably rename this to something like "GetChangesForActiveNetwork".
Changed this to UpdateState(), since it's changing notifier state based on the
active n/w update.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:78:
NetworkChangeNotifier::ConnectionType connection_type_;
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> We should be consistent with whether or not we use the NetworkChangeNotifier::
> qualifier here. We should either use it fin front of
> NetworkChangeCalculatorParams, or remove it before ConnectionType here and
> elsewhere. (I think that removing it is fine and more common).

Made it consistent. I prefer the qualifier because there are plenty of name
collisions (NetworkState in chromeos:: and net::, ConnectionType in chromeos::
and net::).

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos.h:87: ConnectionTypeFromShill);
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> FRIEND* should be just below private

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
File net/base/network_change_notifier_chromeos_unittest.cc (right):

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos_unittest.cc:23: } type_mappings[] = {
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> nit: define named struct separately

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos_unittest.cc:24:
{flimflam::kTypeEthernet, "", NetworkChangeNotifier::CONNECTION_ETHERNET},
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> ' ' after { and before }

Done.

https://codereview.chromium.org/11469044/diff/12001/net/base/network_change_n...
net/base/network_change_notifier_chromeos_unittest.cc:84: // owns the returned
pointer and must delete it.
On 2012/12/13 19:28:22, stevenjb (chromium) wrote:
> Update comment (no returned pointer)

Done.

[szym, 2012-12-17 06:16:53]

https://codereview.chromium.org/11469044/diff/21002/chromeos/network/network_...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/11469044/diff/21002/chromeos/network/network_...
chromeos/network/network_state.h:11: class
NetworkChangeNotifierChromeosUpdateTest;
This class is now in chromeos:: so forward declaration is not needed.

[gauravsh, 2012-12-17 21:05:11]

https://codereview.chromium.org/11469044/diff/21002/chromeos/network/network_...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/11469044/diff/21002/chromeos/network/network_...
chromeos/network/network_state.h:11: class
NetworkChangeNotifierChromeosUpdateTest;
On 2012/12/17 06:16:53, szym wrote:
> This class is now in chromeos:: so forward declaration is not needed.

Done.

[stevenjb, 2012-12-19 20:26:15]

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:32:
NetworkChangeNotifierChromeos::DnsConfigService::DnsConfigService() {}
nit: } on new line

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:34:
NetworkChangeNotifierChromeos::DnsConfigService::~DnsConfigService() {}
ditto

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:108: // support is hooked
into NetworkStateHandler. For now,
NetworkStateHandler does support ip config now - it's per network.
NetworkServiceChanged(network) will get called when any network's IP address
changes.

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:114: // we should never
ever get duplicate no active network notifications).
nit: s/never ever/never/

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:122: }
return here and remove else {}

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:130: <<
new_connection_type;
nit: align << with <<

[gauravsh, 2012-12-19 20:36:31]

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:32:
NetworkChangeNotifierChromeos::DnsConfigService::DnsConfigService() {}
On 2012/12/19 20:26:16, stevenjb (chromium) wrote:
> nit: } on new line

Done.

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:34:
NetworkChangeNotifierChromeos::DnsConfigService::~DnsConfigService() {}
On 2012/12/19 20:26:16, stevenjb (chromium) wrote:
> ditto

Done.

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:108: // support is hooked
into NetworkStateHandler. For now,
On 2012/12/19 20:26:16, stevenjb (chromium) wrote:
> NetworkStateHandler does support ip config now - it's per network.
> NetworkServiceChanged(network) will get called when any network's IP address
> changes.

This is about being able to query new ip config parameters to let us determine,
for example, if the DNS configuration changed.

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:114: // we should never
ever get duplicate no active network notifications).
On 2012/12/19 20:26:16, stevenjb (chromium) wrote:
> nit: s/never ever/never/

Done.

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:122: }
On 2012/12/19 20:26:16, stevenjb (chromium) wrote:
> return here and remove else {}

Done.

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:130: <<
new_connection_type;
On 2012/12/19 20:26:16, stevenjb (chromium) wrote:
> nit: align << with <<

Done.

[stevenjb, 2012-12-19 20:44:19]

lgtm

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
File chromeos/network/network_change_notifier_chromeos.cc (right):

https://codereview.chromium.org/11469044/diff/31001/chromeos/network/network_...
chromeos/network/network_change_notifier_chromeos.cc:108: // support is hooked
into NetworkStateHandler. For now,
On 2012/12/19 20:36:31, gauravsh wrote:
> On 2012/12/19 20:26:16, stevenjb (chromium) wrote:
> > NetworkStateHandler does support ip config now - it's per network.
> > NetworkServiceChanged(network) will get called when any network's IP address
> > changes.
> 
> This is about being able to query new ip config parameters to let us
determine,
> for example, if the DNS configuration changed.

Ah, OK. We should sync up about what we need there, but we don't need to do that
for this CL.

[commit-bot: I haz the power, 2012-12-19 21:21:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/11469044/40001

[commit-bot: I haz the power, 2012-12-20 03:38:49]

Retried try job too often on win_rel for step(s) content_browsertests

[commit-bot: I haz the power, 2012-12-20 07:39:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/11469044/40001

[commit-bot: I haz the power, 2012-12-20 10:28:51]

Change committed as 174127

[commit-bot: I haz the power, 2012-12-20 19:39:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/11469044/44002

[commit-bot: I haz the power, 2012-12-21 19:27:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/11469044/44002

[commit-bot: I haz the power, 2012-12-21 19:28:31]

Change committed as 174431