ambient: only update settings when pref enabled

There was a case when |UpdateSettings| could be called the first time
a user opened Ambient mode settings, even when Ambient mode was
disabled. Now, only update settings if pref is enabled.

Additionally, issue an |UpdateSettings| call when the pref is
enabled. This ensures that Ambient settings are synced to server even
when a user does not edit temperature settings or photo albums.

BUG=b:177456397
TEST=unit_tests --gtest_filter=AmbientModeHandlerTest.*
TEST=open settings page, personalization, screen saver, toggle ambient

Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Change-Id: I924af998954927f0fc38746b34b8fce1ff641509
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2631305Reviewed-by: Jimmy Gong <jimmyxgong@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Commit-Queue: Jeffrey Young <cowmoo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845050}

ash/public/cpp/ambient/fake_ambient_backend_controller_impl.cc

@@ -119,6 +119,8 @@ void FakeAmbientBackendControllerImpl::GetSettings(
 void FakeAmbientBackendControllerImpl::UpdateSettings(
     const AmbientSettings& settings,
     UpdateSettingsCallback callback) {
+  // |show_weather| should always be set to true.
+  DCHECK(settings.show_weather);
   pending_update_callback_ = std::move(callback);
 }
 
@@ -162,13 +164,14 @@ FakeAmbientBackendControllerImpl::GetBackupPhotoUrls() const {
 }
 
 void FakeAmbientBackendControllerImpl::ReplyFetchSettingsAndAlbums(
-    bool success) {
+    bool success,
+    const base::Optional<AmbientSettings>& settings) {
   if (!pending_fetch_settings_albums_callback_)
     return;
 
   if (success) {
     std::move(pending_fetch_settings_albums_callback_)
-        .Run(CreateFakeSettings(), CreateFakeAlbums());
+        .Run(settings.value_or(CreateFakeSettings()), CreateFakeAlbums());
   } else {
     std::move(pending_fetch_settings_albums_callback_)
         .Run(/*settings=*/base::nullopt, PersonalAlbums());

ash/public/cpp/ambient/fake_ambient_backend_controller_impl.h

@@ -10,6 +10,7 @@
 #include "ash/public/cpp/ambient/ambient_backend_controller.h"
 #include "ash/public/cpp/ash_public_export.h"
 #include "base/callback.h"
+#include "base/optional.h"
 
 namespace ash {
 
@@ -46,7 +47,11 @@ class ASH_PUBLIC_EXPORT FakeAmbientBackendControllerImpl
   // 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);
+  // If |settings| contains a value, that will be used as the argument to
+  // the pending callback.
+  void ReplyFetchSettingsAndAlbums(
+      bool success,
+      const base::Optional<AmbientSettings>& settings = base::nullopt);
 
   // Whether there is a pending FetchSettingsAndAlbums() request.
   bool IsFetchSettingsAndAlbumsPending() const;

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

@@ -10,6 +10,7 @@
 
 #include "ash/public/cpp/ambient/ambient_backend_controller.h"
 #include "ash/public/cpp/ambient/ambient_metrics.h"
+#include "ash/public/cpp/ambient/ambient_prefs.h"
 #include "ash/public/cpp/ambient/common/ambient_settings.h"
 #include "ash/public/cpp/image_downloader.h"
 #include "base/barrier_closure.h"
@@ -25,6 +26,7 @@
 #include "base/values.h"
 #include "chrome/grit/generated_resources.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "components/prefs/pref_service.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -124,9 +126,10 @@ base::string16 GetAlbumDescription(const ash::PersonalAlbum& album) {
 
 }  // namespace
 
-AmbientModeHandler::AmbientModeHandler()
+AmbientModeHandler::AmbientModeHandler(PrefService* pref_service)
     : fetch_settings_retry_backoff_(&kRetryBackoffPolicy),
-      update_settings_retry_backoff_(&kRetryBackoffPolicy) {}
+      update_settings_retry_backoff_(&kRetryBackoffPolicy),
+      pref_service_(pref_service) {}
 
 AmbientModeHandler::~AmbientModeHandler() = default;
 
@@ -152,9 +155,30 @@ void AmbientModeHandler::RegisterMessages() {
                           base::Unretained(this)));
 }
 
+void AmbientModeHandler::OnJavascriptAllowed() {
+  pref_change_registrar_.Init(pref_service_);
+  pref_change_registrar_.Add(
+      ash::ambient::prefs::kAmbientModeEnabled,
+      base::BindRepeating(&AmbientModeHandler::OnEnabledPrefChanged,
+                          base::Unretained(this)));
+}
+
 void AmbientModeHandler::OnJavascriptDisallowed() {
   backend_weak_factory_.InvalidateWeakPtrs();
   ui_update_weak_factory_.InvalidateWeakPtrs();
+  pref_change_registrar_.RemoveAll();
+}
+
+bool AmbientModeHandler::IsAmbientModeEnabled() {
+  return pref_service_->GetBoolean(ash::ambient::prefs::kAmbientModeEnabled);
+}
+
+void AmbientModeHandler::OnEnabledPrefChanged() {
+  // Call |UpdateSettings| when Ambient mode is enabled to make sure that
+  // settings are properly synced to the server even if the user never touches
+  // the other controls.
+  if (settings_ && IsAmbientModeEnabled())
+    UpdateSettings();
 }
 
 void AmbientModeHandler::HandleRequestSettings(const base::ListValue* args) {
@@ -347,6 +371,10 @@ void AmbientModeHandler::SendRecentHighlightsPreviews() {
 }
 
 void AmbientModeHandler::UpdateSettings() {
+  DCHECK(IsAmbientModeEnabled())
+      << "Ambient mode must be enabled to update settings";
+  DCHECK(settings_);
+
   // Prevent fetch settings callback changing |settings_| and |personal_albums_|
   // while updating.
   ui_update_weak_factory_.InvalidateWeakPtrs();
@@ -359,7 +387,10 @@ void AmbientModeHandler::UpdateSettings() {
   has_pending_updates_for_backend_ = false;
   is_updating_backend_ = true;
 
-  DCHECK(settings_);
+  // Explicitly set show_weather to true to force server to respond with
+  // weather information. See: b/158630188.
+  settings_->show_weather = true;
+
   settings_sent_for_update_ = settings_;
   ash::AmbientBackendController::Get()->UpdateSettings(
       *settings_, base::BindOnce(&AmbientModeHandler::OnUpdateSettings,
@@ -475,14 +506,9 @@ void AmbientModeHandler::OnSettingsAndAlbumsFetched(
     SendTopicSource();
     SendTemperatureUnit();
 
-    // Explicitly enable the weather settings if necessary to make sure we
-    // can always get weather info in the response. Leaving this settings as
-    // default could result in unpredictable behavior (b/158630188). Note that
-    // right now the weather info is designed to be always shown on ambient
-    // screen, so we don't expose an option in ambient Settings for users to
-    // switch it off.
-    if (!settings_->show_weather) {
-      settings_->show_weather = true;
+    // If weather info is disabled, call |UpdateSettings| immediately to force
+    // it to true.
+    if (!settings_->show_weather && IsAmbientModeEnabled()) {
       UpdateSettings();
     }
     return;

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 "components/prefs/pref_change_registrar.h"
 #include "net/base/backoff_entry.h"
 
 namespace ash {
@@ -26,6 +27,8 @@ namespace gfx {
 class ImageSkia;
 }  // namespace gfx
 
+class PrefService;
+
 namespace chromeos {
 namespace settings {
 
@@ -33,19 +36,23 @@ namespace settings {
 // photo frame and other related functionalities.
 class AmbientModeHandler : public ::settings::SettingsPageUIHandler {
  public:
-  AmbientModeHandler();
+  explicit AmbientModeHandler(PrefService* pref_service);
   AmbientModeHandler(const AmbientModeHandler&) = delete;
   AmbientModeHandler& operator=(const AmbientModeHandler&) = delete;
   ~AmbientModeHandler() override;
 
   // settings::SettingsPageUIHandler:
   void RegisterMessages() override;
-  void OnJavascriptAllowed() override {}
+  void OnJavascriptAllowed() override;
   void OnJavascriptDisallowed() override;
 
  private:
   friend class AmbientModeHandlerTest;
 
+  bool IsAmbientModeEnabled();
+
+  void OnEnabledPrefChanged();
+
   // WebUI call to request topic source and temperature unit related data.
   void HandleRequestSettings(const base::ListValue* args);
 
@@ -162,6 +169,10 @@ class AmbientModeHandler : public ::settings::SettingsPageUIHandler {
 
   std::vector<gfx::ImageSkia> recent_highlights_preview_images_;
 
+  PrefService* pref_service_;
+
+  PrefChangeRegistrar pref_change_registrar_;
+
   base::WeakPtrFactory<AmbientModeHandler> backend_weak_factory_{this};
   base::WeakPtrFactory<AmbientModeHandler> ui_update_weak_factory_{this};
   base::WeakPtrFactory<AmbientModeHandler>

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

@@ -6,11 +6,14 @@
 
 #include <memory>
 
+#include "ash/public/cpp/ambient/ambient_prefs.h"
 #include "ash/public/cpp/ambient/common/ambient_settings.h"
 #include "ash/public/cpp/ambient/fake_ambient_backend_controller_impl.h"
 #include "ash/public/cpp/test/test_image_downloader.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
 #include "content/public/test/test_web_ui.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -24,7 +27,8 @@ const char kWebCallbackFunctionName[] = "cr.webUIListenerCallback";
 
 class TestAmbientModeHandler : public AmbientModeHandler {
  public:
-  TestAmbientModeHandler() = default;
+  explicit TestAmbientModeHandler(PrefService* pref_service)
+      : AmbientModeHandler(pref_service) {}
   ~TestAmbientModeHandler() override = default;
 
   // Make public for testing.
@@ -42,7 +46,13 @@ class AmbientModeHandlerTest : public testing::Test {
 
   void SetUp() override {
     web_ui_ = std::make_unique<content::TestWebUI>();
-    handler_ = std::make_unique<TestAmbientModeHandler>();
+    test_pref_service_ = std::make_unique<TestingPrefServiceSimple>();
+
+    test_pref_service_->registry()->RegisterBooleanPref(
+        ash::ambient::prefs::kAmbientModeEnabled, true);
+
+    handler_ =
+        std::make_unique<TestAmbientModeHandler>(test_pref_service_.get());
     handler_->set_web_ui(web_ui_.get());
     handler_->RegisterMessages();
     handler_->AllowJavascript();
@@ -51,12 +61,23 @@ class AmbientModeHandlerTest : public testing::Test {
     image_downloader_ = std::make_unique<ash::TestImageDownloader>();
   }
 
+  void TearDown() override { handler_->DisallowJavascript(); }
+
   content::TestWebUI* web_ui() { return web_ui_.get(); }
 
   const base::HistogramTester& histogram_tester() const {
     return histogram_tester_;
   }
 
+  base::Optional<ash::AmbientSettings>& settings() {
+    return handler_->settings_;
+  }
+
+  void SetEnabledPref(bool enabled) {
+    test_pref_service_->SetBoolean(ash::ambient::prefs::kAmbientModeEnabled,
+                                   enabled);
+  }
+
   void SetTopicSource(ash::AmbientModeTopicSource topic_source) {
     if (!handler_->settings_)
       handler_->settings_ = ash::AmbientSettings();
@@ -129,8 +150,11 @@ class AmbientModeHandlerTest : public testing::Test {
     return fake_backend_controller_->IsFetchSettingsAndAlbumsPending();
   }
 
-  void ReplyFetchSettingsAndAlbums(bool success) {
-    fake_backend_controller_->ReplyFetchSettingsAndAlbums(success);
+  void ReplyFetchSettingsAndAlbums(
+      bool success,
+      base::Optional<ash::AmbientSettings> settings = base::nullopt) {
+    fake_backend_controller_->ReplyFetchSettingsAndAlbums(success,
+                                                          std::move(settings));
   }
 
   bool IsUpdateSettingsPendingAtBackend() const {
@@ -234,6 +258,7 @@ class AmbientModeHandlerTest : public testing::Test {
   std::unique_ptr<ash::TestImageDownloader> image_downloader_;
   std::unique_ptr<TestAmbientModeHandler> handler_;
   base::HistogramTester histogram_tester_;
+  std::unique_ptr<TestingPrefServiceSimple> test_pref_service_;
 };
 
 TEST_F(AmbientModeHandlerTest, TestSendTemperatureUnitAndTopicSource) {
@@ -781,5 +806,49 @@ TEST_F(AmbientModeHandlerTest, TestSameTemperatureUnitSkipsUpdate) {
   EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
 }
 
+TEST_F(AmbientModeHandlerTest, TestEnabledPrefChangeUpdatesSettings) {
+  // Simulate initial page request.
+  RequestSettings();
+  ReplyFetchSettingsAndAlbums(/*success=*/true);
+
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+
+  // Should not trigger |UpdateSettings|.
+  SetEnabledPref(/*enabled=*/false);
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+
+  // Settings this to true should trigger |UpdateSettings|.
+  SetEnabledPref(/*enabled=*/true);
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+}
+
+TEST_F(AmbientModeHandlerTest, TestWeatherFalseTriggersUpdateSettings) {
+  ash::AmbientSettings weather_off_settings;
+  weather_off_settings.show_weather = false;
+
+  // Simulate initial page request with weather settings false. Because Ambient
+  // mode pref is enabled and |settings.show_weather| is false, this should
+  // trigger a call to |UpdateSettings| that sets |settings.show_weather| to
+  // true.
+  RequestSettings();
+  ReplyFetchSettingsAndAlbums(/*success=*/true, weather_off_settings);
+
+  // A call to |UpdateSettings| should have happened.
+  EXPECT_TRUE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_TRUE(IsUpdateSettingsPendingAtBackend());
+
+  ReplyUpdateSettings(/*success=*/true);
+
+  EXPECT_FALSE(IsUpdateSettingsPendingAtHandler());
+  EXPECT_FALSE(IsUpdateSettingsPendingAtBackend());
+
+  // |settings.show_weather| should now be true after the successful settings
+  // update.
+  EXPECT_TRUE(settings()->show_weather);
+}
+
 }  // namespace settings
 }  // namespace chromeos

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

@@ -260,7 +260,8 @@ void PersonalizationSection::AddHandlers(content::WebUI* web_ui) {
   if (!profile()->IsGuestSession() &&
       chromeos::features::IsAmbientModeEnabled()) {
     web_ui->AddMessageHandler(
-        std::make_unique<chromeos::settings::AmbientModeHandler>());
+        std::make_unique<chromeos::settings::AmbientModeHandler>(
+            pref_service_));
   }
 }