ambient: Add retry logic for UpdateSettings

Changes:
1. When updating fails, we will use backoff policy until max retries.
2. However, we will update immediately if the update is called from
   Settings UI.
3. Caching the incoming updates when there is a ongoing updating and
   will update the cache when the ongoing update finishes.

Bug: b/152921891
Test: manual
Change-Id: I48f6c13f724696b75ee58ed1f7fc24cad7107a68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2378894Reviewed-by: Jimmy Gong <jimmyxgong@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804809}

ash/public/cpp/ambient/fake_ambient_backend_controller_impl.cc

@@ -103,9 +103,7 @@ void FakeAmbientBackendControllerImpl::GetSettings(
 void FakeAmbientBackendControllerImpl::UpdateSettings(
     const AmbientSettings& settings,
     UpdateSettingsCallback callback) {
-  // Pretend to respond asynchronously.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), /*success=*/true));
+  pending_update_callback_ = std::move(callback);
 }
 
 void FakeAmbientBackendControllerImpl::FetchSettingPreview(
@@ -145,4 +143,15 @@ void FakeAmbientBackendControllerImpl::SetPhotoRefreshInterval(
   NOTIMPLEMENTED();
 }
 
+void FakeAmbientBackendControllerImpl::ReplyUpdateSettings(bool success) {
+  if (!pending_update_callback_)
+    return;
+
+  std::move(pending_update_callback_).Run(success);
+}
+
+bool FakeAmbientBackendControllerImpl::IsUpdateSettingsPending() const {
+  return !pending_update_callback_.is_null();
+}
+
 }  // namespace ash

ash/public/cpp/ambient/fake_ambient_backend_controller_impl.h

@@ -7,6 +7,7 @@
 
 #include "ash/public/cpp/ambient/ambient_backend_controller.h"
 #include "ash/public/cpp/ash_public_export.h"
+#include "base/callback.h"
 
 namespace ash {
 
@@ -39,6 +40,15 @@ class ASH_PUBLIC_EXPORT FakeAmbientBackendControllerImpl
       int num_albums,
       OnSettingsAndAlbumsFetchedCallback callback) override;
   void SetPhotoRefreshInterval(base::TimeDelta interval) override;
+
+  // Simulate to reply the request of UpdateSettings() with |success|.
+  void ReplyUpdateSettings(bool success);
+
+  // Whether there is a pending UpdateSettings() request.
+  bool IsUpdateSettingsPending() const;
+
+ private:
+  UpdateSettingsCallback pending_update_callback_;
 };
 
 }  // namespace ash

chrome/browser/ui/webui/settings/chromeos/ambient_mode_handler.cc

@@ -18,6 +18,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/values.h"
 #include "chrome/grit/generated_resources.h"
 #include "chromeos/constants/chromeos_features.h"
@@ -43,6 +44,18 @@ constexpr int kBannerHeightPx = 160;
 constexpr char kCelsius[] = "celsius";
 constexpr char kFahrenheit[] = "fahrenheit";
 
+constexpr int kMaxRetries = 3;
+
+constexpr net::BackoffEntry::Policy kRetryBackoffPolicy = {
+    0,          // Number of initial errors to ignore.
+    500,        // Initial delay in ms.
+    2.0,        // Factor by which the waiting time will be multiplied.
+    0.2,        // Fuzzing percentage.
+    60 * 1000,  // Maximum delay in ms.
+    -1,         // Never discard the entry.
+    true,       // Use initial delay.
+};
+
 ash::AmbientModeTemperatureUnit ExtractTemperatureUnit(
     const base::ListValue* args) {
   auto temperature_unit = args->GetList()[0].GetString();
@@ -108,7 +121,8 @@ base::string16 GetAlbumDescription(const ash::PersonalAlbum& album) {
 
 }  // namespace
 
-AmbientModeHandler::AmbientModeHandler() = default;
+AmbientModeHandler::AmbientModeHandler()
+    : update_settings_retry_backoff_(&kRetryBackoffPolicy) {}
 
 AmbientModeHandler::~AmbientModeHandler() = default;
 
@@ -298,6 +312,14 @@ void AmbientModeHandler::SendAlbumPreview(
 }
 
 void AmbientModeHandler::UpdateSettings() {
+  if (is_updating_backend_) {
+    has_pending_updates_for_backend_ = true;
+    return;
+  }
+
+  has_pending_updates_for_backend_ = false;
+  is_updating_backend_ = true;
+
   DCHECK(settings_);
   ash::AmbientBackendController::Get()->UpdateSettings(
       *settings_, base::BindOnce(&AmbientModeHandler::OnUpdateSettings,
@@ -305,11 +327,26 @@ void AmbientModeHandler::UpdateSettings() {
 }
 
 void AmbientModeHandler::OnUpdateSettings(bool success) {
-  if (success)
-    return;
+  is_updating_backend_ = false;
+  update_settings_retry_backoff_.InformOfRequest(success);
+
+  if (success) {
+    update_settings_retries_ = 0;
+  } else {
+    ++update_settings_retries_;
+    if (update_settings_retries_ >= kMaxRetries)
+      return;
+  }
 
-  // TODO(b/152921891): Retry a small fixed number of times, then only retry
-  // when user confirms in the error message dialog.
+  if (has_pending_updates_for_backend_ || !success) {
+    const base::TimeDelta kDelay =
+        update_settings_retry_backoff_.GetTimeUntilRelease();
+    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&AmbientModeHandler::UpdateSettings,
+                       backend_weak_factory_.GetWeakPtr()),
+        kDelay);
+  }
 }
 
 void AmbientModeHandler::RequestSettingsAndAlbums(

chrome/browser/ui/webui/settings/chromeos/ambient_mode_handler.h

@@ -12,6 +12,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "chrome/browser/ui/webui/settings/settings_page_ui_handler.h"
+#include "net/base/backoff_entry.h"
 
 namespace ash {
 struct AmbientSettings;
@@ -115,6 +116,18 @@ class AmbientModeHandler : public ::settings::SettingsPageUIHandler {
 
   ash::PersonalAlbums personal_albums_;
 
+  // Whether the Settings updating is ongoing.
+  bool is_updating_backend_ = false;
+
+  // Whether there are pending updates.
+  bool has_pending_updates_for_backend_ = false;
+
+  // Backoff retries for UpdateSettings().
+  net::BackoffEntry update_settings_retry_backoff_;
+
+  // Number of attempts to update.
+  int update_settings_retries_ = 0;
+
   base::WeakPtrFactory<AmbientModeHandler> backend_weak_factory_{this};
   base::WeakPtrFactory<AmbientModeHandler> ui_update_weak_factory_{this};
 };

chrome/browser/ui/webui/settings/chromeos/ambient_mode_handler_unittest.cc

@@ -59,6 +59,35 @@ class AmbientModeHandlerTest : public testing::Test {
     handler_->HandleRequestAlbums(&args);
   }
 
+  void UpdateSettings() {
+    handler_->settings_ = ash::AmbientSettings();
+    handler_->UpdateSettings();
+  }
+
+  bool IsUpdateSettingsPendingAtHandler() const {
+    return handler_->is_updating_backend_;
+  }
+
+  bool HasPendingUpdatesForTesting() const {
+    return handler_->has_pending_updates_for_backend_;
+  }
+
+  base::TimeDelta GetUpdateSettingsDelay() {
+    return handler_->update_settings_retry_backoff_.GetTimeUntilRelease();
+  }
+
+  void FastForwardBy(base::TimeDelta time) {
+    task_environment_.FastForwardBy(time);
+  }
+
+  bool IsUpdateSettingsPendingAtBackend() const {
+    return fake_backend_controller_->IsUpdateSettingsPending();
+  }
+
+  void ReplyUpdateSettings(bool success) {
+    fake_backend_controller_->ReplyUpdateSettings(success);
+  }
+
   std::string BoolToString(bool x) { return x ? "true" : "false"; }
 
   void VerifySettingsSent(base::RunLoop* run_loop) {
@@ -150,9 +179,11 @@ class AmbientModeHandlerTest : public testing::Test {
   }
 
  private:
-  base::test::TaskEnvironment task_environment_;
+  base::test::TaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
   std::unique_ptr<content::TestWebUI> web_ui_;
-  std::unique_ptr<ash::AmbientBackendController> fake_backend_controller_;
+  std::unique_ptr<ash::FakeAmbientBackendControllerImpl>
+      fake_backend_controller_;
   std::unique_ptr<TestAmbientModeHandler> handler_;
 };
 
@@ -192,5 +223,85 @@ TEST_F(AmbientModeHandlerTest, TestSendAlbumsForArtGallery) {
   run_loop.Run();
 }
 
+TEST_F(AmbientModeHandlerTest, TestUpdateSettings) {
+  UpdateSettings();
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  ReplyUpdateSettings(/*success=*/true);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+}
+
+TEST_F(AmbientModeHandlerTest, TestUpdateSettingsTwice) {
+  UpdateSettings();
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  UpdateSettings();
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_TRUE(HasPendingUpdatesForTesting());
+
+  ReplyUpdateSettings(/*success=*/true);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_TRUE(HasPendingUpdatesForTesting());
+
+  FastForwardBy(GetUpdateSettingsDelay() * 1.5);
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+}
+
+TEST_F(AmbientModeHandlerTest, TestUpdateSettingsFailedWillRetry) {
+  UpdateSettings();
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  ReplyUpdateSettings(/*success=*/false);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  FastForwardBy(GetUpdateSettingsDelay() * 1.5);
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+}
+
+TEST_F(AmbientModeHandlerTest, TestUpdateSettingsSecondRetryWillBackoff) {
+  UpdateSettings();
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  ReplyUpdateSettings(/*success=*/false);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  base::TimeDelta delay1 = GetUpdateSettingsDelay();
+  FastForwardBy(delay1 * 1.5);
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  ReplyUpdateSettings(/*success=*/false);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  base::TimeDelta delay2 = GetUpdateSettingsDelay();
+  EXPECT_GT(delay2, delay1);
+
+  FastForwardBy(delay2 * 1.5);
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+}
+
 }  // namespace settings
 }  // namespace chromeos