Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
328793002:
Add wifi AP info to system profile metrics (Closed)
|
Created:
6 years, 6 months ago by zqiu1 Modified:
6 years, 3 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd wifi AP info to system profile metrics
Report wifi AP info to UMA system profile information stats for connected APs. The data will not be collected for the APs that contain "_nomap" in their SSID. The stats will be display/analyze in the dashboard that will be created with future CLs.
BUG=chromium:378892
TEST=Use NET_LOG messages to verify the AP vendor information. Will
also be verified in the future CLs.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290350
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : #
Total comments: 14
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : Update proto_buf for access point information. #Patch Set 8 : Ignore APs that contain _nomap in their SSID. #
Total comments: 29
Patch Set 9 : #
Total comments: 25
Patch Set 10 : #Patch Set 11 : Add default constructor/destructor to WifiAccessPointInfo struct to fix compilation error on certai… #
Total comments: 11
Patch Set 12 : #
Total comments: 9
Patch Set 13 : #Patch Set 14 : #Patch Set 15 : Using new interface to provide wifi access point information. #
Total comments: 10
Patch Set 16 : #
Total comments: 4
Patch Set 17 : #Patch Set 18 : Moved access point info provider interface to chrome/browser/metrics/ #Patch Set 19 : #
Total comments: 1
Patch Set 20 : #Patch Set 21 : Rebase to ToT #Patch Set 22 : #Patch Set 23 : #
Total comments: 6
Patch Set 24 : #Messages
Total messages: 106 (1 generated)
Hi,
So, we specifically want to avoid caching data like this. NetworkState is
intended to cache frequently used data where we require immediate access.
Instead, I suggest one of two options:
1) If you are only interested in AP vendor info for Connected networks, create a
NetworkStateHandlerObserver derived class and when DefaultNetworkChanged() is
triggered call NetworkConfigurationHandler::GetProperties() for that network,
e.g.:
ApVendorNetworkObserver::DefaultNetworkChanged(const NetworkState* network) {
if (!network || network->path() == default_network)
return;
NetworkHandler::Get()->network_configuration_handler()->GetProperties(network->path(),
base::Bind(&ApVendorNetworkObserver::ParseNetwrokProperties, GetWeakPtr()));
}
etc.
2) If you want to collect AP vendor info for all visible networks (which I think
will raise both privacy and bandwidth concerns, there can easily be O(100)
visible networks), then you could have
NetworkState::InitialPropertiesReceived(const base::DictionaryValue& properties)
parse the properties you are interested in and collected the UMA stats.
I am interested in AP vendor info for connected networks only, I will go with the first approach. But instead of creating another NetworkStateHandlerObserver, I am thinking about parsing the vendor info in NetworkChangeNotifierChromeos: NetworkHandler::Get()->network_configuration_handler()->GetProperties(network->path(), base::Bind(&NetworkChangeNotifierChromeos::ParseVendorInfo, GetWeakPtr())); The reason is that I am exposing the wifi AP info to metric provider (NetworkMetricProvider) via net::NetworkChangeNotifier, and NetworkChangeNotifierChromeos is already a dervied subclass of it. The wifi AP info will be cached in NetworkChangeNotifierChromeos. // NetworkChangeNotifier overrides. virtual net::NetworkChangeNotifier::ConnectionType GetCurrentConnectionType() const OVERRIDE; virtual bool GetCurrentWifiApInfo(net::NetworkChangeNotifier::WifiApInfo &info) OVERRIDE; Let me know if that make sense to you.
That sounds reasonable to me. Cheers, -Steven
Several comments: (1) Can you record this data as histograms? If so, that's likely a better approach. I'm happy to discuss in more detail if it would be helpful. (2) We try to avoid logging strings in UMA data, as it's too easy to accidentally include PII. Can you use something like an enum instead? (3) If you decide that histograms do not suit your needs, and you definitely want to make changes to the protocol buffer, those changes will need to land in the google3 repository first. I can help you get set up to hack on google3 if you're not sure how to do so.
https://codereview.chromium.org/328793002/diff/40001/chromeos/network/network... File chromeos/network/network_change_notifier_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/40001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:91: } nit: {} unnecessary. https://codereview.chromium.org/328793002/diff/40001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:136: shill::kWifiBSsid, &(info.bssid)); nit: () after & unnecessary here and below https://codereview.chromium.org/328793002/diff/40001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:164: // Nothing to be done, most likely the network service is already removed. Since this is a noop you can just pass network_handler::ErrorCallback() to GetProperties instead. https://codereview.chromium.org/328793002/diff/40001/net/base/network_change_... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/40001/net/base/network_change_... net/base/network_change_notifier.h:52: ~WifiApInfo(); empty constructors (and destructors for no-virtual classes or structs) are unnecessary. https://codereview.chromium.org/328793002/diff/40001/net/base/network_change_... net/base/network_change_notifier.h:153: virtual bool GetCurrentWifiApInfo(WifiApInfo &info) { return false; } virtual methods should never be inlined (only execption is an empty destructor for a pure interface class).
On 2014/06/12 20:31:07, Ilya Sherman wrote: > Several comments: > (1) Can you record this data as histograms? If so, that's likely a better > approach. I'm happy to discuss in more detail if it would be helpful. I am trying to make a dashboard for wifi that's similar to this one: https://uma.googleplex.com/bluetooth_stats/ I don't think I can use histograms for the vendor information. > (2) We try to avoid logging strings in UMA data, as it's too easy to > accidentally include PII. Can you use something like an enum instead? For security mode, I can change it to enum. I can remove the manufacturer field since that can be determine from the vendor_prefix. For the other string fields (model_number, model_name, device_name), I don't think I can change them to enum. > (3) If you decide that histograms do not suit your needs, and you definitely > want to make changes to the protocol buffer, those changes will need to land in > the google3 repository first. I can help you get set up to hack on google3 if > you're not sure how to do so. Thanks for the heads up, I didn't know the process for updating protocol buffer. I will make the changes on the google3 as well.
On 2014/06/12 20:57:59, zqiu1 wrote: > On 2014/06/12 20:31:07, Ilya Sherman wrote: > > Several comments: > > (1) Can you record this data as histograms? If so, that's likely a better > > approach. I'm happy to discuss in more detail if it would be helpful. > I am trying to make a dashboard for wifi that's similar to this one: > https://uma.googleplex.com/bluetooth_stats/ > I don't think I can use histograms for the vendor information. As I note below, I think you'll need to find some way to guarantee that vendor information does not contain PII. In doing so, I expect that you'll probably end up logging data that could be represented using sparse histograms. For context, we were historically overly permissive in what we added to the SystemProfile. The result is that there are a lot of fields that don't really make a ton of sense on many UMA platforms. That's not to say that no new fields should ever be added to the SystemProfile; but the bar is now higher. > > (2) We try to avoid logging strings in UMA data, as it's too easy to > > accidentally include PII. Can you use something like an enum instead? > For security mode, I can change it to enum. I can remove the manufacturer > field since that can be determine from the vendor_prefix. For the other > string fields (model_number, model_name, device_name), I don't think I can > change them to enum. You'll note that the Bluetooth section specifically does not use any strings, because strings could contain PII like "Ilya's WiFi hotspot". The downside is, of course, that the data is harder to interpret; but logging PII via UMA is entirely verboten. > > (3) If you decide that histograms do not suit your needs, and you definitely > > want to make changes to the protocol buffer, those changes will need to land > in > > the google3 repository first. I can help you get set up to hack on google3 if > > you're not sure how to do so. > Thanks for the heads up, I didn't know the process for updating protocol buffer. > I will make the changes on the google3 as well. Thanks.
https://codereview.chromium.org/328793002/diff/40001/chromeos/network/network... File chromeos/network/network_change_notifier_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/40001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:91: } On 2014/06/12 20:54:40, stevenjb wrote: > nit: {} unnecessary. Done. https://codereview.chromium.org/328793002/diff/40001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:136: shill::kWifiBSsid, &(info.bssid)); On 2014/06/12 20:54:40, stevenjb wrote: > nit: () after & unnecessary here and below Done. https://codereview.chromium.org/328793002/diff/40001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:164: // Nothing to be done, most likely the network service is already removed. On 2014/06/12 20:54:40, stevenjb wrote: > Since this is a noop you can just pass network_handler::ErrorCallback() to > GetProperties instead. Done. https://codereview.chromium.org/328793002/diff/40001/net/base/network_change_... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/40001/net/base/network_change_... net/base/network_change_notifier.h:52: ~WifiApInfo(); On 2014/06/12 20:54:40, stevenjb wrote: > empty constructors (and destructors for no-virtual classes or structs) are > unnecessary. Done. https://codereview.chromium.org/328793002/diff/40001/net/base/network_change_... net/base/network_change_notifier.h:153: virtual bool GetCurrentWifiApInfo(WifiApInfo &info) { return false; } On 2014/06/12 20:54:40, stevenjb wrote: > virtual methods should never be inlined (only execption is an empty destructor > for a pure interface class). Done.
https://codereview.chromium.org/328793002/diff/60001/chrome/browser/metrics/n... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/60001/chrome/browser/metrics/n... chrome/browser/metrics/network_metrics_provider.cc:49: if (net::NetworkChangeNotifier::GetWifiApInfo(info)) { Is this modifying info? If so it should pass a * not a &. https://codereview.chromium.org/328793002/diff/60001/chrome/browser/metrics/n... File chrome/browser/metrics/network_metrics_provider.h (right): https://codereview.chromium.org/328793002/diff/60001/chrome/browser/metrics/n... chrome/browser/metrics/network_metrics_provider.h:47: const net::NetworkChangeNotifier::WifiApInfo *info); non-const result parameters should follow const parameters. https://codereview.chromium.org/328793002/diff/60001/chromeos/network/network... File chromeos/network/network_change_notifier_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/60001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:88: net::NetworkChangeNotifier::WifiApInfo &info) { *info https://codereview.chromium.org/328793002/diff/60001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:131: const base::DictionaryValue& properties) { Verify that service_path == service_path_, since this is async. https://codereview.chromium.org/328793002/diff/60001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:170: wifi_ap_info_ = info; Why copy this instead of updating wifi_ap_info_ directly? I don't see any early exits. https://codereview.chromium.org/328793002/diff/60001/net/base/network_change_... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/60001/net/base/network_change_... net/base/network_change_notifier.h:161: virtual bool GetCurrentWifiApInfo(WifiApInfo &info); *info https://codereview.chromium.org/328793002/diff/60001/net/base/network_change_... net/base/network_change_notifier.h:188: static bool GetWifiApInfo(WifiApInfo &info); *info
https://codereview.chromium.org/328793002/diff/60001/chrome/browser/metrics/n... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/60001/chrome/browser/metrics/n... chrome/browser/metrics/network_metrics_provider.cc:49: if (net::NetworkChangeNotifier::GetWifiApInfo(info)) { On 2014/07/08 00:24:53, stevenjb wrote: > Is this modifying info? If so it should pass a * not a &. Done. https://codereview.chromium.org/328793002/diff/60001/chrome/browser/metrics/n... File chrome/browser/metrics/network_metrics_provider.h (right): https://codereview.chromium.org/328793002/diff/60001/chrome/browser/metrics/n... chrome/browser/metrics/network_metrics_provider.h:47: const net::NetworkChangeNotifier::WifiApInfo *info); On 2014/07/08 00:24:53, stevenjb wrote: > non-const result parameters should follow const parameters. Done. https://codereview.chromium.org/328793002/diff/60001/chromeos/network/network... File chromeos/network/network_change_notifier_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/60001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:88: net::NetworkChangeNotifier::WifiApInfo &info) { On 2014/07/08 00:24:53, stevenjb wrote: > *info Done. https://codereview.chromium.org/328793002/diff/60001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:131: const base::DictionaryValue& properties) { On 2014/07/08 00:24:53, stevenjb wrote: > Verify that service_path == service_path_, since this is async. Done. https://codereview.chromium.org/328793002/diff/60001/chromeos/network/network... chromeos/network/network_change_notifier_chromeos.cc:170: wifi_ap_info_ = info; On 2014/07/08 00:24:53, stevenjb wrote: > Why copy this instead of updating wifi_ap_info_ directly? I don't see any early > exits. The original intent was to make sure the fields in the wifi_ap_info_ is resetted, so the fields doesn't contain any old values from previous udpate. I will reset the wifi_ap_info_ directly instead. https://codereview.chromium.org/328793002/diff/60001/net/base/network_change_... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/60001/net/base/network_change_... net/base/network_change_notifier.h:161: virtual bool GetCurrentWifiApInfo(WifiApInfo &info); On 2014/07/08 00:24:53, stevenjb wrote: > *info Done. https://codereview.chromium.org/328793002/diff/60001/net/base/network_change_... net/base/network_change_notifier.h:188: static bool GetWifiApInfo(WifiApInfo &info); On 2014/07/08 00:24:53, stevenjb wrote: > *info Done.
owner lgtm for chromeos/ with parameter order fix. Please make sure that someone more familiar with UMA stats also approves this. https://codereview.chromium.org/328793002/diff/80001/chrome/browser/metrics/n... File chrome/browser/metrics/network_metrics_provider.h (right): https://codereview.chromium.org/328793002/diff/80001/chrome/browser/metrics/n... chrome/browser/metrics/network_metrics_provider.h:47: const net::NetworkChangeNotifier::WifiApInfo &info); You still have the const (input) parameter following the non-const (output) paramater. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
https://codereview.chromium.org/328793002/diff/80001/chrome/browser/metrics/n... File chrome/browser/metrics/network_metrics_provider.h (right): https://codereview.chromium.org/328793002/diff/80001/chrome/browser/metrics/n... chrome/browser/metrics/network_metrics_provider.h:47: const net::NetworkChangeNotifier::WifiApInfo &info); On 2014/07/08 21:55:08, stevenjb wrote: > You still have the const (input) parameter following the non-const (output) > paramater. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done.
https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:11: #include "base/strings/utf_string_conversions.h" Where do you use UTF string conversions? (Am I just overlooking it?) https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:51: } nit: No need for curly braces. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:132: SystemProfileProto::Network *network_proto) { Note: The nit from the header file applies here as well. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:156: default: 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. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:166: bssid[8] == ':') { nit: Please de-indent this line by two spaces. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:166: bssid[8] == ':') { Why not check the full format, rather than just the prefix? Is it ever valid to have a format like "xx:xx:xx:xxxxxx"? https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:172: base::HexStringToUInt(vendor_prefix_str, &vendor_prefix); What if this conversion fails? https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:179: !info.device_name.empty() || !info.oui_list.empty()) { 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? https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:180: SystemProfileProto::Network::WifiAccessPoint::VendorInformation *vendor = nit: Swap the space and asterisk https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:190: } nit: No need for curly braces for one-line if-stmts. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:196: for (std::vector<std::string>::const_iterator i = oui_list.begin(); nit: "i" -> "it" https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:200: base::HexStringToUInt(*i, &oui); What happens if this conversion fails? https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.h (right): https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.h:45: void WriteWifiApProto( Please add documentation for this method. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.h:47: metrics::SystemProfileProto::Network *network_proto); nit: The ampersand and the star should each precede the space, i.e. "WifiApInfo &info" should be "WifiApInfo& info". https://codereview.chromium.org/328793002/diff/140001/net/base/network_change... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/140001/net/base/network_change... net/base/network_change_notifier.h:62: struct NET_EXPORT WifiApInfo { nit: "ap" -> "access point" (applies throughout)
https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:11: #include "base/strings/utf_string_conversions.h" On 2014/07/10 00:39:09, Ilya Sherman wrote: > Where do you use UTF string conversions? (Am I just overlooking it?) Not needed. Will remove. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:51: } On 2014/07/10 00:39:10, Ilya Sherman wrote: > nit: No need for curly braces. Done. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:132: SystemProfileProto::Network *network_proto) { On 2014/07/10 00:39:09, Ilya Sherman wrote: > Note: The nit from the header file applies here as well. Done. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:156: default: On 2014/07/10 00:39:10, Ilya Sherman wrote: > 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. Done. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:166: bssid[8] == ':') { On 2014/07/10 00:39:09, Ilya Sherman wrote: > Why not check the full format, rather than just the prefix? Is it ever valid to > have a format like "xx:xx:xx:xxxxxx"? 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. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:172: base::HexStringToUInt(vendor_prefix_str, &vendor_prefix); On 2014/07/10 00:39:10, Ilya Sherman wrote: > What if this conversion fails? We will log a warning message. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:179: !info.device_name.empty() || !info.oui_list.empty()) { On 2014/07/10 00:39:09, Ilya Sherman wrote: > 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? 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. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:180: SystemProfileProto::Network::WifiAccessPoint::VendorInformation *vendor = On 2014/07/10 00:39:09, Ilya Sherman wrote: > nit: Swap the space and asterisk Done. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:190: } On 2014/07/10 00:39:09, Ilya Sherman wrote: > nit: No need for curly braces for one-line if-stmts. Done. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:196: for (std::vector<std::string>::const_iterator i = oui_list.begin(); On 2014/07/10 00:39:10, Ilya Sherman wrote: > nit: "i" -> "it" Done. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:200: base::HexStringToUInt(*i, &oui); On 2014/07/10 00:39:09, Ilya Sherman wrote: > What happens if this conversion fails? We will log a warning message. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.h (right): https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.h:45: void WriteWifiApProto( On 2014/07/10 00:39:10, Ilya Sherman wrote: > Please add documentation for this method. Done. https://codereview.chromium.org/328793002/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.h:47: metrics::SystemProfileProto::Network *network_proto); On 2014/07/10 00:39:10, Ilya Sherman wrote: > nit: The ampersand and the star should each precede the space, i.e. "WifiApInfo > &info" should be "WifiApInfo& info". Done. https://codereview.chromium.org/328793002/diff/140001/net/base/network_change... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/140001/net/base/network_change... net/base/network_change_notifier.h:62: struct NET_EXPORT WifiApInfo { On 2014/07/10 00:39:10, Ilya Sherman wrote: > nit: "ap" -> "access point" (applies throughout) Done.
https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:175: LOG(WARNING) << "Failed to convert vendor prefix: " << vendor_prefix_str; Can this be a NOTREACHED() instead? If not, I'd at least make it a DVLOG(1) or something -- I don't think this message needs to be included in production builds of Chrome. https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:179: ap_info->mutable_vendor_info(); This actually sets creates a vendor_info field. Please add code so that, if none of the fields are set, the field gets cleared again. https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:197: LOG(WARNING) << "Failed to convert element identifier: " << *it; Ditto. https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.h (right): https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.h:45: // Writes info about the wireless access points that this system had nit: "had" -> "is"? https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... File chromeos/network/network_change_notifier_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.cc:89: net::NetworkChangeNotifier::WifiAccessPointInfo *info) { Ditto https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.cc:91: // This assumes the BSSID is never empty when accesc point information exist. nit: "accesc" -> "access" https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.cc:91: // This assumes the BSSID is never empty when accesc point information exist. nit: "exist" -> "exists" https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... File chromeos/network/network_change_notifier_chromeos.h (right): https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.h:39: virtual bool GetCurrentWifiAccessPointInfo(WifiAccessPointInfo *info) nit: Swap position of space and asterisk. https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... File net/base/network_change_notifier.cc (right): https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... net/base/network_change_notifier.cc:530: NetworkChangeNotifier::WifiAccessPointInfo *info) { nit: Swap space and asterisk. https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... net/base/network_change_notifier.cc:543: bool NetworkChangeNotifier::GetWifiAccessPointInfo(WifiAccessPointInfo *info) { nit: Swap space and asterisk. https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... net/base/network_change_notifier.h:162: virtual bool GetCurrentWifiAccessPointInfo(WifiAccessPointInfo *info); nit: Swap space and asterisk. https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... net/base/network_change_notifier.h:189: static bool GetWifiAccessPointInfo(WifiAccessPointInfo *info); nit: Swap space and asterisk.
https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:175: LOG(WARNING) << "Failed to convert vendor prefix: " << vendor_prefix_str; On 2014/07/11 01:32:17, Ilya Sherman wrote: > Can this be a NOTREACHED() instead? If not, I'd at least make it a DVLOG(1) or > something -- I don't think this message needs to be included in production > builds of Chrome. Will change to NOTREACHED() https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:179: ap_info->mutable_vendor_info(); On 2014/07/11 01:32:17, Ilya Sherman wrote: > This actually sets creates a vendor_info field. Please add code so that, if > none of the fields are set, the field gets cleared again. Then that's no different than my original implementation where the vendor_info field is only created when at least one of vendor fields are provided. I am new to protobuf stuff, referring to the original comment, how do you tell if a message is empty or not? Using SpaceUsed function? https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:197: LOG(WARNING) << "Failed to convert element identifier: " << *it; On 2014/07/11 01:32:17, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.h (right): https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.h:45: // Writes info about the wireless access points that this system had On 2014/07/11 01:32:17, Ilya Sherman wrote: > nit: "had" -> "is"? Done. https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... File chromeos/network/network_change_notifier_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.cc:89: net::NetworkChangeNotifier::WifiAccessPointInfo *info) { On 2014/07/11 01:32:17, Ilya Sherman wrote: > Ditto Done. https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.cc:91: // This assumes the BSSID is never empty when accesc point information exist. On 2014/07/11 01:32:17, Ilya Sherman wrote: > nit: "exist" -> "exists" Done. https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.cc:91: // This assumes the BSSID is never empty when accesc point information exist. On 2014/07/11 01:32:17, Ilya Sherman wrote: > nit: "accesc" -> "access" Done. https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... File chromeos/network/network_change_notifier_chromeos.h (right): https://codereview.chromium.org/328793002/diff/160001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.h:39: virtual bool GetCurrentWifiAccessPointInfo(WifiAccessPointInfo *info) On 2014/07/11 01:32:17, Ilya Sherman wrote: > nit: Swap position of space and asterisk. Done. https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... File net/base/network_change_notifier.cc (right): https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... net/base/network_change_notifier.cc:530: NetworkChangeNotifier::WifiAccessPointInfo *info) { On 2014/07/11 01:32:17, Ilya Sherman wrote: > nit: Swap space and asterisk. Done. https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... net/base/network_change_notifier.cc:543: bool NetworkChangeNotifier::GetWifiAccessPointInfo(WifiAccessPointInfo *info) { On 2014/07/11 01:32:18, Ilya Sherman wrote: > nit: Swap space and asterisk. Done. https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... net/base/network_change_notifier.h:162: virtual bool GetCurrentWifiAccessPointInfo(WifiAccessPointInfo *info); On 2014/07/11 01:32:18, Ilya Sherman wrote: > nit: Swap space and asterisk. Done. https://codereview.chromium.org/328793002/diff/160001/net/base/network_change... net/base/network_change_notifier.h:189: static bool GetWifiAccessPointInfo(WifiAccessPointInfo *info); On 2014/07/11 01:32:18, Ilya Sherman wrote: > nit: Swap space and asterisk. Done.
https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/160001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:179: ap_info->mutable_vendor_info(); On 2014/07/14 17:21:57, zqiu1 wrote: > On 2014/07/11 01:32:17, Ilya Sherman wrote: > > This actually sets creates a vendor_info field. Please add code so that, if > > none of the fields are set, the field gets cleared again. > Then that's no different than my original implementation where the vendor_info > field is only created when at least one of vendor fields are provided. I am new > to protobuf stuff, referring to the original comment, how do you tell if a > message is empty or not? Using SpaceUsed function? I agree that the functionality is equivalent -- my suggestion was more about style and maintainability. I think that BytesUsed() is indeed a reasonable way to check whether the protocol buffer is empty. I encourage you to test this locally to verify. Anyhow, if you really prefer not to dig into that and leave this as-is, then LGTM.
The CQ bit was checked by zqiu@chromium.org
The CQ bit was unchecked by zqiu@chromium.org
The CQ bit was checked by zqiu@chromium.org
The CQ bit was unchecked by zqiu@chromium.org
The CQ bit was checked by zqiu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/328793002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/28574)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/28637)
The CQ bit was checked by zqiu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/328793002/200001
lgtm w/nits https://codereview.chromium.org/328793002/diff/200001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:179: !info.device_name.empty() || !info.oui_list.empty()) { nit: early exit https://codereview.chromium.org/328793002/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:189: if (!info.oui_list.empty()) { nit: early exit https://codereview.chromium.org/328793002/diff/200001/chromeos/network/networ... File chromeos/network/network_change_notifier_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/200001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.cc:171: &vendor_dict)) { nit: early exit https://codereview.chromium.org/328793002/diff/200001/net/base/network_change... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/200001/net/base/network_change... net/base/network_change_notifier.h:63: WifiAccessPointInfo(); nit: struct constructor can be inlined https://codereview.chromium.org/328793002/diff/200001/net/base/network_change... net/base/network_change_notifier.h:64: ~WifiAccessPointInfo(); No need for empty destructor in struct
https://codereview.chromium.org/328793002/diff/200001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:179: !info.device_name.empty() || !info.oui_list.empty()) { On 2014/07/15 18:16:14, stevenjb wrote: > nit: early exit Done. https://codereview.chromium.org/328793002/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:189: if (!info.oui_list.empty()) { On 2014/07/15 18:16:14, stevenjb wrote: > nit: early exit Done. https://codereview.chromium.org/328793002/diff/200001/chromeos/network/networ... File chromeos/network/network_change_notifier_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/200001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.cc:171: &vendor_dict)) { On 2014/07/15 18:16:14, stevenjb wrote: > nit: early exit Done. https://codereview.chromium.org/328793002/diff/200001/net/base/network_change... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/200001/net/base/network_change... net/base/network_change_notifier.h:63: WifiAccessPointInfo(); On 2014/07/15 18:16:14, stevenjb wrote: > nit: struct constructor can be inlined There are structs with constructor/destructor that are not inlined (for example WireAccessPoint in network_util.h). I will leave it as is for now. https://codereview.chromium.org/328793002/diff/200001/net/base/network_change... net/base/network_change_notifier.h:64: ~WifiAccessPointInfo(); On 2014/07/15 18:16:14, stevenjb wrote: > No need for empty destructor in struct I think this is still needed, encountered compilation errors on some builds without the destructor. ../../net/base/network_change_notifier.h:62:3:error: [chromium-style] Complex class/struct needs an explicit out-of-line destructor.
https://codereview.chromium.org/328793002/diff/200001/net/base/network_change... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/328793002/diff/200001/net/base/network_change... net/base/network_change_notifier.h:64: ~WifiAccessPointInfo(); On 2014/07/15 19:34:02, zqiu1 wrote: > On 2014/07/15 18:16:14, stevenjb wrote: > > No need for empty destructor in struct > I think this is still needed, encountered compilation errors on some builds > without the destructor. > ../../net/base/network_change_notifier.h:62:3:error: [chromium-style] Complex > class/struct needs an explicit out-of-line destructor. > Ah, right, I forgot about that. OK.
The CQ bit was checked by zqiu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/328793002/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
presubmit failed due to missing LGTM for files: net/base/network_change_notifier.h net/base/network_change_notifier.cc stevenjb and isherman, I am not sure which one of you is the owner of those two files, can you guys LGTM one more time so the CL can be committed? Thanks,
On 2014/07/16 16:00:28, zqiu1 wrote: > presubmit failed due to missing LGTM for files: > net/base/network_change_notifier.h > net/base/network_change_notifier.cc > > stevenjb and isherman, I am not sure which one of you is the owner of those two > files, can you guys LGTM one more time so the CL can be committed? > > Thanks, Neither of us is an owner for these two files. The list of owners is here: https://code.google.com/p/chromium/codesearch#chromium/src/net/OWNERS
But it claims you and stevenjb already own all the files in the patch: *Owners: *Assigned reviewers already OWN all files in this patch! I think it might have lost the LGTM from your two in the latest patch for nit changes. So can you try to LGTM one more time to see if that works or not? Thanks, Peter, On Wed, Jul 16, 2014 at 10:52 AM, <isherman@chromium.org> wrote: > On 2014/07/16 16:00:28, zqiu1 wrote: > >> presubmit failed due to missing LGTM for files: >> net/base/network_change_notifier.h >> net/base/network_change_notifier.cc >> > > stevenjb and isherman, I am not sure which one of you is the owner of >> those >> > two > >> files, can you guys LGTM one more time so the CL can be committed? >> > > Thanks, >> > > Neither of us is an owner for these two files. The list of owners is here: > https://code.google.com/p/chromium/codesearch#chromium/src/net/OWNERS > > https://codereview.chromium.org/328793002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
But it claims you and stevenjb already own all the files in the patch: *Owners: *Assigned reviewers already OWN all files in this patch! I think it might have lost the LGTM from your two in the latest patch for nit changes. So can you try to LGTM one more time to see if that works or not? Thanks, Peter, On Wed, Jul 16, 2014 at 10:52 AM, <isherman@chromium.org> wrote: > On 2014/07/16 16:00:28, zqiu1 wrote: > >> presubmit failed due to missing LGTM for files: >> net/base/network_change_notifier.h >> net/base/network_change_notifier.cc >> > > stevenjb and isherman, I am not sure which one of you is the owner of >> those >> > two > >> files, can you guys LGTM one more time so the CL can be committed? >> > > Thanks, >> > > Neither of us is an owner for these two files. The list of owners is here: > https://code.google.com/p/chromium/codesearch#chromium/src/net/OWNERS > > https://codereview.chromium.org/328793002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
But it claims you and stevenjb already own all the files in the patch: *Owners: *Assigned reviewers already OWN all files in this patch! I think it might have lost the LGTM from your two in the latest patch for nit changes. So can you try to LGTM one more time to see if that works or not? Thanks, Peter, On Wed, Jul 16, 2014 at 10:52 AM, <isherman@chromium.org> wrote: > On 2014/07/16 16:00:28, zqiu1 wrote: > >> presubmit failed due to missing LGTM for files: >> net/base/network_change_notifier.h >> net/base/network_change_notifier.cc >> > > stevenjb and isherman, I am not sure which one of you is the owner of >> those >> > two > >> files, can you guys LGTM one more time so the CL can be committed? >> > > Thanks, >> > > Neither of us is an owner for these two files. The list of owners is here: > https://code.google.com/p/chromium/codesearch#chromium/src/net/OWNERS > > https://codereview.chromium.org/328793002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/16 18:41:49, chromium-reviews wrote: > But it claims you and stevenjb already own all the files in the patch: > *Owners: *Assigned reviewers already OWN all files in this patch! > > I think it might have lost the LGTM from your two in the latest patch for > nit changes. So can you try to LGTM one more time to see if that works or > not? You added Darin as a reviewer for this CL. Darin is one of the two OWNERS listed under src/OWNERS. He's also ridiculously busy, and not particularly familiar with this code. Please swap in a //net reviewer instead. (Note: The *assigned* reviewers are the ones you've listed as reviewers. This list has nothing to do with who has approved your CL or not.)
Illya pointed you to the correct OWNERS file for src/net, you should pick someone from there, preferably someone who has recently worked on the file in question. +pauljensen@ -darin@ Still lgtm w/nits https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:180: return; nit: Actually, this isn't really necessary since we check empty() on each value individually. https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:193: return; nit: WS after early exit https://codereview.chromium.org/328793002/diff/220001/chromeos/network/networ... File chromeos/network/network_change_notifier_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/220001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.cc:172: return; nit: WS after early exit (or, better, use {} after multi line condition)
I didn't add Darin as a reviewer, not sure how he get assigned. I will add a reviewer from //net. On Wed, Jul 16, 2014 at 11:50 AM, <isherman@chromium.org> wrote: > On 2014/07/16 18:41:49, chromium-reviews wrote: > >> But it claims you and stevenjb already own all the files in the patch: >> *Owners: *Assigned reviewers already OWN all files in this patch! >> > > I think it might have lost the LGTM from your two in the latest patch for >> nit changes. So can you try to LGTM one more time to see if that works or >> not? >> > > You added Darin as a reviewer for this CL. Darin is one of the two OWNERS > listed under src/OWNERS. He's also ridiculously busy, and not particularly > familiar with this code. Please swap in a //net reviewer instead. > > (Note: The *assigned* reviewers are the ones you've listed as reviewers. > This > list has nothing to do with who has approved your CL or not.) > > https://codereview.chromium.org/328793002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:180: return; On 2014/07/16 19:57:29, stevenjb wrote: > nit: Actually, this isn't really necessary since we check empty() on each value > individually. This is needed so we don't create a message for VendorInformation if no vendor information is provided. https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:193: return; On 2014/07/16 19:57:29, stevenjb wrote: > nit: WS after early exit Done. https://codereview.chromium.org/328793002/diff/220001/chromeos/network/networ... File chromeos/network/network_change_notifier_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/220001/chromeos/network/networ... chromeos/network/network_change_notifier_chromeos.cc:172: return; On 2014/07/16 19:57:29, stevenjb wrote: > nit: WS after early exit (or, better, use {} after multi line condition) Done.
(Also still LGTM % my extra nit) https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:131: SystemProfileProto::Network::WifiAccessPoint* ap_info = nit: "ap" -> "access_point" https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:180: return; On 2014/07/16 19:57:29, stevenjb wrote: > nit: Actually, this isn't really necessary since we check empty() on each value > individually. This *is* necessary, because ap_info->mutable_vendor_info() actually allocates memory. (Yay, non-obvious side-effects!)
https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/220001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:131: SystemProfileProto::Network::WifiAccessPoint* ap_info = On 2014/07/16 20:03:52, Ilya Sherman wrote: > nit: "ap" -> "access_point" Done.
I think making this part of the NetworkChangeNotifier API is a bad idea as it has little to do with the purpose of the NetworkChangeNotifier. I think instead implementing similarly to the system profile wifi_phy_layer_protocol member makes sense.
I am not sure why this is a bad idea. NetworkChangeNotifier already provide APIs for getting connection type and dns configuration. I don't see why there is a problem with adding accessor for access point information. The access point information is currently maintained by NetworkChangeNotifierChromeos which is subclass of NetworkChangeNotifier. So adding the accessor API for access point information in NetworkChangeNotifier seems to be the most straight forward thing to do. Even if I add an API similar to GetWifiPHYLayerProtocol (only support for OS_WIN currently) for access point information to net_util.h, I would still need to figure out a way to retrieve the information from chromeos (under chromeos/network/). I am pretty new to the chromium code base, so I maybe missing something. On Thu, Jul 17, 2014 at 7:12 AM, <pauljensen@chromium.org> wrote: > I think making this part of the NetworkChangeNotifier API is a bad idea as > it > has little to do with the purpose of the NetworkChangeNotifier. > I think instead implementing similarly to the system profile > wifi_phy_layer_protocol member makes sense. > > https://codereview.chromium.org/328793002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
NetworkChangeNotifier serves two basic purposes: 1. track network state changes so we have a unified signal indicating when to flush stale state (e.g. DNS cache, socket pools etc) 2. track connectivity so we have a unified signal for updating navigator.onLine and navigator.connection.type, and initiating server communication (e.g. Chrome sync) In both cases it's tasked with tracking OS signals and producing timely notifications. There are 50+ places in Chrome that listen to these signals. It is not a good place to add synchronous state inspection APIs. net_util.h IMHO is, prior art being GetNetworkList(), GetWifiPHYLayerProtocol(), GetHostName().
I tend to agree that this seems like an overloaded use of NetworkChangeNotifer. Looking more closely at the overall CL (I've been busy so i only focused on the Chrome OS specific network code before, I'm not really a maintainer of NCN), it seems like what you really want to do is create an OS specific helper class for NetworkMetricsProvider and make it a NetworkChangeNotifer::NetworkChangeObserver. Then it can retrieve the AP data whenever the network changes and NMP can retrieve that data from it. i.e. move the code you put in NCNChromeos to something like NetworkMetricsProviderAccessPointChromeos. (Alternately, since NMPAPChromeos will be chromeos specific, it could observe NetworkStateHandler directly for DefaultNetworkChanged events and not involve NCN at all). Does that make sense? On Thu, Jul 17, 2014 at 11:34 AM, <pauljensen@chromium.org> wrote: > NetworkChangeNotifier serves two basic purposes: > 1. track network state changes so we have a unified signal indicating when > to > flush stale state (e.g. DNS cache, socket pools etc) > 2. track connectivity so we have a unified signal for updating > navigator.onLine > and navigator.connection.type, and initiating server communication (e.g. > Chrome > sync) > In both cases it's tasked with tracking OS signals and producing timely > notifications. There are 50+ places in Chrome that listen to these > signals. > It is not a good place to add synchronous state inspection APIs. > net_util.h > IMHO is, prior art being GetNetworkList(), GetWifiPHYLayerProtocol(), > GetHostName(). > > https://codereview.chromium.org/328793002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sounds good to me, I will look into it based on your suggestions. On Thu, Jul 17, 2014 at 12:02 PM, Steven Bennetts <stevenjb@chromium.org> wrote: > I tend to agree that this seems like an overloaded use of > NetworkChangeNotifer. > > Looking more closely at the overall CL (I've been busy so i only focused > on the Chrome OS specific network code before, I'm not really a maintainer > of NCN), it seems like what you really want to do is create an OS specific > helper class for NetworkMetricsProvider and make it a > NetworkChangeNotifer::NetworkChangeObserver. Then it can retrieve the AP > data whenever the network changes and NMP can retrieve that data from it. > i.e. move the code you put in NCNChromeos to something like > NetworkMetricsProviderAccessPointChromeos. (Alternately, since > NMPAPChromeos will be chromeos specific, it could observe > NetworkStateHandler directly for DefaultNetworkChanged events and not > involve NCN at all). > > Does that make sense? > > > > > On Thu, Jul 17, 2014 at 11:34 AM, <pauljensen@chromium.org> wrote: > >> NetworkChangeNotifier serves two basic purposes: >> 1. track network state changes so we have a unified signal indicating >> when to >> flush stale state (e.g. DNS cache, socket pools etc) >> 2. track connectivity so we have a unified signal for updating >> navigator.onLine >> and navigator.connection.type, and initiating server communication (e.g. >> Chrome >> sync) >> In both cases it's tasked with tracking OS signals and producing timely >> notifications. There are 50+ places in Chrome that listen to these >> signals. >> It is not a good place to add synchronous state inspection APIs. >> net_util.h >> IMHO is, prior art being GetNetworkList(), GetWifiPHYLayerProtocol(), >> GetHostName(). >> >> https://codereview.chromium.org/328793002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
@stevenjb and @pauljensen, I added a new interface in net/base for providing wifi access point information. Please review it when you got a chance. Thanks, Peter,
https://codereview.chromium.org/328793002/diff/280001/chromeos/network/wifi_a... File chromeos/network/wifi_access_point_info_provider_chromeos.h (right): https://codereview.chromium.org/328793002/diff/280001/chromeos/network/wifi_a... chromeos/network/wifi_access_point_info_provider_chromeos.h:16: // WifiAccessPointInfoProviderChromeos provide the connected wifi s/provide/provides/ https://codereview.chromium.org/328793002/diff/280001/chromeos/network/wifi_a... chromeos/network/wifi_access_point_info_provider_chromeos.h:39: net::WifiAccessPointInfo wifi_access_point_info_; nit: WS https://codereview.chromium.org/328793002/diff/280001/net/base/wifi_access_po... File net/base/wifi_access_point_info_provider.h (right): https://codereview.chromium.org/328793002/diff/280001/net/base/wifi_access_po... net/base/wifi_access_point_info_provider.h:17: virtual ~WifiAccessPointInfoProvider() {} nit: WS https://codereview.chromium.org/328793002/diff/280001/net/base/wifi_access_po... File net/base/wifi_access_point_info_provider_stub.cc (right): https://codereview.chromium.org/328793002/diff/280001/net/base/wifi_access_po... net/base/wifi_access_point_info_provider_stub.cc:18: OVERRIDE { One line https://codereview.chromium.org/328793002/diff/280001/net/base/wifi_access_po... net/base/wifi_access_point_info_provider_stub.cc:28: } So, this mechanism of using a static Create method shouldn't be used across libraries (net/ and chromeos/). When I recommended it I didn't think about hte fact that net/ can't depend on chromeos/ so the cros impl needs to be in chromeos/. Instead, just have the calling code construct either the Chromeos or Stub version using #idfefs. Also, rather than #ifdef out the entire file just include the code on all platforms. (Normally you would just #ifdef out the Create() method, but as mentioned we shouldn't use that here).
https://codereview.chromium.org/328793002/diff/280001/chromeos/network/wifi_a... File chromeos/network/wifi_access_point_info_provider_chromeos.h (right): https://codereview.chromium.org/328793002/diff/280001/chromeos/network/wifi_a... chromeos/network/wifi_access_point_info_provider_chromeos.h:16: // WifiAccessPointInfoProviderChromeos provide the connected wifi On 2014/07/23 20:03:36, stevenjb wrote: > s/provide/provides/ Done. https://codereview.chromium.org/328793002/diff/280001/chromeos/network/wifi_a... chromeos/network/wifi_access_point_info_provider_chromeos.h:39: net::WifiAccessPointInfo wifi_access_point_info_; On 2014/07/23 20:03:36, stevenjb wrote: > nit: WS Done. https://codereview.chromium.org/328793002/diff/280001/net/base/wifi_access_po... File net/base/wifi_access_point_info_provider.h (right): https://codereview.chromium.org/328793002/diff/280001/net/base/wifi_access_po... net/base/wifi_access_point_info_provider.h:17: virtual ~WifiAccessPointInfoProvider() {} On 2014/07/23 20:03:36, stevenjb wrote: > nit: WS Done. https://codereview.chromium.org/328793002/diff/280001/net/base/wifi_access_po... File net/base/wifi_access_point_info_provider_stub.cc (right): https://codereview.chromium.org/328793002/diff/280001/net/base/wifi_access_po... net/base/wifi_access_point_info_provider_stub.cc:18: OVERRIDE { On 2014/07/23 20:03:36, stevenjb wrote: > One line Done. https://codereview.chromium.org/328793002/diff/280001/net/base/wifi_access_po... net/base/wifi_access_point_info_provider_stub.cc:28: } On 2014/07/23 20:03:36, stevenjb wrote: > So, this mechanism of using a static Create method shouldn't be used across > libraries (net/ and chromeos/). When I recommended it I didn't think about hte > fact that net/ can't depend on chromeos/ so the cros impl needs to be in > chromeos/. > > Instead, just have the calling code construct either the Chromeos or Stub > version using #idfefs. > > Also, rather than #ifdef out the entire file just include the code on all > platforms. (Normally you would just #ifdef out the Create() method, but as > mentioned we shouldn't use that here). Done.
https://codereview.chromium.org/328793002/diff/300001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.h (right): https://codereview.chromium.org/328793002/diff/300001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.h:20: #endif // OS_CHROMEOS Put this in the .cc file, not the header. Just include wifi_access_point_info_provider.h here https://codereview.chromium.org/328793002/diff/300001/chromeos/network/wifi_a... File chromeos/network/wifi_access_point_info_provider_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/300001/chromeos/network/wifi_a... chromeos/network/wifi_access_point_info_provider_chromeos.cc:48: if (default_network->type() == shill::kTypeWifi) { nit: early exit instead (could combine with !default_network too)
https://codereview.chromium.org/328793002/diff/300001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.h (right): https://codereview.chromium.org/328793002/diff/300001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.h:20: #endif // OS_CHROMEOS On 2014/07/23 22:21:15, stevenjb wrote: > Put this in the .cc file, not the header. Just include > wifi_access_point_info_provider.h here Done. https://codereview.chromium.org/328793002/diff/300001/chromeos/network/wifi_a... File chromeos/network/wifi_access_point_info_provider_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/300001/chromeos/network/wifi_a... chromeos/network/wifi_access_point_info_provider_chromeos.cc:48: if (default_network->type() == shill::kTypeWifi) { On 2014/07/23 22:21:16, stevenjb wrote: > nit: early exit instead (could combine with !default_network too) Done.
chromeos/ lgtm
@pauljensen, can you review the net/base changes? Thanks, Peter,
I don't think there is a need for a abstract virtual interface when there will only be one static implementation. As I recommended in #51, I think implementing similar to wifi_phy_layer_protocol makes more sense. See GetWifiPHYLayerProtocol().
On 2014/07/28 16:32:28, pauljensen wrote: > I don't think there is a need for a abstract virtual interface when there will > only be one static implementation. > As I recommended in #51, I think implementing similar to wifi_phy_layer_protocol > makes more sense. See GetWifiPHYLayerProtocol(). @pauljensen, the problem is that it depends on chromeos library (NetworkStateHandler) for retrieving wifi access point information. If I implement it in net/base similar to GetWifiPHYLayerProtocol, how I would I retrieve the wifi access point information from chromeos? Since we don't want net/base to depends on chromeos library. Also GetWifiPHYLayerProtocol is currently implemented for windows OS only. @stevenjb, since you're more familiar with the chromeos specific implementation in chrome, any comments regarding pauljensen's suggestion? Thanks, Peter,
On 2014/07/28 16:32:28, pauljensen wrote: > I don't think there is a need for a abstract virtual interface when there will > only be one static implementation. > As I recommended in #51, I think implementing similar to wifi_phy_layer_protocol > makes more sense. See GetWifiPHYLaconcernyerProtocol(). I'm not sure what the specific suggestion here is at this point, since this was removed from NCN as suggested in comment #51. My understanding is that WifiAccessPointInfoProvider provides an interface that can be implemented in an OS specific way. As zqlu1@ mentions, src/net can not depend on src/chromeos. That said, looking at the implementation code, it's not clear to me why this is in net/base at all when the only code that currently accesses it is chrome/browser/metrics/network_metrics_provider.cc, unless there are plans to use this interface elsewhere in the future.
The reason I put WifiAccessPointInfoProvider in net/base is that I thought net/base is a common place for network specific API interfaces, other OS can implement it in the future as needed. If this doesn't have to be the case, I can put that interface in chrome/browser/metrics/ instead. Thanks, Peter, On Mon, Jul 28, 2014 at 3:35 PM, <stevenjb@chromium.org> wrote: > On 2014/07/28 16:32:28, pauljensen wrote: > >> I don't think there is a need for a abstract virtual interface when there >> will >> only be one static implementation. >> As I recommended in #51, I think implementing similar to >> > wifi_phy_layer_protocol > >> makes more sense. See GetWifiPHYLaconcernyerProtocol(). >> > > I'm not sure what the specific suggestion here is at this point, since > this was > removed from NCN as suggested in comment #51. > > My understanding is that WifiAccessPointInfoProvider provides an interface > that > can be implemented in an OS specific way. As zqlu1@ mentions, src/net can > not > depend on src/chromeos. > > That said, looking at the implementation code, it's not clear to me why > this is > in net/base at all when the only code that currently accesses it is > chrome/browser/metrics/network_metrics_provider.cc, unless there are > plans to > use this interface elsewhere in the future. > > > https://codereview.chromium.org/328793002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
@stevenjb, I've moved the access point info provider interface to chrome/browser/metrics/. Please take another look.
Mostly looks good, just one more change, thanks. https://codereview.chromium.org/328793002/diff/360001/chrome/browser/metrics/... File chrome/browser/metrics/wifi_access_point_info_provider.h (right): https://codereview.chromium.org/328793002/diff/360001/chrome/browser/metrics/... chrome/browser/metrics/wifi_access_point_info_provider.h:11: enum WifiSecurityMode { We should either make these part of WifiAccessPointInfoProvider, or put them in a namespace, e.g. 'wifi' or 'metrics'.
@stevenjb, PTAL.
lgtm
The CQ bit was checked by zqiu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/328793002/380001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/34897) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/39918) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/34952) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by zqiu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/328793002/380001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35382) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/40413) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35391) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
The CQ bit was checked by zqiu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/328793002/390001
The CQ bit was unchecked by zqiu@chromium.org
The CQ bit was checked by zqiu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/328793002/410001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by zqiu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/328793002/410001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zqiu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/328793002/410001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
@stevenjb, Wifi access point provider creation is deferred to NetworkMetricsProvider::ProvideSystemProfileMetrics instead of NetworkMetricsProvider constructor to fix the browser_tests failures. Also filter out the APs with BSSID that have local bit set for privacy concern. PTAL.
https://codereview.chromium.org/328793002/diff/430001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/430001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:54: new WifiAccessPointInfoProviderChromeos()); wifi_access_point_info_provider_.reset(new WifiAccessPointInfoProviderChromeos()); (Sorry, I should have caught that in the previous round) https://codereview.chromium.org/328793002/diff/430001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:58: new WifiAccessPointInfoProvider()); ditto https://codereview.chromium.org/328793002/diff/430001/chrome/browser/metrics/... File chrome/browser/metrics/wifi_access_point_info_provider_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/430001/chrome/browser/metrics/... chrome/browser/metrics/wifi_access_point_info_provider_chromeos.cc:67: properties.GetStringWithoutPathExpansion(shill::kWifiBSsid, &bssid); It's possible that the kWifiBSsid property may not exist, in which case we should probably just return (i.e. if this returns false).
@stevenjb, PTAL https://codereview.chromium.org/328793002/diff/430001/chrome/browser/metrics/... File chrome/browser/metrics/network_metrics_provider.cc (right): https://codereview.chromium.org/328793002/diff/430001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:54: new WifiAccessPointInfoProviderChromeos()); On 2014/08/18 17:28:14, stevenjb wrote: > wifi_access_point_info_provider_.reset(new > WifiAccessPointInfoProviderChromeos()); > (Sorry, I should have caught that in the previous round) Done. https://codereview.chromium.org/328793002/diff/430001/chrome/browser/metrics/... chrome/browser/metrics/network_metrics_provider.cc:58: new WifiAccessPointInfoProvider()); On 2014/08/18 17:28:14, stevenjb wrote: > ditto Done. https://codereview.chromium.org/328793002/diff/430001/chrome/browser/metrics/... File chrome/browser/metrics/wifi_access_point_info_provider_chromeos.cc (right): https://codereview.chromium.org/328793002/diff/430001/chrome/browser/metrics/... chrome/browser/metrics/wifi_access_point_info_provider_chromeos.cc:67: properties.GetStringWithoutPathExpansion(shill::kWifiBSsid, &bssid); On 2014/08/18 17:28:14, stevenjb wrote: > It's possible that the kWifiBSsid property may not exist, in which case we > should probably just return (i.e. if this returns false). > Done.
lgtm
The CQ bit was checked by zqiu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zqiu@chromium.org/328793002/450001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Message was sent while issue was closed.
Committed patchset #24 (450001) as 290350
Message was sent while issue was closed.
Patchset #25 (id:470001) has been deleted |
