Add NetworkDeviceHandler

Add NetworkDeviceHandler

This will track Shill Device info, including Wifi access point data.

BUG=167987
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175673

[Greg Spencer (Chromium), 2013-01-04 23:57:26]

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
File chromeos/network/network_device_handler.cc (right):

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler.cc:137: // wifi devices).
wifi --> wifi or wimax

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
File chromeos/network/network_device_handler.h (right):

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler.h:40: class Observer {
This class should be in a separate header.  There was a thread about this in
chromium-dev and in a bug, where it was decided that because it becomes
ambiguous at times when it's a nested class, and because then observers don't
need to include the entire header that it's better to separate observers into
separate headers.

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler.h:44: // Called when |devices_| is
updated, passes |devices_| as a convenience.
Shouldn't one of those be "|devices|" without the underscore?

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
File chromeos/network/network_device_handler_unittest.cc (right):

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler_unittest.cc:50: // Get the test
interface for manager / device / service and clear the
I don't see you clearing the default properties.  Do you mean setup the
defaults?

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler_unittest.cc:110: //
TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerDevicePropertyChanged) {
Don't check in commented out code.

[stevenjb, 2013-01-07 15:56:22]

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
File chromeos/network/network_device_handler.cc (right):

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler.cc:137: // wifi devices).
On 2013/01/04 23:57:26, Greg Spencer (Chromium) wrote:
> wifi --> wifi or wimax

Done.

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
File chromeos/network/network_device_handler.h (right):

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler.h:40: class Observer {
On 2013/01/04 23:57:26, Greg Spencer (Chromium) wrote:
> This class should be in a separate header.  There was a thread about this in
> chromium-dev and in a bug, where it was decided that because it becomes
> ambiguous at times when it's a nested class, and because then observers don't
> need to include the entire header that it's better to separate observers into
> separate headers.

I don't recall the specifics of that discussion, but we still use this pattern
in Ash. I think it still makes sense when the Observer class has dependencies on
the parent class (here we depend on DeviceMap which depends on Device, so we
would have to put Device in its own header as well to remove the dependency).

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler.h:44: // Called when |devices_| is
updated, passes |devices_| as a convenience.
On 2013/01/04 23:57:26, Greg Spencer (Chromium) wrote:
> Shouldn't one of those be "|devices|" without the underscore?

Done.

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
File chromeos/network/network_device_handler_unittest.cc (right):

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler_unittest.cc:50: // Get the test
interface for manager / device / service and clear the
On 2013/01/04 23:57:26, Greg Spencer (Chromium) wrote:
> I don't see you clearing the default properties.  Do you mean setup the
> defaults?
Comment was copy/pasted from another test; fixed.

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler_unittest.cc:110: //
TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerDevicePropertyChanged) {
On 2013/01/04 23:57:26, Greg Spencer (Chromium) wrote:
> Don't check in commented out code.
Oops, juggling too many CLs. Implemented this test for NetworkDeviceHandler.

[stevenjb, 2013-01-08 20:58:38]

Ping

[Greg Spencer (Chromium), 2013-01-08 21:03:07]

LGTM

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
File chromeos/network/network_device_handler.h (right):

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler.h:40: class Observer {
On 2013/01/07 15:56:22, stevenjb (chromium) wrote:
> On 2013/01/04 23:57:26, Greg Spencer (Chromium) wrote:
> > This class should be in a separate header.  There was a thread about this in
> > chromium-dev and in a bug, where it was decided that because it becomes
> > ambiguous at times when it's a nested class, and because then observers
don't
> > need to include the entire header that it's better to separate observers
into
> > separate headers.
> 
> I don't recall the specifics of that discussion, but we still use this pattern
> in Ash. I think it still makes sense when the Observer class has dependencies
on
> the parent class (here we depend on DeviceMap which depends on Device, so we
> would have to put Device in its own header as well to remove the dependency).

Well, Device could go in the same header file as the Observer.

I don't feel super strongly about this, but it's been shown to be a weakness of
this pattern in the past.  I'll let you decide if you think it's worth it or
not.

[stevenjb, 2013-01-08 21:29:17]

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
File chromeos/network/network_device_handler.h (right):

https://codereview.chromium.org/11743006/diff/1/chromeos/network/network_devi...
chromeos/network/network_device_handler.h:40: class Observer {
On 2013/01/08 21:03:07, Greg Spencer (Chromium) wrote:
> On 2013/01/07 15:56:22, stevenjb (chromium) wrote:
> > On 2013/01/04 23:57:26, Greg Spencer (Chromium) wrote:
> > > This class should be in a separate header.  There was a thread about this
in
> > > chromium-dev and in a bug, where it was decided that because it becomes
> > > ambiguous at times when it's a nested class, and because then observers
> don't
> > > need to include the entire header that it's better to separate observers
> into
> > > separate headers.
> > 
> > I don't recall the specifics of that discussion, but we still use this
pattern
> > in Ash. I think it still makes sense when the Observer class has
dependencies
> on
> > the parent class (here we depend on DeviceMap which depends on Device, so we
> > would have to put Device in its own header as well to remove the
dependency).
> 
> Well, Device could go in the same header file as the Observer.
> 
> I don't feel super strongly about this, but it's been shown to be a weakness
of
> this pattern in the past.  I'll let you decide if you think it's worth it or
> not.

Yeah, but it wouldn't make sense to have Device in the _observer file, so you're
really talking three files at that point. I'm going to leave it as is. If this
gains complexity, then Device can become a class of its own and we can extract
the observer, the same as we did for NetworkState / Handler / Observer. In the
meanwhile, I hope to keep this class constrained and simple.

[commit-bot: I haz the power, 2013-01-08 21:30:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11743006/14001

[commit-bot: I haz the power, 2013-01-09 00:10:43]

Retried try job too often on win_aura for step(s) content_browsertests

[commit-bot: I haz the power, 2013-01-09 00:16:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11743006/14001

[commit-bot: I haz the power, 2013-01-09 02:21:21]

Change committed as 175673