Add NetworkChangeNotifier::ConnectionType to SystemProfile.

Add NetworkChangeNotifier::ConnectionType to SystemProfile.

SystemProfile::Network tracks the network environment. The flag
|is_ambiguous| indicates if there was a change to that data since the
time the previous log was closed.

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

[Ilya Sherman, 2013-01-26 00:49:00]

https://codereview.chromium.org/12086008/diff/1/chrome/browser/metrics/metric...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/12086008/diff/1/chrome/browser/metrics/metric...
chrome/browser/metrics/metrics_log.cc:355: return
SystemProfileProto::Network::CONNECTION_UNKNOWN;
nit: Please omit this, so that it's a compiler error to forget to add a case to
the switch stmt.

https://codereview.chromium.org/12086008/diff/1/chrome/common/metrics/proto/s...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/12086008/diff/1/chrome/common/metrics/proto/s...
chrome/common/metrics/proto/system_profile.proto:14: // Next tag: 13
nit: Please bump this one further and use 13 for your tag to avoid conflicting
with https://chromiumcodereview.appspot.com/12036047/

https://codereview.chromium.org/12086008/diff/1/chrome/common/metrics/proto/s...
chrome/common/metrics/proto/system_profile.proto:153: optional bool is_ambiguous
= 1;
nit: Perhaps name this "connection_type_is_ambiguous"?  Or is this likely to
affect any new fields that get added to this message as well?

https://codereview.chromium.org/12086008/diff/1/chrome/common/metrics/proto/s...
chrome/common/metrics/proto/system_profile.proto:155: // See
net::NetworkChangeNotifier::ConnectionType.
nit: IMO it would be better to copy the short comment from there, rather than
pointing to that comment from here.

[szym, 2013-01-28 18:16:25]

Thanks for the quick review.

https://codereview.chromium.org/12086008/diff/1/chrome/browser/metrics/metric...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/12086008/diff/1/chrome/browser/metrics/metric...
chrome/browser/metrics/metrics_log.cc:355: return
SystemProfileProto::Network::CONNECTION_UNKNOWN;
On 2013/01/26 00:49:00, Ilya Sherman wrote:
> nit: Please omit this, so that it's a compiler error to forget to add a case
to
> the switch stmt.

If I drop this, it won't compile on gcc:

../../chrome/browser/metrics/metrics_log.cc: In member function
'metrics::SystemProfileProto_Network::ConnectionType
MetricsLog::NetworkObserver::connection_type() const':
../../chrome/browser/metrics/metrics_log.cc:356:3: error: control reaches end of
non-void function [-Werror=return-type]

I moved it out of the switch(), but it seems it will have to stay.

https://codereview.chromium.org/12086008/diff/1/chrome/common/metrics/proto/s...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/12086008/diff/1/chrome/common/metrics/proto/s...
chrome/common/metrics/proto/system_profile.proto:14: // Next tag: 13
On 2013/01/26 00:49:00, Ilya Sherman wrote:
> nit: Please bump this one further and use 13 for your tag to avoid conflicting
> with https://chromiumcodereview.appspot.com/12036047/

Done.

https://codereview.chromium.org/12086008/diff/1/chrome/common/metrics/proto/s...
chrome/common/metrics/proto/system_profile.proto:153: optional bool is_ambiguous
= 1;
On 2013/01/26 00:49:00, Ilya Sherman wrote:
> nit: Perhaps name this "connection_type_is_ambiguous"?  Or is this likely to
> affect any new fields that get added to this message as well?

Good point. I wanted to have one such flag for all of Network{}, but once we add
gateway OUI or wifi PHY mode (a/b/g/n), there's the opportunity to have specific
connection_type while the other details are ambiguous.

Done.

https://codereview.chromium.org/12086008/diff/1/chrome/common/metrics/proto/s...
chrome/common/metrics/proto/system_profile.proto:155: // See
net::NetworkChangeNotifier::ConnectionType.
On 2013/01/26 00:49:00, Ilya Sherman wrote:
> nit: IMO it would be better to copy the short comment from there, rather than
> pointing to that comment from here.

That link in that other comment is out of date. The current version of
netinfo-api (http://www.w3.org/TR/2012/WD-netinfo-api-20121129/) prefers
"bandwidth" instead of discrete types
(http://www.w3.org/TR/2011/WD-netinfo-api-20110607/).

With this concern, I think it's best to refer what is actually being recorded
here.

[Ilya Sherman, 2013-01-28 20:52:10]

LGTM, thanks.  Please keep in mind that you'll need to update the server-side
protocol buffer definition as well.

https://codereview.chromium.org/12086008/diff/1/chrome/browser/metrics/metric...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/12086008/diff/1/chrome/browser/metrics/metric...
chrome/browser/metrics/metrics_log.cc:355: return
SystemProfileProto::Network::CONNECTION_UNKNOWN;
On 2013/01/28 18:16:25, szym wrote:
> On 2013/01/26 00:49:00, Ilya Sherman wrote:
> > nit: Please omit this, so that it's a compiler error to forget to add a case
> to
> > the switch stmt.
> 
> If I drop this, it won't compile on gcc:
> 
> ../../chrome/browser/metrics/metrics_log.cc: In member function
> 'metrics::SystemProfileProto_Network::ConnectionType
> MetricsLog::NetworkObserver::connection_type() const':
> ../../chrome/browser/metrics/metrics_log.cc:356:3: error: control reaches end
of
> non-void function [-Werror=return-type]
> 
> I moved it out of the switch(), but it seems it will have to stay.

Yes, sorry, I should have been clearer: Moving this code out of the switch stmt
is exactly the right way to go.  Thanks.

[commit-bot: I haz the power, 2013-01-28 22:14:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/12086008/6003

[Mark P, 2013-01-28 23:00:37]

After this has been submitted, don't forget to update the google-internal copy
of this protocol buffer.

[commit-bot: I haz the power, 2013-01-29 01:24:47]

Change committed as 179240