apmanager: Add dbus implementation to chromium.

apmanager: Add dbus implementation to chromium.

Add a dbus implementation for the wifi ap manager. ap_manager is a
ChromeOS service that manages the hostapd instance; this is a dbus
binding that the ChromeOS service.

BUG=463194
Committed: https://crrev.com/4bca047f2cc173183673d799d1bd148cea08efa6
Cr-Commit-Position: refs/heads/master@{#319682}

[dtapuska, 2015-03-05 19:42:14]

dtapuska@chromium.org changed reviewers:
+ stevenjb@chromium.org, zqiu@chromium.org

[stevenjb, 2015-03-06 18:22:15]

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
File chromeos/dbus/ap_manager_client.cc (right):

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.cc:54: // is used.
This comment would be more meaningful in context (i.e. where this is called
from). Or name this function more specifically.

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
File chromeos/dbus/ap_manager_client.h (right):

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:70: class DeviceProperties : public
dbus::PropertySet {
We should be consistent and use structs or classes for PropertySet within this
class, rather than a mixture of both. (class is more consistent with Chrome C++
style, but the bluetooth code uses structs).

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:86: dbus::Property<std::string>
preferred_ap_interface_;
DISALLOW_COPY_AND_ASSIGN for classes

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:105: // Interface for observing changes from a
leadership daemon.
"leadership"?

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:153: const std::string& property_name);
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:162: // Adds and removes observers for events
on all leadership group
"leadership"?

[dtapuska, 2015-03-06 18:25:42]

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
File chromeos/dbus/ap_manager_client.h (right):

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:70: class DeviceProperties : public
dbus::PropertySet {
On 2015/03/06 18:22:15, stevenjb wrote:
> We should be consistent and use structs or classes for PropertySet within this
> class, rather than a mixture of both. (class is more consistent with Chrome
C++
> style, but the bluetooth code uses structs).

So the config is completely mutable; and the classes are not read only
objects...

I can move the struct to a class; but it will add a lot of boilerplate
setters/getters.

[dtapuska, 2015-03-06 18:26:38]

On 2015/03/06 18:25:42, Dave Tapuska wrote:
>
https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
> File chromeos/dbus/ap_manager_client.h (right):
> 
>
https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
> chromeos/dbus/ap_manager_client.h:70: class DeviceProperties : public
> dbus::PropertySet {
> On 2015/03/06 18:22:15, stevenjb wrote:
> > We should be consistent and use structs or classes for PropertySet within
this
> > class, rather than a mixture of both. (class is more consistent with Chrome
> C++
> > style, but the bluetooth code uses structs).
> 
> So the config is completely mutable; and the classes are not read only
> objects...
> 
> I can move the struct to a class; but it will add a lot of boilerplate
> setters/getters.

So the config is completely mutable; and the classes are "read only" objects...

[stevenjb, 2015-03-06 19:17:46]

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
File chromeos/dbus/ap_manager_client.h (right):

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:70: class DeviceProperties : public
dbus::PropertySet {
On 2015/03/06 18:25:42, Dave Tapuska wrote:
> On 2015/03/06 18:22:15, stevenjb wrote:
> > We should be consistent and use structs or classes for PropertySet within
this
> > class, rather than a mixture of both. (class is more consistent with Chrome
> C++
> > style, but the bluetooth code uses structs).
> 
> So the config is completely mutable; and the classes are not read only
> objects...
> 
I'm not sure I follow. If there is a fundamental difference between how these
are used than we should probably be inheriting from a different base class, or
at least making it clear with a comment. (i.e. why is ConfigProperties different
from DeviceProperties or ServiceProperties). To me, reading this, it is not at
all clear why one should be a struct and the other a class.

> I can move the struct to a class; but it will add a lot of boilerplate
> setters/getters.

That's been the argument for not making these classes in the past, but if we
apply that logic to one we should apply that to all of these.

Personally, if we don't want to add a lot of getters/setters, I would prefer to
see ConfigProperties as a class with an embedded Properties struct (which would
be a private member with accessors).

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:85: dbus::Property<bool> in_used_;
Also, these properties should be documented.

[dtapuska, 2015-03-06 19:57:53]

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
File chromeos/dbus/ap_manager_client.cc (right):

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.cc:54: // is used.
On 2015/03/06 18:22:15, stevenjb wrote:
> This comment would be more meaningful in context (i.e. where this is called
> from). Or name this function more specifically.

Done.

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
File chromeos/dbus/ap_manager_client.h (right):

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:70: class DeviceProperties : public
dbus::PropertySet {
On 2015/03/06 19:17:46, stevenjb wrote:
> On 2015/03/06 18:25:42, Dave Tapuska wrote:
> > On 2015/03/06 18:22:15, stevenjb wrote:
> > > We should be consistent and use structs or classes for PropertySet within
> this
> > > class, rather than a mixture of both. (class is more consistent with
Chrome
> > C++
> > > style, but the bluetooth code uses structs).
> > 
> > So the config is completely mutable; and the classes are not read only
> > objects...
> > 
> I'm not sure I follow. If there is a fundamental difference between how these
> are used than we should probably be inheriting from a different base class, or
> at least making it clear with a comment. (i.e. why is ConfigProperties
different
> from DeviceProperties or ServiceProperties). To me, reading this, it is not at
> all clear why one should be a struct and the other a class.
> 
> > I can move the struct to a class; but it will add a lot of boilerplate
> > setters/getters.
> 
> That's been the argument for not making these classes in the past, but if we
> apply that logic to one we should apply that to all of these.
> 
> Personally, if we don't want to add a lot of getters/setters, I would prefer
to
> see ConfigProperties as a class with an embedded Properties struct (which
would
> be a private member with accessors).

Done.

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:85: dbus::Property<bool> in_used_;
On 2015/03/06 19:17:46, stevenjb wrote:
> Also, these properties should be documented.

Done.

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:86: dbus::Property<std::string>
preferred_ap_interface_;
On 2015/03/06 18:22:15, stevenjb wrote:
> DISALLOW_COPY_AND_ASSIGN for classes

Done.

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:105: // Interface for observing changes from a
leadership daemon.
On 2015/03/06 18:22:15, stevenjb wrote:
> "leadership"?

Done.

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:153: const std::string& property_name);
On 2015/03/06 18:22:15, stevenjb wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/980973003/diff/20001/chromeos/dbus/ap_manager...
chromeos/dbus/ap_manager_client.h:162: // Adds and removes observers for events
on all leadership group
On 2015/03/06 18:22:15, stevenjb wrote:
> "leadership"?

Done.

[stevenjb, 2015-03-06 21:36:41]

stevenjb@chromium.org changed required reviewers:
+ zqiu@chromium.org

[stevenjb, 2015-03-06 21:36:42]

Thanks. Even though this is more verbose I think this makes it much more clear
that the ConfigProperties members are settable and the Service/DeviceProperties
are not. This may seem obvious based on the name "Config", but dbus interfaces
are not exactly consistent that way and I think it is best not to rely on naming
conventions for that sort of thing.

Owner lgtm (this should also be reviewed by someone familiar with the service).

[zqiu1, 2015-03-06 21:59:50]

lgtm

[dtapuska, 2015-03-06 22:06:30]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2015-03-06 22:07:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980973003/40001

[commit-bot: I haz the power, 2015-03-07 00:09:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-07 00:09:12]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[dtapuska, 2015-03-09 17:20:41]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2015-03-09 17:21:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980973003/40001

[commit-bot: I haz the power, 2015-03-09 18:01:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-09 18:02:14]

Patchset 3 (id:??) landed as
https://crrev.com/4bca047f2cc173183673d799d1bd148cea08efa6
Cr-Commit-Position: refs/heads/master@{#319682}