Add wifi AP info to system profile metrics

chrome/browser/metrics/network_metrics_provider.cc

Index: chrome/browser/metrics/network_metrics_provider.cc
diff --git a/chrome/browser/metrics/network_metrics_provider.cc b/chrome/browser/metrics/network_metrics_provider.cc
index b339d4b93adcddd000756500e58073e19f3c359a..bb81a8b0d8cbc92e5b079d8a27400a4b340e1755 100644
--- a/chrome/browser/metrics/network_metrics_provider.cc
+++ b/chrome/browser/metrics/network_metrics_provider.cc
@@ -5,6 +5,10 @@
 #include "chrome/browser/metrics/network_metrics_provider.h"
 
 #include "base/compiler_specific.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"

[Ilya Sherman, 2014/07/10 00:39:09]

Where do you use UTF string conversions?  (Am I just overlooking it?)

[zqiu1, 2014/07/10 18:11:55]

Not needed. Will remove.

 #include "base/task_runner_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "content/public/browser/browser_thread.h"
@@ -39,6 +43,12 @@ void NetworkMetricsProvider::ProvideSystemProfileMetrics(
   // TODO(isherman): This line seems unnecessary.
   connection_type_ = net::NetworkChangeNotifier::GetConnectionType();
   wifi_phy_layer_protocol_is_ambiguous_ = false;
+
+  // Connected wifi AP information.
+  net::NetworkChangeNotifier::WifiApInfo info;
+  if (net::NetworkChangeNotifier::GetWifiApInfo(&info)) {
+    WriteWifiApProto(info, network);
+  }

[Ilya Sherman, 2014/07/10 00:39:10]

nit: No need for curly braces.

[zqiu1, 2014/07/10 18:11:55]

Done.

 }
 
 void NetworkMetricsProvider::OnConnectionTypeChanged(
@@ -116,3 +126,80 @@ void NetworkMetricsProvider::OnWifiPHYLayerProtocolResult(
   }
   wifi_phy_layer_protocol_ = mode;
 }
+
+void NetworkMetricsProvider::WriteWifiApProto(
+    const net::NetworkChangeNotifier::WifiApInfo &info,
+    SystemProfileProto::Network *network_proto) {

[Ilya Sherman, 2014/07/10 00:39:09]

Note: The nit from the header file applies here as well.

[zqiu1, 2014/07/10 18:11:55]

Done.

+  SystemProfileProto::Network::WifiAccessPoint* ap_info =
+      network_proto->mutable_access_point_info();
+
+  SystemProfileProto::Network::WifiAccessPoint::SecurityMode security;
+  switch (info.security) {
+    case net::NetworkChangeNotifier::WIFI_SECURITY_NONE:
+      security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_NONE;
+      break;
+    case net::NetworkChangeNotifier::WIFI_SECURITY_WPA:
+      security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_WPA;
+      break;
+    case net::NetworkChangeNotifier::WIFI_SECURITY_WEP:
+      security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_WEP;
+      break;
+    case net::NetworkChangeNotifier::WIFI_SECURITY_RSN:
+      security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_RSN;
+      break;
+    case net::NetworkChangeNotifier::WIFI_SECURITY_802_1X:
+      security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_802_1X;
+      break;
+    case net::NetworkChangeNotifier::WIFI_SECURITY_PSK:
+      security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_PSK;
+      break;
+    default:

[Ilya Sherman, 2014/07/10 00:39:10]

Please replace the default case with explicit handling of
net::NetworkChangeNotifier::UNKNOWN.  That way, if a new member is ever added to
that enum, the compiler will remind you to update this switch stmt as well.

[zqiu1, 2014/07/10 18:11:55]

Done.

+      security = SystemProfileProto::Network::WifiAccessPoint::SECURITY_UNKNOWN;
+      break;
+  }
+  ap_info->set_security_mode(security);
+
+  // |bssid| is xx:xx:xx:xx:xx:xx, extract the first three components and
+  // pack into a uint32.
+  std::string bssid = info.bssid;
+  if (bssid.size() > 9 && bssid[2] == ':' && bssid[5] == ':' &&
+        bssid[8] == ':') {

[Ilya Sherman, 2014/07/10 00:39:09]

Why not check the full format, rather than just the prefix?  Is it ever valid to
have a format like "xx:xx:xx:xxxxxx"?

[Ilya Sherman, 2014/07/10 00:39:10]

nit: Please de-indent this line by two spaces.

[zqiu1, 2014/07/10 18:11:55]

I think the reason was that we only care about the prefix, but it definitely
doesn't hurt to check the full format. I will update it.

+      std::string vendor_prefix_str;
+      uint32 vendor_prefix;
+
+      base::RemoveChars(bssid.substr(0, 9), ":", &vendor_prefix_str);
+      DCHECK_EQ(6U, vendor_prefix_str.size());
+      base::HexStringToUInt(vendor_prefix_str, &vendor_prefix);

[Ilya Sherman, 2014/07/10 00:39:10]

What if this conversion fails?

[zqiu1, 2014/07/10 18:11:55]

We will log a warning message.

+
+      ap_info->set_vendor_prefix(vendor_prefix);
+  }
+
+  // Fill in vendor information if it is provided.
+  if (!info.model_number.empty() || !info.model_name.empty() ||
+      !info.device_name.empty() || !info.oui_list.empty()) {

[Ilya Sherman, 2014/07/10 00:39:09]

Hmm, how about creating a local VendorInformation message, and swapping in its
contents if it is nonempty, rather than needing to duplicate the if-stmts?

[zqiu1, 2014/07/10 18:11:55]

The outer if-stmts are not needed, I will just obtain the mutable pointer
regardless, and stuff in the data if they're present. Let me know if you think
this will cause any problems.

+    SystemProfileProto::Network::WifiAccessPoint::VendorInformation *vendor =

[Ilya Sherman, 2014/07/10 00:39:09]

nit: Swap the space and asterisk

[zqiu1, 2014/07/10 18:11:55]

Done.

+        ap_info->mutable_vendor_info();
+    if (!info.model_number.empty()) {
+      vendor->set_model_number(info.model_number);
+    }
+    if (!info.model_name.empty()) {
+      vendor->set_model_name(info.model_name);
+    }
+    if (!info.device_name.empty()) {
+      vendor->set_device_name(info.device_name);
+    }

[Ilya Sherman, 2014/07/10 00:39:09]

nit: No need for curly braces for one-line if-stmts.

[zqiu1, 2014/07/10 18:11:55]

Done.

+
+    // Parse OUI list.
+    if (!info.oui_list.empty()) {
+      std::vector<std::string> oui_list;
+      base::SplitString(info.oui_list, ' ', &oui_list);
+      for (std::vector<std::string>::const_iterator i = oui_list.begin();

[Ilya Sherman, 2014/07/10 00:39:10]

nit: "i" -> "it"

[zqiu1, 2014/07/10 18:11:55]

Done.

+           i != oui_list.end();
+           ++i) {
+        uint32 oui;
+        base::HexStringToUInt(*i, &oui);

[Ilya Sherman, 2014/07/10 00:39:09]

What happens if this conversion fails?

[zqiu1, 2014/07/10 18:11:55]

We will log a warning message.

+        vendor->add_element_identifier(oui);
+      }
+    }
+  }
+}