dbus: don't fail when reconnecting object signals

dbus: don't fail when reconnecting object signals

Since dbus::ObjectProxy is silently cached, with no way to invalidate,
it's possible that individual instances of objects will come and go
using the same underlying object proxy.  i.e. dbus::PropertySet

These will need to change the signal callbacks to be bound to their
own instance, so the current behaviour of failing in this case with
a log message is pessimal.

Change dbus::ObjectProxy to overwrite the existing signal callbacks
with the new ones on repeated calls, rather than preserve the first.

BUG=chromium-os:28064
TEST=unit test included, and we receive property notifications on devices after connection now

Change-Id: Ic4ae092163a364c53bdfcf88f4ce8f74b110b5cb

R=satorux@chromium.org


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128100

[satorux1, 2012-03-21 04:59:40]

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc#newcode316
dbus/object_proxy.cc:316: method_table_[absolute_signal_name] = signal_callback;
if method_table_[absolute_signal_name] is already present, the existing
signal_callback is replaced with the new one. Is it OK?

[keybuk, 2012-03-21 14:51:29]

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc#newcode316
dbus/object_proxy.cc:316: method_table_[absolute_signal_name] = signal_callback;
Yes, that's the intent of this patch

[satorux1, 2012-03-21 16:20:28]

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc#newcode307
dbus/object_proxy.cc:307: if (match_rules_.find(match_rule) ==
match_rules_.end()) {
When reconnecting to the same signal, match_rules_may already contain the match
rule? If so, the code inside the 'if' won't be executed.

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc#newcode316
dbus/object_proxy.cc:316: method_table_[absolute_signal_name] = signal_callback;
On 2012/03/21 14:51:29, keybuk wrote:
> Yes, that's the intent of this patch

I see I see. Could you document this behavior in object_proxy.h? Would be nice
if we have a unit test for this .

[keybuk, 2012-03-21 16:22:41]

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc#newcode307
dbus/object_proxy.cc:307: if (match_rules_.find(match_rule) ==
match_rules_.end()) {
exactly, which is what you want - there's already a match rule, so don't try and
add it again - just connect the signal callback to it

that was the irony of this method, it was perfectly safe to call multiple times
because it already guarded against all the possibilities, it just refused to run
them :)

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc#newcode316
dbus/object_proxy.cc:316: method_table_[absolute_signal_name] = signal_callback;
yup

[keybuk, 2012-03-21 21:26:28]

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/9808001/diff/1/dbus/object_proxy.cc#newcode316
dbus/object_proxy.cc:316: method_table_[absolute_signal_name] = signal_callback;
On 2012/03/21 16:20:28, satorux1 wrote:
> On 2012/03/21 14:51:29, keybuk wrote:
> > Yes, that's the intent of this patch
> 
> I see I see. Could you document this behavior in object_proxy.h? Would be nice
> if we have a unit test for this .

Done.

[satorux1, 2012-03-21 21:53:55]

LGTM

[commit-bot: I haz the power, 2012-03-21 22:06:05]

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

[commit-bot: I haz the power, 2012-03-21 23:39:19]

Change committed as 128100