[net] Add WifiPhyMode to SystemProfile

chrome/browser/metrics/metrics_log.cc

Index: chrome/browser/metrics/metrics_log.cc
diff --git a/chrome/browser/metrics/metrics_log.cc b/chrome/browser/metrics/metrics_log.cc
index 03fa5cf8868f567c0cdf58132bbc5948c1a68af6..804f7dec630acb95a048a9718d88814b53812422 100644
--- a/chrome/browser/metrics/metrics_log.cc
+++ b/chrome/browser/metrics/metrics_log.cc
@@ -12,12 +12,14 @@
 #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"
@@ -45,6 +47,7 @@
 #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"
@@ -311,7 +314,10 @@ void WriteScreenDPIInformationProto(SystemProfileProto::Hardware* hardware) {
 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();
   }
@@ -322,6 +328,8 @@ class MetricsLog::NetworkObserver
   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:
@@ -334,6 +342,9 @@ class MetricsLog::NetworkObserver
       connection_type_is_ambiguous_ = true;
     }
     connection_type_ = type;
+
+    weak_ptr_factory_.InvalidateWeakPtrs();
+    new WifiPhyModeJob(weak_ptr_factory_.GetWeakPtr());
   }
 
   bool connection_type_is_ambiguous() const {
@@ -360,10 +371,75 @@ class MetricsLog::NetworkObserver
     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);
 };
 
@@ -878,6 +954,9 @@ void MetricsLog::RecordEnvironmentProto(
   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();