Linux/CrOS: Retry connecting to mtpd.

dbus/bus_unittest.cc

Index: dbus/bus_unittest.cc
===================================================================
--- dbus/bus_unittest.cc	(revision 203477)
+++ dbus/bus_unittest.cc	(working copy)
@@ -7,11 +7,13 @@
 #include "base/bind.h"
 #include "base/message_loop.h"
 #include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
 #include "base/threading/thread.h"
 #include "dbus/exported_object.h"
 #include "dbus/object_path.h"
 #include "dbus/object_proxy.h"
 #include "dbus/scoped_dbus_error.h"
+#include "dbus/test_service.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -24,6 +26,94 @@
   return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
+// The test helpers below are for BusTest.ListenForServiceOwnerChange.
+
+class OwnershipChangeListener {
+ public:
+  OwnershipChangeListener() : owner_changes_(0) {}
+  ~OwnershipChangeListener() {}
+
+  void OnServiceOwnerChanged(const std::string& new_service_owner) {
+    current_owner_ = new_service_owner;
+    ++owner_changes_;
+  }
+
+  void set_current_owner(const std::string& owner) { current_owner_ = owner; }
+  const std::string& current_owner() const { return current_owner_; }
+
+  int owner_changes() const { return owner_changes_; }
+
+ private:
+  std::string current_owner_;
+  int owner_changes_;
+
+  DISALLOW_COPY_AND_ASSIGN(OwnershipChangeListener);
+};
+
+void RequestOwnershipHelper(const base::Closure& quit_closure,
+                            std::string* service_name,
+                            bool* is_owner,
+                            const std::string& actual_service_name,
+                            bool actual_is_owner) {
+  *service_name = actual_service_name;
+  *is_owner = actual_is_owner;
+  quit_closure.Run();
+}
+
+void RequestOwnership(dbus::Bus* bus, const std::string& service_name) {
+  base::RunLoop run_loop;
+  std::string requested_service_name;
+  bool got_owner = false;
+  bus->RequestOwnership(service_name,
+                        base::Bind(&RequestOwnershipHelper,
+                                   run_loop.QuitClosure(),
+                                   &requested_service_name,
+                                   &got_owner));
+  run_loop.Run();
+  ASSERT_TRUE(got_owner);
+  ASSERT_EQ(service_name, requested_service_name);
+}
+
+void ReleaseOwnershipHelper(const base::Closure& quit_closure,
+                            dbus::Bus* bus,
+                            const std::string& service_name,
+                            bool* released) {
+  *released = bus->ReleaseOwnership(service_name);
+  bus->PostTaskToOriginThread(FROM_HERE, quit_closure);
+}
+
+void ReleaseOwnership(dbus::Bus* bus, const std::string& service_name) {
+  base::RunLoop run_loop;
+  bool released = false;
+  bus->PostTaskToDBusThread(
+      FROM_HERE,
+      base::Bind(&ReleaseOwnershipHelper,
+                 run_loop.QuitClosure(),
+                 make_scoped_refptr(bus),
+                 service_name,
+                 &released));
+  run_loop.Run();
+  ASSERT_TRUE(released);
+}
+
+void GetServiceOwnerHelper(const base::Closure& quit_closure,
+                           std::string* service_owner,
+                           const std::string& actual_service_owner) {
+  *service_owner = actual_service_owner;
+  quit_closure.Run();
+}
+
+std::string GetServiceOwner(dbus::Bus* bus, const std::string& service_name) {
+  base::RunLoop run_loop;
+  std::string service_owner;
+  bus->GetServiceOwner(service_name,
+                       base::Bind(&GetServiceOwnerHelper,
+                                  run_loop.QuitClosure(),
+                                  &service_owner));
+  run_loop.Run();
+  return service_owner;
+}
+
 }  // namespace
 
 TEST(BusTest, GetObjectProxy) {
@@ -296,3 +386,77 @@
 
   bus->ShutdownAndBlock();
 }
+
+TEST(BusTest, ListenForServiceOwnerChange) {
+  // Setup the current thread's MessageLoop.
+  base::MessageLoop message_loop;
+
+  // Start the D-Bus thread.
+  base::Thread::Options thread_options;
+  thread_options.message_loop_type = base::MessageLoop::TYPE_IO;
+  base::Thread dbus_thread("D-Bus thread");

[satorux1, 2013/06/05 01:33:58]

Do we need to create a D-Bus thread? Other test cases in this file don't create
a D-Bus thread. 

Lots of helper functions (ex. GetServiceOwner() and GetServiceOwnerHelper())
were added in order to emulate blocking calls. These are unnecessary, if the
D-Bus thread is not in use. You can just use blocking functions liek
Bus::GetServiceOwnerAndBlock().

[Lei Zhang, 2013/06/09 04:37:57]

I copied it from the test right above this one and it made things a lot more
complicated. The next patch set doesn't have the DBus thread and is a lot
simpler.

+  dbus_thread.StartWithOptions(thread_options);
+
+  // Create the bus.
+  dbus::Bus::Options bus_options;
+  bus_options.dbus_task_runner = dbus_thread.message_loop_proxy();
+  scoped_refptr<dbus::Bus> bus = new dbus::Bus(bus_options);
+  ASSERT_FALSE(bus->shutdown_completed());
+
+  // Add a listener.
+  OwnershipChangeListener ownership_change_listener_1;

[satorux1, 2013/06/05 01:33:58]

nit: ownership_change_listener1 ? having _ before number looked a bit unusual

[Lei Zhang, 2013/06/09 04:37:57]

Done.

+  ownership_change_listener_1.set_current_owner("dummy1");

[satorux1, 2013/06/05 01:33:58]

At first, I was confused about why a dummy value was set here. This is necessary
to confirm that GetServiceOwner() returns an empty string, rather than the dummy
value, right?

Seems to me that this test case is testing GetServiceOwner() and
ListenForServiceOwnerChange(). I think it'd be nicer to have a separate test
case for  GetServiceOwner(). Then, we can just assume that GetServiceOwner()
works properly hence no need to set a dummy value here.

+  dbus::Bus::GetServiceOwnerCallback callback1 =
+      base::Bind(&OwnershipChangeListener::OnServiceOwnerChanged,

[satorux1, 2013/06/05 01:33:58]

This class seems to be unnecessary. Can we just pass some parameters to a
callback bound to a free function like:

dbus::Bus::GetServiceOwnerCallback callback1 =
   base::Bind(&OnServiceOwnerChanged,
              &current_owner,
              &num_owner_changes));

That'd be simpler than having a class.

If we are to stop creating a D-Bus thread, you'd need to pass a quit closure to
that callback function to quit the message loop. The code would look like:

bus_->RequestOwnershipAndBlock(...);
run_loop.Run();  // OnServiceOwnerChanged() will quit the loop.

[Lei Zhang, 2013/06/09 04:37:57]

Done
But when I have 2 listeners, they will call Quit() 1 time too many. I used
RunUntilIdle() instead.

+                 base::Unretained(&ownership_change_listener_1));
+  bus->ListenForServiceOwnerChange("org.chromium.TestService", callback1);
+  // This should be a no-op.
+  bus->ListenForServiceOwnerChange("org.chromium.TestService", callback1);
+
+  // Nothing has happened yet. Check initial state.
+  // The blocking GetServiceOwner() call makes sure the
+  // ListenForServiceOwnerChange() calls happen first.
+  std::string service_owner = GetServiceOwner(bus, "org.chromium.TestService");
+  ASSERT_TRUE(service_owner.empty());
+  ASSERT_EQ("dummy1", ownership_change_listener_1.current_owner());
+  EXPECT_EQ(0, ownership_change_listener_1.owner_changes());
+
+  // Make an ownership change.
+  RequestOwnership(bus, "org.chromium.TestService");
+  service_owner = GetServiceOwner(bus, "org.chromium.TestService");

[satorux1, 2013/06/05 01:33:58]

How does the |service_owner_| look like? Could you add an example here as a
comment?

+  ASSERT_FALSE(service_owner.empty());
+  ASSERT_EQ(service_owner, ownership_change_listener_1.current_owner());
+
+  // Test the second ListenForServiceOwnerChange() above is indeed a no-op.
+  EXPECT_EQ(1, ownership_change_listener_1.owner_changes());
+
+  // Add a second listener.
+  OwnershipChangeListener ownership_change_listener_2;
+  ownership_change_listener_2.set_current_owner("dummy2");
+  dbus::Bus::GetServiceOwnerCallback callback2 =
+      base::Bind(&OwnershipChangeListener::OnServiceOwnerChanged,
+                 base::Unretained(&ownership_change_listener_2));
+  bus->ListenForServiceOwnerChange("org.chromium.TestService", callback2);
+  bus->ListenForServiceOwnerChange("org.chromium.FakeService", callback2);

[satorux1, 2013/06/05 01:33:58]

Why do we listen to "org.chromium.FakeService"? Could you add a comment, or
remove the code?

[Lei Zhang, 2013/06/09 04:37:57]

I guess it doesn't add too much value to the test, removed.

+
+  // The blocking ReleaseOwnership() call makes sure the
+  // ListenForServiceOwnerChange() calls happen first.
+  ReleaseOwnership(bus, "org.chromium.TestService");
+  service_owner = GetServiceOwner(bus, "org.chromium.TestService");
+  ASSERT_TRUE(service_owner.empty());
+  ASSERT_TRUE(ownership_change_listener_1.current_owner().empty());
+  ASSERT_TRUE(ownership_change_listener_2.current_owner().empty());
+  EXPECT_EQ(2, ownership_change_listener_1.owner_changes());
+  EXPECT_EQ(1, ownership_change_listener_2.owner_changes());
+
+  bus->UnlistenForServiceOwnerChange("org.chromium.FakeService", callback2);
+  bus->UnlistenForServiceOwnerChange("org.chromium.TestService", callback1);
+  bus->UnlistenForServiceOwnerChange("org.chromium.TestService", callback2);
+
+  // Shut down synchronously. The pending UnlistenForServiceOwnerChange()
+  // calls will happen first.
+  bus->ShutdownOnDBusThreadAndBlock();
+  EXPECT_TRUE(bus->shutdown_completed());
+  dbus_thread.Stop();
+}
+