Add NetworkChangeNotifier connectivity events to the NetLog.

Add NetworkChangeNotifier connectivity events to the NetLog.


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

[pauljensen, 2012-10-25 16:14:45]

Matt, could you review this little change?  It looks like when 
LoggingNetworkChangeObserver was added the NetworkChangeNotifier only had one
signal.  Seeing the other signal in the NetLog will definitly help investigate
NetworkChangeNotifier problems.  Thanks!

[mmenke, 2012-10-25 16:25:31]

http://codereview.chromium.org/11270039/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/11270039/diff/1/chrome/browser/io_thread.cc#ne...
chrome/browser/io_thread.cc:268: virtual void OnConnectionTypeChanged(
While you're here, could you make this and OnIPAddressChanged both OVERRIDE? 
Thanks.

http://codereview.chromium.org/11270039/diff/1/net/base/net_log_event_type_li...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/11270039/diff/1/net/base/net_log_event_type_li...
net/base/net_log_event_type_list.h:1303: //     "new_type": <Type of the new
connection>
Suggest either new_connection_type, or just connection_type, to be clearer.  Or
connect instead of connection.  Alternatively, could rename the event to
"NETWORK_CONNECTION_CHANGED".

http://codereview.chromium.org/11270039/diff/1/net/base/network_change_notifi...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/11270039/diff/1/net/base/network_change_notifi...
net/base/network_change_notifier.cc:272: }
To avoid regressions, I suggest putting these in a character array instead, so
you can assert on its length:

static const char* kConnectionNames[] = {
  "CONNECTION_UNKNOWN",
  ...
};
COMPILE_ASSERT(arraysize(kConnectionNames) == CONNECTION_NONE+ 1,
               ConnectionType_name_count_mismatch);

Could also just move connection types into another file, with a macro, like the
NetLog event types.  Either is completely fine with me.

[pauljensen, 2012-10-25 17:08:14]

Thanks for the comments!  I believe I have addressed them all.

http://codereview.chromium.org/11270039/diff/1/net/base/network_change_notifi...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/11270039/diff/1/net/base/network_change_notifi...
net/base/network_change_notifier.cc:272: }
On 2012/10/25 16:25:31, Matt Menke wrote:
> To avoid regressions, I suggest putting these in a character array instead, so
> you can assert on its length:
> 
> static const char* kConnectionNames[] = {
>   "CONNECTION_UNKNOWN",
>   ...
> };
> COMPILE_ASSERT(arraysize(kConnectionNames) == CONNECTION_NONE+ 1,
>                ConnectionType_name_count_mismatch);
> 
> Could also just move connection types into another file, with a macro, like
the
> NetLog event types.  Either is completely fine with me.

I copied the method of NetLog::EventPhaseToString().
Either of the ways you mention are great; for small enum's like this I prefer to
avoid the #include method as its a little harder to debug.  I'll copy the method
of HttpAuth::SchemeToString().
There's also the method of removing the NOTREACHED(); return NULL; and letting
the compiler warning catch it as rsleevi recommends here:
http://codereview.chromium.org/11115009/diff/6003/net/base/network_change_not...

[mmenke, 2012-10-25 17:34:57]

LGTM!  And thanks for adding this - sure it'll come in handy in the future.

http://codereview.chromium.org/11270039/diff/1/net/base/network_change_notifi...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/11270039/diff/1/net/base/network_change_notifi...
net/base/network_change_notifier.cc:272: }
On 2012/10/25 17:08:15, pauljensen wrote:
> On 2012/10/25 16:25:31, Matt Menke wrote:
> > To avoid regressions, I suggest putting these in a character array instead,
so
> > you can assert on its length:
> > 
> > static const char* kConnectionNames[] = {
> >   "CONNECTION_UNKNOWN",
> >   ...
> > };
> > COMPILE_ASSERT(arraysize(kConnectionNames) == CONNECTION_NONE+ 1,
> >                ConnectionType_name_count_mismatch);
> > 
> > Could also just move connection types into another file, with a macro, like
> the
> > NetLog event types.  Either is completely fine with me.
> 
> I copied the method of NetLog::EventPhaseToString().
> Either of the ways you mention are great; for small enum's like this I prefer
to
> avoid the #include method as its a little harder to debug.  I'll copy the
method
> of HttpAuth::SchemeToString().
> There's also the method of removing the NOTREACHED(); return NULL; and letting
> the compiler warning catch it as rsleevi recommends here:
>
http://codereview.chromium.org/11115009/diff/6003/net/base/network_change_not...

Rsleevi's method is nifty, too.  EventPhaseToString is a bit different, in that
I don't think we'll ever be adding another event phase - here, I expect we will
be.

[pauljensen, 2012-10-25 21:34:54]

Scott, could you approve chrome/browser/io_thread.cc?  Matt has already reviewed
the change as a whole.

[sky, 2012-10-25 21:57:09]

I'm not a good reviewer for io_thread. Maybe John.

[jam, 2012-10-26 00:47:13]

On 2012/10/25 21:57:09, sky wrote:
> I'm not a good reviewer for io_thread. Maybe John.

rubberstamp lgtm, i defer to mmenke

[commit-bot: I haz the power, 2012-10-26 13:38:00]

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

[commit-bot: I haz the power, 2012-10-27 12:22:40]

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

[commit-bot: I haz the power, 2012-10-27 12:31:29]

Change committed as 164531