Add DBus Bindings for leaderd

Add DBus Bindings for leaderd

leaderd is a ChromeOS system daemon that exposes group leader
capability. Expose the ability to create groups, join and listen
to events on the groups.

BUG=453872
Committed: https://crrev.com/fb2a760558d423c55722e2a6f36d9c805f4eb9b6
Cr-Commit-Position: refs/heads/master@{#316288}

[dtapuska, 2015-01-30 19:06:52]

dtapuska@chromium.org changed reviewers:
+ benchan@chromium.org, stevenjb@chromium.org, wiley@chromium.org

[dtapuska, 2015-01-30 19:09:19]

dtapuska@chromium.org changed reviewers:
+ pwestin@chromium.org

[dtapuska, 2015-02-04 21:54:19]

On 2015/01/30 19:09:19, Dave Tapuska wrote:
> mailto:dtapuska@chromium.org changed reviewers:
> + mailto:pwestin@chromium.org

Ping

[dtapuska, 2015-02-09 17:32:39]

dtapuska@chromium.org changed reviewers:
+ hashimoto@chromium.org

[stevenjb, 2015-02-12 18:27:25]

FWIW, I am waiting on someone familiar with this effort to do a review pass
before doing any owner review.

[Ben Chan, 2015-02-12 22:05:35]

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
File chromeos/dbus/leadership_daemon_manager_client.h (right):

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:23: // LeadershipDaemon Manager
just personal feeling, perhaps "leaderd's Manager" would be more clear than
"LeadershipDaemon Manager" in this context?

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:35: const std::string&
LeaderUUID() const { return leaderUUID_.value(); }
style: trivial / inline getter should be named as leader_uuid()

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:36: const
std::vector<std::string>& GroupMembers() const {
ditto

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:41: dbus::Property<std::string>
leaderUUID_;
style: it should be named |leader_uuid_|

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:52: virtual void
GroupAdded(const dbus::ObjectPath& object_path) {}
no sure the convention here (I guess it's probably 50-50),

GroupAdded vs OnGroupAdded

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:59: virtual void ManagerAdded()
{}
nit: group related methods together, i.e.

Manager*

Group*

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:118: // Obtain the properties
for the group with object path |object_path|,
Obtains

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:119: // any values should be
copied if needed.
This comment seems to imply ownership of the return value is not transferred to
the caller, and the caller should probably not modify the return value. should
probably return as "const GroupProperties *" to reflect that.

[dtapuska, 2015-02-12 22:45:23]

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
File chromeos/dbus/leadership_daemon_manager_client.h (right):

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:23: // LeadershipDaemon Manager
On 2015/02/12 22:05:35, Ben Chan wrote:
> just personal feeling, perhaps "leaderd's Manager" would be more clear than
> "LeadershipDaemon Manager" in this context?

Done.

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:35: const std::string&
LeaderUUID() const { return leaderUUID_.value(); }
On 2015/02/12 22:05:35, Ben Chan wrote:
> style: trivial / inline getter should be named as leader_uuid()

Done.

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:36: const
std::vector<std::string>& GroupMembers() const {
On 2015/02/12 22:05:35, Ben Chan wrote:
> ditto

Done.

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:41: dbus::Property<std::string>
leaderUUID_;
On 2015/02/12 22:05:35, Ben Chan wrote:
> style: it should be named |leader_uuid_|

Done.

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:52: virtual void
GroupAdded(const dbus::ObjectPath& object_path) {}
On 2015/02/12 22:05:35, Ben Chan wrote:
> no sure the convention here (I guess it's probably 50-50),
> 
> GroupAdded vs OnGroupAdded

The observers in bluetooth don't use the On prefix either. I did the similar
thing for peerd as well.

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:59: virtual void ManagerAdded()
{}
On 2015/02/12 22:05:35, Ben Chan wrote:
> nit: group related methods together, i.e.
> 
> Manager*
> 
> Group*

Done.

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:118: // Obtain the properties
for the group with object path |object_path|,
On 2015/02/12 22:05:35, Ben Chan wrote:
> Obtains

Done.

https://codereview.chromium.org/887083002/diff/40001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:119: // any values should be
copied if needed.
On 2015/02/12 22:05:35, Ben Chan wrote:
> This comment seems to imply ownership of the return value is not transferred
to
> the caller, and the caller should probably not modify the return value. should
> probably return as "const GroupProperties *" to reflect that.

Done.

[stevenjb, 2015-02-12 23:03:23]

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/dbus_clien...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/dbus_clien...
chromeos/dbus/dbus_client_bundle.cc:288: new FakeLeadershipDaemonManagerClient);
{} around multi line if clauses

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/fake_leade...
File chromeos/dbus/fake_leadership_daemon_manager_client.cc (right):

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/fake_leade...
chromeos/dbus/fake_leadership_daemon_manager_client.cc:28:
callback.Run(DBUS_METHOD_CALL_SUCCESS, std::string());
It would actually be preferable to post these callbacks as tasks to better
simulate the actual behavior.

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...
File chromeos/dbus/leadership_daemon_manager_client.cc (right):

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.cc:69: // DBusClient overrides:
Use '.' or ':' (or omit 'overrides' entirely, e.g. just "// DBusClient" )
consistently.

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...
File chromeos/dbus/leadership_daemon_manager_client.h (right):

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:37: }
Either make this a class or use members without the trailing _ and no accessors.

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:50: virtual void ManagerAdded()
{}
Classes with concrete (even empty) implementations should have those defined in
the .cc file. (I think there are a lot of bad examples, but even so we shouldn't
propagate that).

[dtapuska, 2015-02-13 15:23:49]

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/dbus_clien...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/dbus_clien...
chromeos/dbus/dbus_client_bundle.cc:288: new FakeLeadershipDaemonManagerClient);
On 2015/02/12 23:03:22, stevenjb wrote:
> {} around multi line if clauses

Done.

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/fake_leade...
File chromeos/dbus/fake_leadership_daemon_manager_client.cc (right):

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/fake_leade...
chromeos/dbus/fake_leadership_daemon_manager_client.cc:28:
callback.Run(DBUS_METHOD_CALL_SUCCESS, std::string());
On 2015/02/12 23:03:22, stevenjb wrote:
> It would actually be preferable to post these callbacks as tasks to better
> simulate the actual behavior.

Done.

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...
File chromeos/dbus/leadership_daemon_manager_client.cc (right):

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.cc:69: // DBusClient overrides:
On 2015/02/12 23:03:22, stevenjb wrote:
> Use '.' or ':' (or omit 'overrides' entirely, e.g. just "// DBusClient" )
> consistently.

Done.

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...
File chromeos/dbus/leadership_daemon_manager_client.h (right):

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:37: }
On 2015/02/12 23:03:22, stevenjb wrote:
> Either make this a class or use members without the trailing _ and no
accessors.

Done.

https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...
chromeos/dbus/leadership_daemon_manager_client.h:50: virtual void ManagerAdded()
{}
On 2015/02/12 23:03:22, stevenjb wrote:
> Classes with concrete (even empty) implementations should have those defined
in
> the .cc file. (I think there are a lot of bad examples, but even so we
shouldn't
> propagate that).

Done.

[stevenjb, 2015-02-13 18:53:19]

lgtm

https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/fake_lead...
File chromeos/dbus/fake_leadership_daemon_manager_client.cc (right):

https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/fake_lead...
chromeos/dbus/fake_leadership_daemon_manager_client.cc:11: namespace {
bit: blank line here

https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/fake_lead...
chromeos/dbus/fake_leadership_daemon_manager_client.cc:19: }
nit: // namespace + blank line before

https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/leadershi...
File chromeos/dbus/leadership_daemon_manager_client.cc (right):

https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/leadershi...
chromeos/dbus/leadership_daemon_manager_client.cc:72: //
dbus::ObjectManager::Interface override.
nit: overrides

[Ben Chan, 2015-02-13 19:01:03]

On 2015/02/13 18:53:19, stevenjb wrote:
> lgtm
> 
>
https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/fake_lead...
> File chromeos/dbus/fake_leadership_daemon_manager_client.cc (right):
> 
>
https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/fake_lead...
> chromeos/dbus/fake_leadership_daemon_manager_client.cc:11: namespace {
> bit: blank line here
> 
>
https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/fake_lead...
> chromeos/dbus/fake_leadership_daemon_manager_client.cc:19: }
> nit: // namespace + blank line before
> 
>
https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/leadershi...
> File chromeos/dbus/leadership_daemon_manager_client.cc (right):
> 
>
https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/leadershi...
> chromeos/dbus/leadership_daemon_manager_client.cc:72: //
> dbus::ObjectManager::Interface override.
> nit: overrides

lgtm given that wiley signs off on the DBus API

[wiley1, 2015-02-13 19:13:12]

On 2015/02/13 19:01:03, Ben Chan wrote:
> On 2015/02/13 18:53:19, stevenjb wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/fake_lead...
> > File chromeos/dbus/fake_leadership_daemon_manager_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/fake_lead...
> > chromeos/dbus/fake_leadership_daemon_manager_client.cc:11: namespace {
> > bit: blank line here
> > 
> >
>
https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/fake_lead...
> > chromeos/dbus/fake_leadership_daemon_manager_client.cc:19: }
> > nit: // namespace + blank line before
> > 
> >
>
https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/leadershi...
> > File chromeos/dbus/leadership_daemon_manager_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/887083002/diff/100001/chromeos/dbus/leadershi...
> > chromeos/dbus/leadership_daemon_manager_client.cc:72: //
> > dbus::ObjectManager::Interface override.
> > nit: overrides
> 
> lgtm given that wiley signs off on the DBus API

I'm fine with the DBus API in the design document.  I think Dave is testing
against that API, so this should be fine.

[dtapuska, 2015-02-13 20:34:51]

New patchsets have been uploaded after l-g-t-m from
stevenjb@chromium.org,benchan@chromium.org

[dtapuska, 2015-02-13 20:40:17]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2015-02-13 20:41:04]

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

[commit-bot: I haz the power, 2015-02-13 21:12:12]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-13 21:13:02]

Patchset 8 (id:??) landed as
https://crrev.com/fb2a760558d423c55722e2a6f36d9c805f4eb9b6
Cr-Commit-Position: refs/heads/master@{#316288}

[dtapuska, 2015-02-19 13:57:20]

Patchset #9 (id:160001) has been deleted