[net] Add WifiPhyMode to SystemProfile

chrome/browser/metrics/metrics_log.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/metrics/metrics_log.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/perftimer.h"
 #include "base/profiler/alternate_timer.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/sys_info.h"
 #include "base/third_party/nspr/prtime.h"
+#include "base/threading/worker_pool.h"

[Ilya Sherman, 2013/02/11 06:26:13]

You should pretty much never use the WorkerPool class.  Instead, use
SequencedWorkerPool via BrowserThread::GetBlockingPool()

[szym, 2013/02/11 08:30:53]

Habits from net/. BrowserThread is not available there, and most pieces are not
eligible to allocate their own pools.

 #include "base/time.h"
 #include "base/tracked_objects.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_input.h"
 #include "chrome/browser/autocomplete/autocomplete_log.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/autocomplete/autocomplete_provider.h"
 #include "chrome/browser/autocomplete/autocomplete_result.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/google/google_util.h"
 #include "chrome/browser/plugins/plugin_prefs.h"
 #include "chrome/browser/prefs/pref_registry_simple.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/logging_chrome.h"
 #include "chrome/common/metrics/proto/omnibox_event.pb.h"
 #include "chrome/common/metrics/proto/profiler_event.pb.h"
 #include "chrome/common/metrics/proto/system_profile.pb.h"
 #include "chrome/common/metrics/variations/variations_util.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/installer/util/google_update_settings.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/gpu_data_manager.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/gpu_info.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/net_util.h"
 #include "net/base/network_change_notifier.h"
 #include "ui/gfx/screen.h"
 #include "webkit/plugins/webplugininfo.h"
 
 #if defined(OS_ANDROID)
 #include "base/android/build_info.h"
 #endif
 
 #define OPEN_ELEMENT_FOR_SCOPE(name) ScopedElement scoped_element(this, name)
 
@@ skipping 244 matching lines @@
     }
     ReleaseDC(GetDesktopWindow(), desktop_dc);
   }
 }
 
 #endif  // defined(OS_WIN)
 
 }  // namespace
 
 class MetricsLog::NetworkObserver
     : public net::NetworkChangeNotifier::ConnectionTypeObserver {

[Ilya Sherman, 2013/02/11 06:26:13]

Can you move this class out to its own header file?  It's getting a little too
complex to be inlined in this implementation file, IMO.

[szym, 2013/02/11 08:30:53]

Done.

  public:
-  NetworkObserver() : connection_type_is_ambiguous_(false) {
+  NetworkObserver()
+      : weak_ptr_factory_(this),

[Ilya Sherman, 2013/02/11 06:26:13]

nit: ALLOW_THIS_IN_...

[szym, 2013/02/11 08:30:53]

Done.

+        connection_type_is_ambiguous_(false),
+        wifi_phy_mode_is_ambiguous_(false)      {
     net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
     Reset();
   }
   virtual ~NetworkObserver() {
     net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
   }
 
   void Reset() {
     connection_type_is_ambiguous_ = false;
     connection_type_ = net::NetworkChangeNotifier::GetConnectionType();
+    wifi_phy_mode_is_ambiguous_ = false;
+    wifi_phy_mode_ = net::WIFI_PHY_MODE_UNKNOWN;
   }
 
   // ConnectionTypeObserver:
   virtual void OnConnectionTypeChanged(
       net::NetworkChangeNotifier::ConnectionType type) OVERRIDE {
     if (type == net::NetworkChangeNotifier::CONNECTION_NONE)
       return;
     if (type != connection_type_ &&
         connection_type_ != net::NetworkChangeNotifier::CONNECTION_NONE) {
       connection_type_is_ambiguous_ = true;
     }
     connection_type_ = type;
+
+    weak_ptr_factory_.InvalidateWeakPtrs();
+    new WifiPhyModeJob(weak_ptr_factory_.GetWeakPtr());
   }
 
   bool connection_type_is_ambiguous() const {
     return connection_type_is_ambiguous_;
   }
 
   SystemProfileProto::Network::ConnectionType connection_type() const {
     switch (connection_type_) {
       case net::NetworkChangeNotifier::CONNECTION_NONE:
       case net::NetworkChangeNotifier::CONNECTION_UNKNOWN:
         return SystemProfileProto::Network::CONNECTION_UNKNOWN;
       case net::NetworkChangeNotifier::CONNECTION_ETHERNET:
         return SystemProfileProto::Network::CONNECTION_ETHERNET;
       case net::NetworkChangeNotifier::CONNECTION_WIFI:
         return SystemProfileProto::Network::CONNECTION_WIFI;
       case net::NetworkChangeNotifier::CONNECTION_2G:
         return SystemProfileProto::Network::CONNECTION_2G;
       case net::NetworkChangeNotifier::CONNECTION_3G:
         return SystemProfileProto::Network::CONNECTION_3G;
       case net::NetworkChangeNotifier::CONNECTION_4G:
         return SystemProfileProto::Network::CONNECTION_4G;
     }
     NOTREACHED();
     return SystemProfileProto::Network::CONNECTION_UNKNOWN;
   }
 
+  bool wifi_phy_mode_is_ambiguous() const {
+    return wifi_phy_mode_is_ambiguous_;
+  }
+
+  SystemProfileProto::Network::WifiPhyMode wifi_phy_mode() const {
+    switch (wifi_phy_mode_) {
+      case net::WIFI_PHY_MODE_NONE:
+        return SystemProfileProto::Network::WIFI_PHY_MODE_NONE;
+      case net::WIFI_PHY_MODE_ANCIENT:
+        return SystemProfileProto::Network::WIFI_PHY_MODE_ANCIENT;
+      case net::WIFI_PHY_MODE_A:
+        return SystemProfileProto::Network::WIFI_PHY_MODE_A;
+      case net::WIFI_PHY_MODE_B:
+        return SystemProfileProto::Network::WIFI_PHY_MODE_B;
+      case net::WIFI_PHY_MODE_G:
+        return SystemProfileProto::Network::WIFI_PHY_MODE_G;
+      case net::WIFI_PHY_MODE_N:
+        return SystemProfileProto::Network::WIFI_PHY_MODE_N;
+      case net::WIFI_PHY_MODE_UNKNOWN:
+        return SystemProfileProto::Network::WIFI_PHY_MODE_UNKNOWN;
+    }
+    NOTREACHED();
+    return SystemProfileProto::Network::WIFI_PHY_MODE_UNKNOWN;
+  }
+
  private:
+  class WifiPhyModeJob {

[Ilya Sherman, 2013/02/11 06:26:13]

Why are you creating a class for this, rather than just implementing this as a
pair of methods on the NetworkObserver class?

[szym, 2013/02/11 08:30:53]

A separate class (a DataBuffer) to hold the result is needed for
PostTaskAndReply.

This pattern:
PostTaskAndReply(Bind(&Work, Unretained(this)),
                 Bind(&Complete, Owned(this)));
was suggested to me by eroman@ in
https://codereview.chromium.org/11359007/diff/14/net/base/host_resolver_impl....

This allows us to avoid the need for reference counting in the DataBuffer class.

+   public:
+    explicit WifiPhyModeJob(const base::WeakPtr<NetworkObserver>& observer)
+        : observer_(observer) {
+      base::WorkerPool::PostTaskAndReply(
+          FROM_HERE,
+          base::Bind(&WifiPhyModeJob::DoProbe, base::Unretained(this)),
+          base::Bind(&WifiPhyModeJob::OnProbeComplete, base::Owned(this)),
+          true /* isSlow */);
+    }
+    virtual ~WifiPhyModeJob() {}
+
+   private:
+    // Runs on worker thread.
+    void DoProbe() {
+      result_ = net::GetWifiPhyMode();

[Ilya Sherman, 2013/02/11 06:26:13]

This is not thread-safe.  Why are you storing the result into a member variable,
rather than just passing it back as part of PostTaskAndReply?

[szym, 2013/02/11 08:30:53]

This is as thread-safe as the PostTaskAndReply example in:
http://src.chromium.org/viewvc/chrome/trunk/src/base/task_runner.h

PostTaskAndReply guarantees that the two closures are not called concurrently so
there is no need for locking.

The only difference is that the result is passed as part of |this| rather than
separate ref-counted class.

That said, I realized there's no need for a special class at all. PTAL.

+    }
+
+    void OnProbeComplete() {
+      if (observer_)
+        observer_->OnWifiModeChanged(result_);
+    }
+
+    base::WeakPtr<NetworkObserver> observer_;
+    net::WifiPhyMode result_;
+
+    DISALLOW_COPY_AND_ASSIGN(WifiPhyModeJob);
+  };
+
+  void OnWifiModeChanged(net::WifiPhyMode mode) {
+    if (mode != wifi_phy_mode_)
+      wifi_phy_mode_is_ambiguous_ = true;
+    wifi_phy_mode_ = mode;
+  }
+
+  base::WeakPtrFactory<NetworkObserver> weak_ptr_factory_;
+
   bool connection_type_is_ambiguous_;
   net::NetworkChangeNotifier::ConnectionType connection_type_;
 
+  bool wifi_phy_mode_is_ambiguous_;
+  net::WifiPhyMode wifi_phy_mode_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkObserver);
 };
 
 GoogleUpdateMetrics::GoogleUpdateMetrics() : is_system_install(false) {}
 
 GoogleUpdateMetrics::~GoogleUpdateMetrics() {}
 
 static base::LazyInstance<std::string>::Leaky
   g_version_extension = LAZY_INSTANCE_INITIALIZER;
 
@@ skipping 494 matching lines @@
   hardware->set_cpu_architecture(base::SysInfo::OperatingSystemArchitecture());
   hardware->set_system_ram_mb(base::SysInfo::AmountOfPhysicalMemoryMB());
 #if defined(OS_WIN)
   hardware->set_dll_base(reinterpret_cast<uint64>(&__ImageBase));
 #endif
 
   SystemProfileProto::Network* network = system_profile->mutable_network();
   network->set_connection_type_is_ambiguous(
       network_observer_->connection_type_is_ambiguous());
   network->set_connection_type(network_observer_->connection_type());
+  network->set_wifi_phy_mode_is_ambiguous(
+      network_observer_->wifi_phy_mode_is_ambiguous());
+  network->set_wifi_phy_mode(network_observer_->wifi_phy_mode());
   network_observer_->Reset();
 
   SystemProfileProto::OS* os = system_profile->mutable_os();
   std::string os_name = base::SysInfo::OperatingSystemName();
 #if defined(OS_WIN)
   // TODO(mad): This only checks whether the main process is a Metro process at
   // upload time; not whether the collected metrics were all gathered from
   // Metro.  This is ok as an approximation for now, since users will rarely be
   // switching from Metro to Desktop mode; but we should re-evaluate whether we
   // can distinguish metrics more cleanly in the future: http://crbug.com/140568
@@ skipping 253 matching lines @@
     ProductDataToProto(google_update_metrics.google_update_data,
                        google_update->mutable_google_update_status());
   }
 
   if (!google_update_metrics.product_data.version.empty()) {
     ProductDataToProto(google_update_metrics.product_data,
                        google_update->mutable_client_status());
   }
 #endif  // defined(GOOGLE_CHROME_BUILD) && defined(OS_WIN)
 }