arc: Change wakeup timers implementation to adapt to new API semantics

The Chrome OS power daemon's create timers D-Bus API now implicitly deletes
any old timers registered with a tag. Consequently, this change changes
the ARC++ client's implementation to not call a delete timers call
before calling the create D-Bus API.

BUG=chromium:916044
TEST=Run unit tests.

Change-Id: I2fb07b5d417d24f6518e369f5f82b18165962506
Reviewed-on: https://chromium-review.googlesource.com/c/1493819Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Abhishek Bhardwaj <abhishekbh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636580}

chromeos/dbus/fake_power_manager_client.cc

@@ -263,12 +263,9 @@ void FakePowerManagerClient::CreateArcTimers(
     return;
   }
 
-  // Check if client tag already exists. Return error iff it does.
-  if (base::ContainsKey(client_timer_ids_, tag)) {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback), std::vector<TimerId>()));
-    return;
-  }
+  // Just like the real implementation, delete any old timers associated with
+  // |tag|.
+  DeleteArcTimersInternal(tag);
 
   // First, ensure that there are no duplicate clocks in the arguments. Return
   // error if there are.
@@ -323,19 +320,7 @@ void FakePowerManagerClient::StartArcTimer(
 
 void FakePowerManagerClient::DeleteArcTimers(const std::string& tag,
                                              VoidDBusMethodCallback callback) {
-  // Retrieve all timer ids associated with |tag|. Delete all timers associated
-  // with these timer ids. Return true even if |tag| isn't found.
-  auto it = client_timer_ids_.find(tag);
-  if (it == client_timer_ids_.end()) {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback), true));
-    return;
-  }
-
-  for (auto timer_id : it->second)
-    timer_expiration_fds_.erase(timer_id);
-
-  client_timer_ids_.erase(it);
+  DeleteArcTimersInternal(tag);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), true));
 }
@@ -440,6 +425,19 @@ void FakePowerManagerClient::HandleSuspendReadiness() {
   --num_pending_suspend_readiness_callbacks_;
 }
 
+void FakePowerManagerClient::DeleteArcTimersInternal(const std::string& tag) {
+  // Retrieve all timer ids associated with |tag|. Delete all timers associated
+  // with these timer ids.
+  auto it = client_timer_ids_.find(tag);
+  if (it == client_timer_ids_.end())
+    return;
+
+  for (auto timer_id : it->second)
+    timer_expiration_fds_.erase(timer_id);
+
+  client_timer_ids_.erase(it);
+}
+
 void FakePowerManagerClient::SetPowerPolicyQuitClosure(
     base::OnceClosure quit_closure) {
   power_policy_quit_closure_ = std::move(quit_closure);

chromeos/dbus/fake_power_manager_client.h

@@ -191,6 +191,9 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakePowerManagerClient
   // Notifies |observers_| that |props_| has been updated.
   void NotifyObservers();
 
+  // Deletes all timers, if any, associated with |tag|.
+  void DeleteArcTimersInternal(const std::string& tag);
+
   base::ObserverList<Observer>::Unchecked observers_;
 
   // Last policy passed to SetPolicy().

components/arc/timer/arc_timer_bridge.cc

@@ -128,6 +128,7 @@ void ArcTimerBridge::CreateTimers(
   // Convert mojo arguments to D-Bus arguments required by powerd to create
   // timers.
   std::vector<std::pair<clockid_t, base::ScopedFD>> requests;
+  std::vector<clockid_t> clock_ids;
   for (auto& request : arc_timer_requests) {
     clockid_t clock_id = request->clock_id;
     base::ScopedFD expiration_fd =
@@ -138,11 +139,14 @@ void ArcTimerBridge::CreateTimers(
       return;
     }
     requests.emplace_back(clock_id, std::move(expiration_fd));
+    clock_ids.emplace_back(clock_id);
   }
-  chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->DeleteArcTimers(
-      kTag, base::BindOnce(&ArcTimerBridge::OnDeleteBeforeCreateArcTimers,
-                           weak_ptr_factory_.GetWeakPtr(), std::move(requests),
-                           std::move(callback)));
+
+  chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->CreateArcTimers(
+      kTag, std::move(requests),
+      base::BindOnce(&ArcTimerBridge::OnCreateArcTimers,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(clock_ids),
+                     std::move(callback)));
 }
 
 void ArcTimerBridge::StartTimer(clockid_t clock_id,
@@ -177,36 +181,15 @@ void ArcTimerBridge::OnDeleteArcTimers(bool result) {
   timer_ids_.clear();
 }
 
-void ArcTimerBridge::OnDeleteBeforeCreateArcTimers(
-    std::vector<std::pair<clockid_t, base::ScopedFD>>
-        create_arc_timers_requests,
-    CreateTimersCallback callback,
-    bool result) {
-  if (!result) {
-    LOG(ERROR) << "Delete timers before create failed";
-    std::move(callback).Run(mojom::ArcTimerResult::FAILURE);
-    return;
-  }
-
-  DVLOG(1) << "Delete before create timers succeeded";
-  // If the delete call succeeded then delete any timer ids stored and make a
-  // create timers call.
-  timer_ids_.clear();
-  std::vector<clockid_t> clock_ids;
-  for (const auto& request : create_arc_timers_requests)
-    clock_ids.emplace_back(request.first);
-
-  chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->CreateArcTimers(
-      kTag, std::move(create_arc_timers_requests),
-      base::BindOnce(&ArcTimerBridge::OnCreateArcTimers,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(clock_ids),
-                     std::move(callback)));
-}
-
 void ArcTimerBridge::OnCreateArcTimers(
     std::vector<clockid_t> clock_ids,
     CreateTimersCallback callback,
     base::Optional<std::vector<TimerId>> timer_ids) {
+  // Any old timers associated with the same tag are always cleared by the API
+  // regardless of the new timers being created successfully or not. Clear the
+  // cached timer ids in that case.
+  timer_ids_.clear();
+
   // The API returns a list of timer ids corresponding to each clock in
   // |clock_ids|.
   if (!timer_ids.has_value()) {

components/arc/timer/arc_timer_bridge.h

@@ -69,13 +69,6 @@ class ArcTimerBridge : public KeyedService,
   // Callback for (powerd API) call made in |DeleteArcTimers|.
   void OnDeleteArcTimers(bool result);
 
-  // Callback for delete timers (powerd API) call made in |CreateTimers|.
-  void OnDeleteBeforeCreateArcTimers(
-      std::vector<std::pair<clockid_t, base::ScopedFD>>
-          create_arc_timers_requests,
-      CreateTimersCallback callback,
-      bool result);
-
   // Callback for powerd's D-Bus API called in |CreateTimers|.
   void OnCreateArcTimers(std::vector<clockid_t> clock_ids,
                          CreateTimersCallback callback,

components/arc/timer/arc_timer_bridge_unittest.cc

@@ -285,8 +285,8 @@ TEST_F(ArcTimerTest, InvalidStartTimerArgsTest) {
 TEST_F(ArcTimerTest, CheckMultipleCreateTimersTest) {
   std::vector<clockid_t> clocks = {CLOCK_REALTIME_ALARM};
   EXPECT_TRUE(CreateTimers(clocks));
-  // The create implementation calls powerd's delete API before calling create.
-  // The second create should thus succeed.
+  // The power manager implicitly deletes old timers associated with a tag
+  // during a create call. Thus, consecutive create calls should succeed.
   EXPECT_TRUE(CreateTimers(clocks));
 }