dbus: don't fail when reconnecting object signals

dbus/object_proxy.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "dbus/bus.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/metrics/histogram.h"
@@ skipping 263 matching lines @@
   delete data;
 }
 
 void ObjectProxy::ConnectToSignalInternal(
     const std::string& interface_name,
     const std::string& signal_name,
     SignalCallback signal_callback,
     OnConnectedCallback on_connected_callback) {
   bus_->AssertOnDBusThread();
 
-  // Check if the object is already connected to the signal.
   const std::string absolute_signal_name =
       GetAbsoluteSignalName(interface_name, signal_name);
-  if (method_table_.find(absolute_signal_name) != method_table_.end()) {
-    LOG(ERROR) << "The object proxy is already connected to "
-               << absolute_signal_name;
-    return;
-  }
 
   // Will become true, if everything is successful.
   bool success = false;
 
   if (bus_->Connect() && bus_->SetUpAsyncOperations()) {
     // We should add the filter only once. Otherwise, HandleMessage() will
     // be called more than once.
     if (!filter_added_) {
       if (bus_->AddFilterFunction(&ObjectProxy::HandleMessageThunk, this)) {
         filter_added_ = true;
       } else {
         LOG(ERROR) << "Failed to add filter function";
       }
     }
     // Add a match rule so the signal goes through HandleMessage().
     const std::string match_rule =
         base::StringPrintf("type='signal', interface='%s', path='%s'",
                            interface_name.c_str(),
                            object_path_.value().c_str());
 
     // Add the match rule if we don't have it.
     if (match_rules_.find(match_rule) == match_rules_.end()) {

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

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.

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

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 :)

       ScopedDBusError error;
       bus_->AddMatch(match_rule, error.get());;
       if (error.is_set()) {
         LOG(ERROR) << "Failed to add match rule: " << match_rule;
       } else {
         // Store the match rule, so that we can remove this in Detach().
         match_rules_.insert(match_rule);
         // Add the signal callback to the method table.
         method_table_[absolute_signal_name] = signal_callback;

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

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]

Yes, that's the intent of this patch

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

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]

yup

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

Done.

         success = true;
       }
     } else {
       // We already have the match rule.
       method_table_[absolute_signal_name] = signal_callback;
       success = true;
     }
   }
 
   // Run on_connected_callback in the origin thread.
@@ skipping 96 matching lines @@
 void ObjectProxy::LogMethodCallFailure(
     const base::StringPiece& error_name,
     const base::StringPiece& error_message) const {
   if (ignore_service_unknown_errors_ && error_name == kErrorServiceUnknown)
     return;
   LOG(ERROR) << "Failed to call method: " << error_name
              << ": " << error_message;
 }
 
 }  // namespace dbus