ambient: Backoff get settings retries

Changes:
1. When get settings fails, we will use backoff policy until max
   retries.
2. However, we will request settings immediately if the request is
   called from Settings UI.

Bug: b/152921891
Test: added new tests
Change-Id: Ied3f02b7dcb24d6fb95b528bac932987b9ee3ee1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410299
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Jimmy Gong <jimmyxgong@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808613}

ash/public/cpp/ambient/fake_ambient_backend_controller_impl.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "ash/public/cpp/ambient/common/ambient_settings.h"
 #include "base/callback.h"
 #include "base/optional.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -141,10 +142,7 @@ void FakeAmbientBackendControllerImpl::FetchSettingsAndAlbums(
     int banner_height,
     int num_albums,
     OnSettingsAndAlbumsFetchedCallback callback) {
-  // Pretend to respond asynchronously.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), CreateFakeSettings(),
-                                CreateFakeAlbums()));
+  pending_fetch_settings_albums_callback_ = std::move(callback);
 }
 
 void FakeAmbientBackendControllerImpl::SetPhotoRefreshInterval(
@@ -152,6 +150,24 @@ void FakeAmbientBackendControllerImpl::SetPhotoRefreshInterval(
   NOTIMPLEMENTED();
 }
 
+void FakeAmbientBackendControllerImpl::ReplyFetchSettingsAndAlbums(
+    bool success) {
+  if (!pending_fetch_settings_albums_callback_)
+    return;
+
+  if (success) {
+    std::move(pending_fetch_settings_albums_callback_)
+        .Run(CreateFakeSettings(), CreateFakeAlbums());
+  } else {
+    std::move(pending_fetch_settings_albums_callback_)
+        .Run(/*settings=*/base::nullopt, PersonalAlbums());
+  }
+}
+
+bool FakeAmbientBackendControllerImpl::IsFetchSettingsAndAlbumsPending() const {
+  return !pending_fetch_settings_albums_callback_.is_null();
+}
+
 void FakeAmbientBackendControllerImpl::ReplyUpdateSettings(bool success) {
   if (!pending_update_callback_)
     return;

ash/public/cpp/ambient/fake_ambient_backend_controller_impl.h

@@ -41,6 +41,14 @@ class ASH_PUBLIC_EXPORT FakeAmbientBackendControllerImpl
       OnSettingsAndAlbumsFetchedCallback callback) override;
   void SetPhotoRefreshInterval(base::TimeDelta interval) override;
 
+  // Simulate to reply the request of FetchSettingsAndAlbums().
+  // If |success| is true, will return fake data.
+  // If |success| is false, will return null |settings| data.
+  void ReplyFetchSettingsAndAlbums(bool success);
+
+  // Whether there is a pending FetchSettingsAndAlbums() request.
+  bool IsFetchSettingsAndAlbumsPending() const;
+
   // Simulate to reply the request of UpdateSettings() with |success|.
   void ReplyUpdateSettings(bool success);
 
@@ -48,6 +56,8 @@ class ASH_PUBLIC_EXPORT FakeAmbientBackendControllerImpl
   bool IsUpdateSettingsPending() const;
 
  private:
+  OnSettingsAndAlbumsFetchedCallback pending_fetch_settings_albums_callback_;
+
   UpdateSettingsCallback pending_update_callback_;
 };
 

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

@@ -15,6 +15,7 @@
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/optional.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
@@ -122,7 +123,8 @@ base::string16 GetAlbumDescription(const ash::PersonalAlbum& album) {
 }  // namespace
 
 AmbientModeHandler::AmbientModeHandler()
-    : update_settings_retry_backoff_(&kRetryBackoffPolicy) {}
+    : fetch_settings_retry_backoff_(&kRetryBackoffPolicy),
+      update_settings_retry_backoff_(&kRetryBackoffPolicy) {}
 
 AmbientModeHandler::~AmbientModeHandler() = default;
 
@@ -163,10 +165,7 @@ void AmbientModeHandler::HandleRequestSettings(const base::ListValue* args) {
   // since the last time requesting the data. Abort any request in progress to
   // avoid unnecessary updating invisible subpage.
   ui_update_weak_factory_.InvalidateWeakPtrs();
-  RequestSettingsAndAlbums(
-      base::BindOnce(&AmbientModeHandler::OnSettingsAndAlbumsFetched,
-                     ui_update_weak_factory_.GetWeakPtr(),
-                     /*topic_source=*/base::nullopt));
+  RequestSettingsAndAlbums(/*topic_source=*/base::nullopt);
 }
 
 void AmbientModeHandler::HandleRequestAlbums(const base::ListValue* args) {
@@ -179,9 +178,7 @@ void AmbientModeHandler::HandleRequestAlbums(const base::ListValue* args) {
   // Photos to Art gallery, since the last time requesting the data.
   // Abort any request in progress to avoid updating incorrect contents.
   ui_update_weak_factory_.InvalidateWeakPtrs();
-  RequestSettingsAndAlbums(base::BindOnce(
-      &AmbientModeHandler::OnSettingsAndAlbumsFetched,
-      ui_update_weak_factory_.GetWeakPtr(), ExtractTopicSource(args)));
+  RequestSettingsAndAlbums(ExtractTopicSource(args));
 }
 
 void AmbientModeHandler::HandleSetSelectedTemperatureUnit(
@@ -333,13 +330,12 @@ void AmbientModeHandler::UpdateSettings() {
 
 void AmbientModeHandler::OnUpdateSettings(bool success) {
   is_updating_backend_ = false;
-  update_settings_retry_backoff_.InformOfRequest(success);
 
   if (success) {
-    update_settings_retries_ = 0;
+    update_settings_retry_backoff_.Reset();
   } else {
-    ++update_settings_retries_;
-    if (update_settings_retries_ >= kMaxRetries)
+    update_settings_retry_backoff_.InformOfRequest(/*succeeded=*/false);
+    if (update_settings_retry_backoff_.failure_count() > kMaxRetries)
       return;
   }
 
@@ -355,23 +351,36 @@ void AmbientModeHandler::OnUpdateSettings(bool success) {
 }
 
 void AmbientModeHandler::RequestSettingsAndAlbums(
-    ash::AmbientBackendController::OnSettingsAndAlbumsFetchedCallback
-        callback) {
+    base::Optional<ash::AmbientModeTopicSource> topic_source) {
   // TODO(b/161044021): Add a helper function to get all the albums. Currently
   // only load 100 latest modified albums.
   ash::AmbientBackendController::Get()->FetchSettingsAndAlbums(
-      kBannerWidthPx, kBannerHeightPx, /*num_albums=*/100, std::move(callback));
+      kBannerWidthPx, kBannerHeightPx, /*num_albums=*/100,
+      base::BindOnce(&AmbientModeHandler::OnSettingsAndAlbumsFetched,
+                     ui_update_weak_factory_.GetWeakPtr(), topic_source));
 }
 
 void AmbientModeHandler::OnSettingsAndAlbumsFetched(
     base::Optional<ash::AmbientModeTopicSource> topic_source,
     const base::Optional<ash::AmbientSettings>& settings,
     ash::PersonalAlbums personal_albums) {
-  // TODO(b/152921891): Retry a small fixed number of times, then only retry
-  // when user confirms in the error message dialog.
-  if (!settings)
+  // |settings| value implies success.
+  if (!settings) {
+    fetch_settings_retry_backoff_.InformOfRequest(/*succeeded=*/false);
+    if (fetch_settings_retry_backoff_.failure_count() > kMaxRetries)
+      return;
+
+    const base::TimeDelta kDelay =
+        fetch_settings_retry_backoff_.GetTimeUntilRelease();
+    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&AmbientModeHandler::RequestSettingsAndAlbums,
+                       ui_update_weak_factory_.GetWeakPtr(), topic_source),
+        kDelay);
     return;
+  }
 
+  fetch_settings_retry_backoff_.Reset();
   settings_ = settings;
   personal_albums_ = std::move(personal_albums);
   SyncSettingsAndAlbums();

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

@@ -81,13 +81,14 @@ class AmbientModeHandler : public ::settings::SettingsPageUIHandler {
   // Called when the settings is updated.
   void OnUpdateSettings(bool success);
 
+  // Will be called from ambientMode/photos subpage and ambientMode subpage.
+  // |topic_source| is used to request the albums in that source and identify
+  // the callers:
+  //   1. |kGooglePhotos|: ambientMode/photos?topicSource=0
+  //   2. |kArtGallery|:   ambientMode/photos?topicSource=1
+  //   3. base::nullopt:   ambientMode/
   void RequestSettingsAndAlbums(
-      ash::AmbientBackendController::OnSettingsAndAlbumsFetchedCallback
-          callback);
-
-  // |topic_source| is what the |settings_| and |personal_albums_| were
-  // requested for the ambientMode/photos subpage. It is base::nullopt if they
-  // were requested by the ambientMode subpage.
+      base::Optional<ash::AmbientModeTopicSource> topic_source);
   void OnSettingsAndAlbumsFetched(
       base::Optional<ash::AmbientModeTopicSource> topic_source,
       const base::Optional<ash::AmbientSettings>& settings,
@@ -116,6 +117,9 @@ class AmbientModeHandler : public ::settings::SettingsPageUIHandler {
 
   ash::PersonalAlbums personal_albums_;
 
+  // Backoff retries for RequestSettingsAndAlbums().
+  net::BackoffEntry fetch_settings_retry_backoff_;
+
   // Whether the Settings updating is ongoing.
   bool is_updating_backend_ = false;
 
@@ -125,9 +129,6 @@ class AmbientModeHandler : public ::settings::SettingsPageUIHandler {
   // 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

@@ -61,6 +61,10 @@ class AmbientModeHandlerTest : public testing::Test {
     handler_->HandleRequestAlbums(&args);
   }
 
+  void FetchSettings() {
+    handler_->RequestSettingsAndAlbums(/*topic_source=*/base::nullopt);
+  }
+
   void UpdateSettings() {
     handler_->settings_ = ash::AmbientSettings();
     handler_->UpdateSettings();
@@ -74,6 +78,10 @@ class AmbientModeHandlerTest : public testing::Test {
     return handler_->has_pending_updates_for_backend_;
   }
 
+  base::TimeDelta GetFetchSettingsDelay() {
+    return handler_->fetch_settings_retry_backoff_.GetTimeUntilRelease();
+  }
+
   base::TimeDelta GetUpdateSettingsDelay() {
     return handler_->update_settings_retry_backoff_.GetTimeUntilRelease();
   }
@@ -82,6 +90,14 @@ class AmbientModeHandlerTest : public testing::Test {
     task_environment_.FastForwardBy(time);
   }
 
+  bool IsFetchSettingsPendingAtBackend() const {
+    return fake_backend_controller_->IsFetchSettingsAndAlbumsPending();
+  }
+
+  void ReplyFetchSettingsAndAlbums(bool success) {
+    fake_backend_controller_->ReplyFetchSettingsAndAlbums(success);
+  }
+
   bool IsUpdateSettingsPendingAtBackend() const {
     return fake_backend_controller_->IsUpdateSettingsPending();
   }
@@ -92,7 +108,7 @@ class AmbientModeHandlerTest : public testing::Test {
 
   std::string BoolToString(bool x) { return x ? "true" : "false"; }
 
-  void VerifySettingsSent(base::RunLoop* run_loop) {
+  void VerifySettingsSent() {
     EXPECT_EQ(2U, web_ui_->call_data().size());
 
     // The call is structured such that the function name is the "web callback"
@@ -118,12 +134,9 @@ class AmbientModeHandlerTest : public testing::Test {
               temperature_unit_call_data.arg1()->GetString());
     // In FakeAmbientBackendControllerImpl, the |temperature_unit| is kCelsius.
     EXPECT_EQ("celsius", temperature_unit_call_data.arg2()->GetString());
-
-    run_loop->Quit();
   }
 
-  void VerifyAlbumsSent(ash::AmbientModeTopicSource topic_source,
-                        base::RunLoop* run_loop) {
+  void VerifyAlbumsSent(ash::AmbientModeTopicSource topic_source) {
     // Art gallery has an extra call to update the topic source to Art gallery.
     std::vector<std::unique_ptr<content::TestWebUI::CallData>>::size_type call_size =
         topic_source == ash::AmbientModeTopicSource::kGooglePhotos ? 1U : 2U;
@@ -177,7 +190,6 @@ class AmbientModeHandlerTest : public testing::Test {
       EXPECT_EQ(false, album1->FindKey("checked")->GetBool());
       EXPECT_EQ("art1", album1->FindKey("title")->GetString());
     }
-    run_loop->Quit();
   }
 
  private:
@@ -192,38 +204,98 @@ class AmbientModeHandlerTest : public testing::Test {
 
 TEST_F(AmbientModeHandlerTest, TestSendTemperatureUnitAndTopicSource) {
   RequestSettings();
-
-  base::RunLoop run_loop;
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&AmbientModeHandlerTest::VerifySettingsSent,
-                                base::Unretained(this), &run_loop));
-  run_loop.Run();
+  ReplyFetchSettingsAndAlbums(/*success=*/true);
+  VerifySettingsSent();
 }
 
 TEST_F(AmbientModeHandlerTest, TestSendAlbumsForGooglePhotos) {
   ash::AmbientModeTopicSource topic_source =
       ash::AmbientModeTopicSource::kGooglePhotos;
   RequestAlbums(topic_source);
-
-  base::RunLoop run_loop;
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&AmbientModeHandlerTest::VerifyAlbumsSent,
-                     base::Unretained(this), topic_source, &run_loop));
-  run_loop.Run();
+  ReplyFetchSettingsAndAlbums(/*success=*/true);
+  VerifyAlbumsSent(topic_source);
 }
 
 TEST_F(AmbientModeHandlerTest, TestSendAlbumsForArtGallery) {
   ash::AmbientModeTopicSource topic_source =
       ash::AmbientModeTopicSource::kArtGallery;
   RequestAlbums(topic_source);
+  ReplyFetchSettingsAndAlbums(/*success=*/true);
+  VerifyAlbumsSent(topic_source);
+}
+
+TEST_F(AmbientModeHandlerTest, TestFetchSettings) {
+  FetchSettings();
+  EXPECT_TRUE(IsFetchSettingsPendingAtBackend());
+
+  ReplyFetchSettingsAndAlbums(/*success=*/true);
+  EXPECT_FALSE(IsFetchSettingsPendingAtBackend());
+}
+
+TEST_F(AmbientModeHandlerTest, TestFetchSettingsFailedWillRetry) {
+  FetchSettings();
+  EXPECT_TRUE(IsFetchSettingsPendingAtBackend());
+
+  ReplyFetchSettingsAndAlbums(/*success=*/false);
+  EXPECT_FALSE(IsFetchSettingsPendingAtBackend());
+
+  FastForwardBy(GetFetchSettingsDelay() * 1.5);
+  EXPECT_TRUE(IsFetchSettingsPendingAtBackend());
+}
+
+TEST_F(AmbientModeHandlerTest, TestFetchSettingsSecondRetryWillBackoff) {
+  FetchSettings();
+  EXPECT_TRUE(IsFetchSettingsPendingAtBackend());
+
+  ReplyFetchSettingsAndAlbums(/*success=*/false);
+  EXPECT_FALSE(IsFetchSettingsPendingAtBackend());
 
-  base::RunLoop run_loop;
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&AmbientModeHandlerTest::VerifyAlbumsSent,
-                     base::Unretained(this), topic_source, &run_loop));
-  run_loop.Run();
+  base::TimeDelta delay1 = GetFetchSettingsDelay();
+  FastForwardBy(delay1 * 1.5);
+  EXPECT_TRUE(IsFetchSettingsPendingAtBackend());
+
+  ReplyFetchSettingsAndAlbums(/*success=*/false);
+  EXPECT_FALSE(IsFetchSettingsPendingAtBackend());
+
+  base::TimeDelta delay2 = GetFetchSettingsDelay();
+  EXPECT_GT(delay2, delay1);
+
+  FastForwardBy(delay2 * 1.5);
+  EXPECT_TRUE(IsFetchSettingsPendingAtBackend());
+}
+
+TEST_F(AmbientModeHandlerTest,
+       TestFetchSettingsWillNotRetryMoreThanThreeTimes) {
+  FetchSettings();
+  EXPECT_TRUE(IsFetchSettingsPendingAtBackend());
+
+  ReplyFetchSettingsAndAlbums(/*success=*/false);
+  EXPECT_FALSE(IsFetchSettingsPendingAtBackend());
+
+  // 1st retry.
+  FastForwardBy(GetFetchSettingsDelay() * 1.5);
+  EXPECT_TRUE(IsFetchSettingsPendingAtBackend());
+
+  ReplyFetchSettingsAndAlbums(/*success=*/false);
+  EXPECT_FALSE(IsFetchSettingsPendingAtBackend());
+
+  // 2nd retry.
+  FastForwardBy(GetFetchSettingsDelay() * 1.5);
+  EXPECT_TRUE(IsFetchSettingsPendingAtBackend());
+
+  ReplyFetchSettingsAndAlbums(/*success=*/false);
+  EXPECT_FALSE(IsFetchSettingsPendingAtBackend());
+
+  // 3rd retry.
+  FastForwardBy(GetFetchSettingsDelay() * 1.5);
+  EXPECT_TRUE(IsFetchSettingsPendingAtBackend());
+
+  ReplyFetchSettingsAndAlbums(/*success=*/false);
+  EXPECT_FALSE(IsFetchSettingsPendingAtBackend());
+
+  // Will not retry.
+  FastForwardBy(GetFetchSettingsDelay() * 1.5);
+  EXPECT_FALSE(IsFetchSettingsPendingAtBackend());
 }
 
 TEST_F(AmbientModeHandlerTest, TestUpdateSettings) {
@@ -306,5 +378,57 @@ TEST_F(AmbientModeHandlerTest, TestUpdateSettingsSecondRetryWillBackoff) {
   EXPECT_FALSE(HasPendingUpdatesForTesting());
 }
 
+TEST_F(AmbientModeHandlerTest,
+       TestUpdateSettingsWillNotRetryMoreThanThreeTimes) {
+  UpdateSettings();
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  ReplyUpdateSettings(/*success=*/false);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  // 1st retry.
+  FastForwardBy(GetUpdateSettingsDelay() * 1.5);
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  ReplyUpdateSettings(/*success=*/false);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  // 2nd retry.
+  FastForwardBy(GetUpdateSettingsDelay() * 1.5);
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  ReplyUpdateSettings(/*success=*/false);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  // 3rd retry.
+  FastForwardBy(GetUpdateSettingsDelay() * 1.5);
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  ReplyUpdateSettings(/*success=*/false);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+
+  // Will not retry.
+  FastForwardBy(GetUpdateSettingsDelay() * 1.5);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(HasPendingUpdatesForTesting());
+}
+
 }  // namespace settings
 }  // namespace chromeos