Support D-Bus Object Manager

Support D-Bus Object Manager

Object Manager is a new standard D-Bus interface, closely related to
the Properties interface. It is used by BlueZ 5.x thus the need to
implement it now.

The intended use is that Chrome D-Bus Client singletons set up a link
to an object manager in their constructor and register themselves to
handle their particular interface.

BUG=220951
TEST=dbus_unittests

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

[keybuk, 2013-03-12 22:13:57]

wip, posting in case I get hit by a bus on vacation ;)

[keybuk, 2013-03-20 01:31:52]

This should now be complete.

satorux: please review

[satorux1, 2013-03-21 05:45:42]

Looks pretty neat.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.cc#new...
dbus/object_manager.cc:195: it != interface_names.end(); ++it)
matter of taste: for (size_t i = 0; ...) is more succinct.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h
File dbus/object_manager.h (right):

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:17: // notification of changes in the set of available
objects.
Please add a pointer to the spec:

http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-o...

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:52: // The link between the implementation class and the
object manager is setup
setup -> set up?

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:80: //       OVERRIDE{
OVERRIDE {

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:115: // documented in dbus/property.h still apply.
Nicely documented. Thank you for writing this!

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:178: };
You might want to externalize this class as ObjectManagerInterface, so client
code can forward-declare this class.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:199: std::vector<ObjectPath> GetObjects();
Might want to mention that the order is undefined?

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:201: // Returns the list of object paths of objects
implementing the interface
ditto.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:227: typedef std::map<std::string, Interface*>
InterfaceMap;
Please document. Key is an interface name, right?

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:241: ObjectMap object_map_;
Per the style guide, data members should be placed at the end.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:260: const std::string& interface_name);
Could you add comments for the three non-trivial functions?

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
File dbus/object_manager_unittest.cc (right):

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:196: EXPECT_TRUE(object_proxy != NULL);
I think EXPECT_TRUE(object_proxy) should compile

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:222: EXPECT_TRUE(object_proxy == NULL);
EXPECT_FALSE(object_proxy) ?

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:295: std::vector<dbus::ObjectPath> object_paths
= object_manager_->GetObjects();
ASSERT_EQ(2U, object_paths.size()) ?

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:299: *iter ==
dbus::ObjectPath("/org/chromium/SecondObject"));
What about making object paths sortable and do:

std::sort(object_paths.begin(), object_paths.end());
EXPECT_EQ(dbus::ObjectPath("/org/chromium/SecondObject", object_paths[0]);
EXPECT_EQ(dbus::ObjectPath("/org/chromium/TestObject", object_paths[1]);

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:307: *iter ==
dbus::ObjectPath("/org/chromium/SecondObject"));
ditto.

[keybuk, 2013-03-21 17:50:00]

ptal

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.cc#new...
dbus/object_manager.cc:195: it != interface_names.end(); ++it)
On 2013/03/21 05:45:43, satorux1 wrote:
> matter of taste: for (size_t i = 0; ...) is more succinct.

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h
File dbus/object_manager.h (right):

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:17: // notification of changes in the set of available
objects.
On 2013/03/21 05:45:43, satorux1 wrote:
> Please add a pointer to the spec:
> 
>
http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-o...

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:52: // The link between the implementation class and the
object manager is setup
On 2013/03/21 05:45:43, satorux1 wrote:
> setup -> set up?

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:80: //       OVERRIDE{
On 2013/03/21 05:45:43, satorux1 wrote:
> OVERRIDE {

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:178: };
On 2013/03/21 05:45:43, satorux1 wrote:
> You might want to externalize this class as ObjectManagerInterface, so client
> code can forward-declare this class.

You can't inherit from forward-declared classes, you always need the full
definition, and only ObjectManager takes an pointer to the base class.

It seemed to fit the contained class pattern better, since like Observer or
Delegate, it's an interface this class needs you to implement.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:199: std::vector<ObjectPath> GetObjects();
On 2013/03/21 05:45:43, satorux1 wrote:
> Might want to mention that the order is undefined?

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:201: // Returns the list of object paths of objects
implementing the interface
On 2013/03/21 05:45:43, satorux1 wrote:
> ditto.

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:227: typedef std::map<std::string, Interface*>
InterfaceMap;
On 2013/03/21 05:45:43, satorux1 wrote:
> Please document. Key is an interface name, right?

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:241: ObjectMap object_map_;
On 2013/03/21 05:45:43, satorux1 wrote:
> Per the style guide, data members should be placed at the end.

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:260: const std::string& interface_name);
On 2013/03/21 05:45:43, satorux1 wrote:
> Could you add comments for the three non-trivial functions?

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
File dbus/object_manager_unittest.cc (right):

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:196: EXPECT_TRUE(object_proxy != NULL);
On 2013/03/21 05:45:43, satorux1 wrote:
> I think EXPECT_TRUE(object_proxy) should compile

One or the other of these doesn't compile, because it can't cast NULL to bool;
the EXPECT_TRUE/FALSE macros expand out to templates, and logic ceases to apply.

I got bitten by this before, so figured to write it explicitly since I can't
remember which way round fails ;)

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:222: EXPECT_TRUE(object_proxy == NULL);
On 2013/03/21 05:45:43, satorux1 wrote:
> EXPECT_FALSE(object_proxy) ?

See above.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:295: std::vector<dbus::ObjectPath> object_paths
= object_manager_->GetObjects();
On 2013/03/21 05:45:43, satorux1 wrote:
> ASSERT_EQ(2U, object_paths.size()) ?

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:299: *iter ==
dbus::ObjectPath("/org/chromium/SecondObject"));
On 2013/03/21 05:45:43, satorux1 wrote:
> What about making object paths sortable and do:
> 
> std::sort(object_paths.begin(), object_paths.end());
> EXPECT_EQ(dbus::ObjectPath("/org/chromium/SecondObject", object_paths[0]);
> EXPECT_EQ(dbus::ObjectPath("/org/chromium/TestObject", object_paths[1]);

Done.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:307: *iter ==
dbus::ObjectPath("/org/chromium/SecondObject"));
On 2013/03/21 05:45:43, satorux1 wrote:
> ditto.

Done.

[satorux1, 2013-03-22 01:41:33]

LGTM with nits

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h
File dbus/object_manager.h (right):

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager.h#newc...
dbus/object_manager.h:178: };
On 2013/03/21 17:50:00, keybuk wrote:
> On 2013/03/21 05:45:43, satorux1 wrote:
> > You might want to externalize this class as ObjectManagerInterface, so
client
> > code can forward-declare this class.
> 
> You can't inherit from forward-declared classes, you always need the full
> definition, and only ObjectManager takes an pointer to the base class.
> 
> It seemed to fit the contained class pattern better, since like Observer or
> Delegate, it's an interface this class needs you to implement.

Sounds reasonable.

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
File dbus/object_manager_unittest.cc (right):

https://codereview.chromium.org/12491014/diff/9001/dbus/object_manager_unitte...
dbus/object_manager_unittest.cc:196: EXPECT_TRUE(object_proxy != NULL);
On 2013/03/21 17:50:00, keybuk wrote:
> On 2013/03/21 05:45:43, satorux1 wrote:
> > I think EXPECT_TRUE(object_proxy) should compile
> 
> One or the other of these doesn't compile, because it can't cast NULL to bool;
> the EXPECT_TRUE/FALSE macros expand out to templates, and logic ceases to
apply.
> 
> I got bitten by this before, so figured to write it explicitly since I can't
> remember which way round fails ;)

Ah I see.

https://codereview.chromium.org/12491014/diff/15001/dbus/object_manager.h
File dbus/object_manager.h (right):

https://codereview.chromium.org/12491014/diff/15001/dbus/object_manager.h#new...
dbus/object_manager.h:225: void GetManagedObjects();
The message (list of list of dictionaries) we'll receive by this method is
fairly complex. Could you draw some example in an ASCII art form?

https://codereview.chromium.org/12491014/diff/15001/dbus/object_manager.h#new...
dbus/object_manager.h:250: // GetManagedObjects() method return and the
InterfacesAdded() signal.
Likewise, having some example of the input message would be helpful here.

https://codereview.chromium.org/12491014/diff/15001/dbus/object_manager.h#new...
dbus/object_manager.h:273: Bus* bus_;
Maybe add a comment about the ownership:

Bus* bus_;  // Not owned.

https://codereview.chromium.org/12491014/diff/15001/dbus/object_manager.h#new...
dbus/object_manager.h:276: ObjectProxy* object_proxy_;
ditto

[satorux1, 2013-03-22 01:56:27]

https://codereview.chromium.org/12491014/diff/15001/dbus/object_manager.h
File dbus/object_manager.h (right):

https://codereview.chromium.org/12491014/diff/15001/dbus/object_manager.h#new...
dbus/object_manager.h:179: const std::string& interface_name) { }
Looking at the other patch, ObjectAdded() and ObjectRemoved() seemed to be
always implemented. Maybe you can make them pure virtual?

[keybuk, 2013-03-25 16:00:10]

https://codereview.chromium.org/12491014/diff/15001/dbus/object_manager.h
File dbus/object_manager.h (right):

https://codereview.chromium.org/12491014/diff/15001/dbus/object_manager.h#new...
dbus/object_manager.h:179: const std::string& interface_name) { }
On 2013/03/22 01:56:27, satorux1 wrote:
> Looking at the other patch, ObjectAdded() and ObjectRemoved() seemed to be
> always implemented. Maybe you can make them pure virtual?
> 
I didn't feel it should be mandatory, it just happened I was going to implement
them for both

[commit-bot: I haz the power, 2013-03-25 16:00:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/12491014/15001

[commit-bot: I haz the power, 2013-03-25 16:16:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-25 16:54:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/12491014/22003

[commit-bot: I haz the power, 2013-03-25 17:14:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-25 17:14:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/12491014/34001

[commit-bot: I haz the power, 2013-03-25 18:08:44]

Retried try job too often on linux_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[keybuk, 2013-03-25 18:20:20]

Committed patchset #8 manually as r190440 (presubmit successful).