[net] Add WifiPhyMode to SystemProfile

Add WifiPHYLayerProtocol to SystemProfile.

WifiPHYLayerProtocol characterizes the physical layer (PHY) protocol
of the current association with an 802.11 access point. It uses
the common nomenclature of referring to the IEEE 802.11 standard
amendment which introduced the protocol, i.e. a/b/g/n.

TBR=sky
BUG=174414
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182231

[szym, 2013-02-07 22:30:25]

rvargas: please review net/
isherman: please review */metrics/

[szym, 2013-02-08 00:12:50]

Per request of the bug filer (tburkard), it'd be great if this made it into M26.

Branch point is on Monday, so thanks in advance for help on this.

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

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:22: #include
"base/threading/worker_pool.h"
You should pretty much never use the WorkerPool class.  Instead, use
SequencedWorkerPool via BrowserThread::GetBlockingPool()

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:315: : public
net::NetworkChangeNotifier::ConnectionTypeObserver {
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.

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:318: : weak_ptr_factory_(this),
nit: ALLOW_THIS_IN_...

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:400: class WifiPhyModeJob {
Why are you creating a class for this, rather than just implementing this as a
pair of methods on the NetworkObserver class?

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:415: result_ = net::GetWifiPhyMode();
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?

https://codereview.chromium.org/12082090/diff/11003/chrome/common/metrics/pro...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/12082090/diff/11003/chrome/common/metrics/pro...
chrome/common/metrics/proto/system_profile.proto:171: optional bool
wifi_phy_mode_is_ambiguous = 3;
nit: Docs.

https://codereview.chromium.org/12082090/diff/11003/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/12082090/diff/11003/net/base/net_util.h#newco...
net/base/net_util.h:527: };
nit: Docs?

https://codereview.chromium.org/12082090/diff/11003/net/base/net_util.h#newco...
net/base/net_util.h:529: // Characterize the PHY mode of the currently
associated access point.
nit: I have no idea what "PHY" is.  Is there a description for this that doesn't
use this acronym?

https://codereview.chromium.org/12082090/diff/11003/net/base/net_util.h#newco...
net/base/net_util.h:529: // Characterize the PHY mode of the currently
associated access point.
nit: Mention that this is only implemented on Windows?

[szym, 2013-02-11 08:30:52]

sleevi: Sorry to bother, but would you mind taking a look at changes to net/?

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:22: #include
"base/threading/worker_pool.h"
On 2013/02/11 06:26:13, Ilya Sherman wrote:
> You should pretty much never use the WorkerPool class.  Instead, use
> SequencedWorkerPool via BrowserThread::GetBlockingPool()

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

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:315: : public
net::NetworkChangeNotifier::ConnectionTypeObserver {
On 2013/02/11 06:26:13, Ilya Sherman wrote:
> 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.

Done.

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:318: : weak_ptr_factory_(this),
On 2013/02/11 06:26:13, Ilya Sherman wrote:
> nit: ALLOW_THIS_IN_...

Done.

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:400: class WifiPhyModeJob {
On 2013/02/11 06:26:13, Ilya Sherman wrote:
> Why are you creating a class for this, rather than just implementing this as a
> pair of methods on the NetworkObserver class?

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.

https://codereview.chromium.org/12082090/diff/11003/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:415: result_ = net::GetWifiPhyMode();
On 2013/02/11 06:26:13, Ilya Sherman wrote:
> 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?

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.

https://codereview.chromium.org/12082090/diff/11003/chrome/common/metrics/pro...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/12082090/diff/11003/chrome/common/metrics/pro...
chrome/common/metrics/proto/system_profile.proto:171: optional bool
wifi_phy_mode_is_ambiguous = 3;
On 2013/02/11 06:26:13, Ilya Sherman wrote:
> nit: Docs.

Done.

https://codereview.chromium.org/12082090/diff/11003/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/12082090/diff/11003/net/base/net_util.h#newco...
net/base/net_util.h:527: };
On 2013/02/11 06:26:13, Ilya Sherman wrote:
> nit: Docs?

Done.

https://codereview.chromium.org/12082090/diff/11003/net/base/net_util.h#newco...
net/base/net_util.h:529: // Characterize the PHY mode of the currently
associated access point.
On 2013/02/11 06:26:13, Ilya Sherman wrote:
> nit: I have no idea what "PHY" is.  Is there a description for this that
doesn't
> use this acronym?

Done.

PHY is not an acronym. It stands for "physical layer".

https://codereview.chromium.org/12082090/diff/11003/net/base/net_util.h#newco...
net/base/net_util.h:529: // Characterize the PHY mode of the currently
associated access point.
On 2013/02/11 06:26:13, Ilya Sherman wrote:
> nit: Mention that this is only implemented on Windows?

Done.

[Ryan Sleevi, 2013-02-11 18:32:27]

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc
File net/base/net_util_win.cc (right):

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:146: FilePath(FILE_PATH_LITERAL("wlanapi.dll")));
See rvargas' previous comments re: ScopedNativeLibrary being "wrong", and it
being ok to just open an HMODULE and leak it.

If this is always called on the IO thread, 'static HMODULE ... =
LoadLibrary(...)'. See also his feedback of making sure that the DLL search
paths are explicitly set, to avoid DLL sideloading.

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:167: DWORD result = open_handle_func(2 /* max_client
*/, NULL, &cur_version,
Can you make this a meaningful constant?

const DWORD kMaxVersion = 2;

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:172: base::Bind(base::IgnoreResult(close_handle_func),
client, (VOID*)NULL));
style nit: c-style casts are no good.

Other points: Why are you using a ScopedClosureRunner rather than a scoped_ptr
with a custom deleter?

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:179: base::Bind(free_memory_func, interface_list));
same here

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:184: // Assume one wifi interface.
Why?

Should we be treating the results as a collection, similar to the interface
list?

Is it possible to have multiple wifi interfaces - one or more offline, one or
more online? Is it safe/correct to assume that the first interface will always
be online?

I didn't find an answer to these questions in MSDN. EnumInterfaces only remarks
about "Enabled", but that's not necessarily the same.

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:195: &conn_info_size, (VOID**)&conn_info, &op_code);
style nit: wrong casts

[rvargas (doing something else), 2013-02-11 21:14:43]

Sorry for the delay.

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc
File net/base/net_util_win.cc (right):

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:16: #include "base/scoped_native_library.h"
not good.

Note that I failed to mention the other day that an alternative to manual
binding is delay loading. There is some interest in improving/customizing delay
loading and if that pans out we'll be removing manual loading.

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:165: HANDLE client = NULL;
This should be a GenericScopedHandle with a custom HandleTraits

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:174: WLAN_INTERFACE_INFO_LIST* interface_list = NULL;
scoped_ptr with custom Deleter

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:187: return WIFI_PHY_MODE_NONE;
look at the next interface?

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:189: WLAN_CONNECTION_ATTRIBUTES* conn_info = NULL;
same here

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:195: &conn_info_size, (VOID**)&conn_info, &op_code);
c++ cast (Doesn't this work without a cast?)

[szym, 2013-02-11 22:51:19]

Thank you all for the helpful review.

As I was educating myself about LoadLibraryEx, etc, I discovered another use of
wlanapi (for geolocation):
https://code.google.com/p/chromium/codesearch#chrome/src/content/browser/geol...

It doesn't seem like there's that much to reuse there, but since this will be
only used in metrics/ we could move it chrome/ and try to reuse some of this.

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc
File net/base/net_util_win.cc (right):

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:146: FilePath(FILE_PATH_LITERAL("wlanapi.dll")));
On 2013/02/11 18:32:28, Ryan Sleevi wrote:
> See rvargas' previous comments re: ScopedNativeLibrary being "wrong", and it
> being ok to just open an HMODULE and leak it.
> If this is always called on the IO thread, 'static HMODULE ... =
> LoadLibrary(...)'. See also his feedback of making sure that the DLL search
> paths are explicitly set, to avoid DLL sideloading.

From my experiments, this takes a while, on WindowsXP T61, the median was ~10ms,
with 99% in the seconds. Perhaps this was because of some overhead of
ScopedNativeLibrary, but I don't think this should ever be called from the IO
thread.

This will almost certainly be called from a worker pool.

Wrapped it up as WlanApi and used LazyInstance::Leaky.

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:167: DWORD result = open_handle_func(2 /* max_client
*/, NULL, &cur_version,
On 2013/02/11 18:32:28, Ryan Sleevi wrote:
> Can you make this a meaningful constant?
> 
> const DWORD kMaxVersion = 2;

Done.

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:172: base::Bind(base::IgnoreResult(close_handle_func),
client, (VOID*)NULL));
On 2013/02/11 18:32:28, Ryan Sleevi wrote:
> style nit: c-style casts are no good.
> 
> Other points: Why are you using a ScopedClosureRunner rather than a scoped_ptr
> with a custom deleter?

I didn't think HANDLE can be scoped_ptr'ed.

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:184: // Assume one wifi interface.
On 2013/02/11 18:32:28, Ryan Sleevi wrote:
> Why?
> 
> Should we be treating the results as a collection, similar to the interface
> list?
> 
> Is it possible to have multiple wifi interfaces - one or more offline, one or
> more online? Is it safe/correct to assume that the first interface will always
> be online?

Yes. No. But I'd guesstimate this works for >95% of the devices out there.

> I didn't find an answer to these questions in MSDN. EnumInterfaces only
remarks
> about "Enabled", but that's not necessarily the same.

https://codereview.chromium.org/12082090/diff/15014/net/base/net_util_win.cc#...
net/base/net_util_win.cc:195: &conn_info_size, (VOID**)&conn_info, &op_code);
On 2013/02/11 18:32:28, Ryan Sleevi wrote:
> style nit: wrong casts

Done.

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc
File net/base/net_util_win.cc (right):

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:16: #include "base/scoped_native_library.h"
On 2013/02/11 21:14:44, rvargas wrote:
> not good.
> 
> Note that I failed to mention the other day that an alternative to manual
> binding is delay loading. There is some interest in improving/customizing
delay
> loading and if that pans out we'll be removing manual loading.

Done.

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:165: HANDLE client = NULL;
On 2013/02/11 21:14:44, rvargas wrote:
> This should be a GenericScopedHandle with a custom HandleTraits

Done.

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:174: WLAN_INTERFACE_INFO_LIST* interface_list = NULL;
On 2013/02/11 21:14:44, rvargas wrote:
> scoped_ptr with custom Deleter

Done, although there does not seem to be something as convenient as
ScopedHandle::Receiver.

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:187: return WIFI_PHY_MODE_NONE;
On 2013/02/11 21:14:44, rvargas wrote:
> look at the next interface?

Done.

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:189: WLAN_CONNECTION_ATTRIBUTES* conn_info = NULL;
On 2013/02/11 21:14:44, rvargas wrote:
> same here

Done.

https://codereview.chromium.org/12082090/diff/18009/net/base/net_util_win.cc#...
net/base/net_util_win.cc:195: &conn_info_size, (VOID**)&conn_info, &op_code);
On 2013/02/11 21:14:44, rvargas wrote:
> c++ cast (Doesn't this work without a cast?)

Done.

[szym, 2013-02-12 00:01:21]

I took a 1000 samples of GetWifiPhyMode (run on the IO thread) and found the
following distribution:

Histogram: WifiPHY.Time recorded 1071 samples, average = 8.1 (flags = 0x1)
0     --O                                                                      
(17 = 1.6%)
1     -----------------------------O                                           
(218 = 20.4%) {1.6%}
2     ------------------------------------------------------------------------O
(535 = 50.0%) {21.9%}
3     --------------O                                                          
(107 = 10.0%) {71.9%}
4     ------O                                                                  
(46 = 4.3%) {81.9%}
5     -------O                                                                 
(52 = 4.9%) {86.2%}
6     ---O                                                                     
(26 = 2.4%) {91.0%}
7     ---O                                                                     
(19 = 1.8%) {93.5%}
8     -O                                                                       
(10 = 0.9%) {95.2%}
10    O                                                                        
(7 = 0.7%) {96.2%}
12    O                                                                        
(7 = 0.7%) {96.8%}
14    O                                                                        
(10 = 0.9%) {97.5%}
17    O                                                                        
(1 = 0.1%) {98.4%}
20    O                                                                        
(2 = 0.2%) {98.5%}
24    O                                                                        
(2 = 0.2%) {98.7%}
29    O                                                                        
(4 = 0.4%) {98.9%}
34    O                                                                        
(1 = 0.1%) {99.3%}
40    O                                                                        
(0 = 0.0%) {99.3%}
48    O                                                                        
(1 = 0.1%) {99.3%}
57    O                                                                        
(0 = 0.0%) {99.4%}
68    O                                                                        
(1 = 0.1%) {99.4%}
81    ...
533   O                                                                        
(1 = 0.1%) {99.5%}
633   O                                                                        
(0 = 0.0%) {99.6%}
752   O                                                                        
(1 = 0.1%) {99.6%}
894   O                                                                        
(1 = 0.1%) {99.7%}
1062  O                                                                        
(0 = 0.0%) {99.8%}
1262  O                                                                        
(1 = 0.1%) {99.8%}
1500  O                                                                        
(1 = 0.1%) {99.9%}


This will be called only when a new MetricsLog is opened, so maybe there's no
need to run it on the worker pool.

isherman: up to you.

[rvargas (doing something else), 2013-02-12 02:42:33]

net lgtm

https://codereview.chromium.org/12082090/diff/12011/net/base/host_resolver_im...
File net/base/host_resolver_impl.cc (right):

https://codereview.chromium.org/12082090/diff/12011/net/base/host_resolver_im...
net/base/host_resolver_impl.cc:1985: base::TimeTicks then =
base::TimeTicks::Now();
You don't mean to check this in, right?

https://codereview.chromium.org/12082090/diff/12011/net/base/net_util_win.cc
File net/base/net_util_win.cc (right):

https://codereview.chromium.org/12082090/diff/12011/net/base/net_util_win.cc#...
net/base/net_util_win.cc:181: struct WlanApiHandleTraits {
nit: class

[szym, 2013-02-12 04:53:43]

https://codereview.chromium.org/12082090/diff/12011/net/base/host_resolver_im...
File net/base/host_resolver_impl.cc (right):

https://codereview.chromium.org/12082090/diff/12011/net/base/host_resolver_im...
net/base/host_resolver_impl.cc:1985: base::TimeTicks then =
base::TimeTicks::Now();
On 2013/02/12 02:42:33, rvargas wrote:
> You don't mean to check this in, right?

No, sorry, this is just my shamefully dirty way of testing that it works. (My
test machine is a ganky old WinXP laptop, so it's easy to just copy
mini_installer.)

[szym, 2013-02-12 16:59:41]

isherman: PTAL metrics

[Ilya Sherman, 2013-02-13 01:54:32]

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.h:192: scoped_ptr<MetricsNetworkObserver>
network_observer_;
nit: Docs.  Also, can this be a direct variable, i.e. "MetricsNetworkObserver
network_observer_", rather than a pointer?

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_network_observer.cc (right):

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.cc:36: ProbeWifiPhyMode();
Should you invalidate existing WeakPtrs as part of Reset()?

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.cc:53:
weak_ptr_factory_.InvalidateWeakPtrs();
Are you sure you want to invalidate any existing WeakPtrs?  Wouldn't that
potentially cause you to miss some cases where wifi_phy_mode should be
ambiguous?

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_network_observer.h (right):

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:25: void Reset();
nit: Docs.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:53: }
nit: Please move this method into the implementation file, as it's not
completely trivial.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:55: bool
wifi_phy_mode_is_ambiguous() const {
nit: Please spell out whatever term "phy" represents. 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:78: }
nit: Ditto.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:82: void
OnWifiPhyModeResult(net::WifiPhyMode* mode);
nit: Docs.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:87:
net::NetworkChangeNotifier::ConnectionType connection_type_;
nit: Docs.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:90: net::WifiPhyMode
wifi_phy_mode_;
nit: Docs.

[szym, 2013-02-13 05:09:34]

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.h:192: scoped_ptr<MetricsNetworkObserver>
network_observer_;
On 2013/02/13 01:54:33, Ilya Sherman wrote:
> nit: Docs.  Also, can this be a direct variable, i.e. "MetricsNetworkObserver
> network_observer_", rather than a pointer?

Then we have to include the header file.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_network_observer.cc (right):

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.cc:36: ProbeWifiPhyMode();
On 2013/02/13 01:54:33, Ilya Sherman wrote:
> Should you invalidate existing WeakPtrs as part of Reset()?

On second thought, I realized there's no need to ProbeWifiPhyMode here, because
there should be no change since the last recording. (|connection_type_| should
be updated just in case it's currently CONNECTION_NONE.) I moved this to the
constructor instead.

If I understand correctly, MetricsLog is always closed immediately after
RecordEnvironmentProto, so I don't think Reset is actually really necessary.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.cc:53:
weak_ptr_factory_.InvalidateWeakPtrs();
On 2013/02/13 01:54:33, Ilya Sherman wrote:
> Are you sure you want to invalidate any existing WeakPtrs?  Wouldn't that
> potentially cause you to miss some cases where wifi_phy_mode should be
> ambiguous?

I don't think it matters in practice, since GetWifiPhyMode should finish very
quickly.

I removed it.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_network_observer.h (right):

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:25: void Reset();
On 2013/02/13 01:54:33, Ilya Sherman wrote:
> nit: Docs.

Done.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:53: }
On 2013/02/13 01:54:33, Ilya Sherman wrote:
> nit: Please move this method into the implementation file, as it's not
> completely trivial.

Done.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:55: bool
wifi_phy_mode_is_ambiguous() const {
On 2013/02/13 01:54:33, Ilya Sherman wrote:
> nit: Please spell out whatever term "phy" represents. 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

Are you suggesting I replace "phy" with "physical_layer" in every instance of
this shortcut?

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:78: }
On 2013/02/13 01:54:33, Ilya Sherman wrote:
> nit: Ditto.

Done.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:82: void
OnWifiPhyModeResult(net::WifiPhyMode* mode);
On 2013/02/13 01:54:33, Ilya Sherman wrote:
> nit: Docs.

Done.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:87:
net::NetworkChangeNotifier::ConnectionType connection_type_;
On 2013/02/13 01:54:33, Ilya Sherman wrote:
> nit: Docs.

Done.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:90: net::WifiPhyMode
wifi_phy_mode_;
On 2013/02/13 01:54:33, Ilya Sherman wrote:
> nit: Docs.

Done.

[Ilya Sherman, 2013-02-13 08:05:54]

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.h:192: scoped_ptr<MetricsNetworkObserver>
network_observer_;
On 2013/02/13 05:09:34, szym wrote:
> On 2013/02/13 01:54:33, Ilya Sherman wrote:
> > nit: Docs.  Also, can this be a direct variable, i.e.
"MetricsNetworkObserver
> > network_observer_", rather than a pointer?
> 
> Then we have to include the header file.

Including the header file is fine.

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_network_observer.h (right):

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:55: bool
wifi_phy_mode_is_ambiguous() const {
On 2013/02/13 05:09:34, szym wrote:
> On 2013/02/13 01:54:33, Ilya Sherman wrote:
> > nit: Please spell out whatever term "phy" represents. 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...
> 
> Are you suggesting I replace "phy" with "physical_layer" in every instance of
> this shortcut?

Yes.  Actually, I'd suggest a name like "WifiProtocol", which is both short and
clear.

[szym, 2013-02-13 08:48:08]

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_network_observer.h (right):

https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_network_observer.h:55: bool
wifi_phy_mode_is_ambiguous() const {
On 2013/02/13 08:05:54, Ilya Sherman wrote:
> On 2013/02/13 05:09:34, szym wrote:
> > On 2013/02/13 01:54:33, Ilya Sherman wrote:
> > > nit: Please spell out whatever term "phy" represents. 
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...
> > 
> > Are you suggesting I replace "phy" with "physical_layer" in every instance
of
> > this shortcut?
> 
> Yes.  Actually, I'd suggest a name like "WifiProtocol", which is both short
and
> clear.

Done, although IMO this is ambiguous, since 802.11 defines a number of
protocols: PHY, link (framing), MAC, and security. In this case, WifiProtocol
could mean IBSS (adhoc) vs. station (client), or perhaps WEP vs. WPA2-PSK.

In comparison, Windows' Native Wifi API uses "dot11PhyType" and Mac's CoreWLAN
uses "activePHYMode".

[Ryan Sleevi, 2013-02-13 08:55:19]

I have to agree with Szym, given that that PHY layer shows up heavily in
the standards and academic papers. Yes, it took me a second, but it was
clear and unambiguous. If we are going to expand PHY, then yes, I agree
that PhysicalLayerProtocol is probably the most correct/meaningful to
search on...
On Feb 13, 2013 12:48 AM, <szym@chromium.org> wrote:

>
> https://codereview.chromium.**org/12082090/diff/15020/**
>
chrome/browser/metrics/**metrics_network_observer.h<https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/metrics_network_observer.h>
> File chrome/browser/metrics/**metrics_network_observer.h (right):
>
> https://codereview.chromium.**org/12082090/diff/15020/**
>
chrome/browser/metrics/**metrics_network_observer.h#**newcode55<https://codereview.chromium.org/12082090/diff/15020/chrome/browser/metrics/metrics_network_observer.h#newcode55>
> chrome/browser/metrics/**metrics_network_observer.h:55: bool
> wifi_phy_mode_is_ambiguous() const {
> On 2013/02/13 08:05:54, Ilya Sherman wrote:
>
>> On 2013/02/13 05:09:34, szym wrote:
>> > On 2013/02/13 01:54:33, Ilya Sherman wrote:
>> > > nit: Please spell out whatever term "phy" represents.
>> > >
>> >
>>
>
> http://google-styleguide.**googlecode.com/svn/trunk/**
>
cppguide.xml#General_Naming_**Rules<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming_Rules>
>
>> >
>> > Are you suggesting I replace "phy" with "physical_layer" in every
>>
> instance of
>
>> > this shortcut?
>>
>
>  Yes.  Actually, I'd suggest a name like "WifiProtocol", which is both
>>
> short and
>
>> clear.
>>
>
> Done, although IMO this is ambiguous, since 802.11 defines a number of
> protocols: PHY, link (framing), MAC, and security. In this case,
> WifiProtocol could mean IBSS (adhoc) vs. station (client), or perhaps
> WEP vs. WPA2-PSK.
>
> In comparison, Windows' Native Wifi API uses "dot11PhyType" and Mac's
> CoreWLAN uses "activePHYMode".
>
>
https://codereview.chromium.**org/12082090/<https://codereview.chromium.org/1...
>

[Ilya Sherman, 2013-02-13 09:16:51]

LGTM, thanks.

On 2013/02/13 08:55:19, Ryan Sleevi wrote:
> I have to agree with Szym, given that that PHY layer shows up heavily in
> the standards and academic papers. Yes, it took me a second, but it was
> clear and unambiguous. If we are going to expand PHY, then yes, I agree
> that PhysicalLayerProtocol is probably the most correct/meaningful to
> search on...

Ok, if "protocol" on its own is ambiguous, then my bad for suggesting it.  I'd
still prefer "PhysicalLayerProtocol" to "PHY", since I can guess at what
"PhysicalLayerProtocol" refers to, whereas "PHY" is a completely opaque bit of
jargon.  But, if you feel that "PhysicalLayerProtocol" is really excessively
long, and that "PHY" is a term that anyone interested in the split between
802.11a/b/g/n/etc would be familiar with, I'm willing to be outvoted on this
point.

[szym, 2013-02-13 15:24:12]

Many thanks for the review.

I'm going with WIFI_PHY_LAYER_PROTOCOL to allow lines to still be reasonably
wrapped.

[szym, 2013-02-13 15:27:07]

sky: approve changes to chrome_browser.gypi (TBR)