ambient: fix settings being overridden upon service enabled.

This CL fixes the settings issue introduced by the CL below:
https://chromium-review.googlesource.com/c/chromium/src/+/2363914/comment/81a70101_1ab41885/

We now move the logic of updating weather settings inside the
|OnSettingsAndAlbumsFetched|, where the previous user settings stored on
the server side have been retrieved. We then check the |show_weather|
field and update the weather settings if necessary.

Bug: b/158630188
Change-Id: Ifcd66699ca483a88d119afc5182539e508639c82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2389416Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Meilin Wang <meilinw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812601}

ash/ambient/ambient_controller.cc

@@ -282,16 +282,6 @@ void AmbientController::OnAutoShowTimeOut() {
   ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kShown);
 }
 
-void AmbientController::OnActiveUserPrefServiceChanged(
-    PrefService* pref_service) {
-  // Only registers for primary prefs as Ambient Mode is only available for
-  // primary profile.
-  PrefService* primary_user_prefs =
-      Shell::Get()->session_controller()->GetPrimaryUserPrefService();
-  if (primary_user_prefs == pref_service)
-    RegisterPrefChanges(primary_user_prefs);
-}
-
 void AmbientController::OnLockStateChanged(bool locked) {
   if (!IsAmbientModeEnabled()) {
     VLOG(1) << "Ambient mode is not allowed.";
@@ -565,31 +555,6 @@ void AmbientController::StopRefreshingImages() {
   ambient_photo_controller_.StopScreenUpdate();
 }
 
-void AmbientController::RegisterPrefChanges(PrefService* pref_service) {
-  DCHECK(pref_service);
-
-  // Registers preference changes.
-  pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
-  pref_change_registrar_->Init(pref_service);
-  pref_change_registrar_->Add(
-      ash::ambient::prefs::kAmbientModeEnabled,
-      base::BindRepeating(&AmbientController::OnEnabledStateChanged,
-                          weak_ptr_factory_.GetWeakPtr()));
-}
-
-void AmbientController::OnEnabledStateChanged() {
-  auto enabled = pref_change_registrar_->prefs()->GetBoolean(
-      ash::ambient::prefs::kAmbientModeEnabled);
-
-  // Initiates settings for ambient service upon enabled.
-  if (enabled) {
-    ambient_backend_controller_->InitSettings(base::BindOnce([](bool success) {
-      if (!success)
-        LOG(ERROR) << "Failed to initiate settings for ambient service.";
-    }));
-  }
-}
-
 void AmbientController::set_backend_controller_for_testing(
     std::unique_ptr<AmbientBackendController> backend_controller) {
   ambient_backend_controller_ = std::move(backend_controller);

ash/ambient/ambient_controller.h

@@ -28,9 +28,7 @@
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 
-class PrefChangeRegistrar;
 class PrefRegistrySimple;
-class PrefService;
 
 namespace ash {
 
@@ -59,7 +57,6 @@ class ASH_EXPORT AmbientController
   void OnAmbientUiVisibilityChanged(AmbientUiVisibility visibility) override;
 
   // SessionObserver:
-  void OnActiveUserPrefServiceChanged(PrefService* pref_service) override;
   void OnLockStateChanged(bool locked) override;
 
   // PowerStatus::Observer:
@@ -141,8 +138,6 @@ class ASH_EXPORT AmbientController
 
   void CloseWidget(bool immediately);
 
-  void RegisterPrefChanges(PrefService* pref_service);
-
   // Invoked when the |kAmbientModeEnabled| pref state changed.
   void OnEnabledStateChanged();
 

ash/ambient/backdrop/ambient_backend_controller_impl.cc

@@ -323,20 +323,6 @@ void AmbientBackendControllerImpl::FetchScreenUpdateInfo(
       weak_factory_.GetWeakPtr(), num_topics, std::move(callback)));
 }
 
-void AmbientBackendControllerImpl::InitSettings(
-    UpdateSettingsCallback callback) {
-  AmbientSettings settings;
-
-  // Inits weather-related settings. 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, and we don't expose an option in ambient Settings for
-  // users to switch it off.
-  settings.show_weather = true;
-
-  UpdateSettings(settings, std::move(callback));
-}
-
 void AmbientBackendControllerImpl::GetSettings(GetSettingsCallback callback) {
   Shell::Get()->ambient_controller()->RequestAccessToken(
       base::BindOnce(&AmbientBackendControllerImpl::StartToGetSettings,

ash/ambient/backdrop/ambient_backend_controller_impl.h

@@ -30,7 +30,6 @@ class AmbientBackendControllerImpl : public AmbientBackendController {
   void FetchScreenUpdateInfo(
       int num_topics,
       OnScreenUpdateInfoFetchedCallback callback) override;
-  void InitSettings(UpdateSettingsCallback callback) override;
   void GetSettings(GetSettingsCallback callback) override;
   void UpdateSettings(const AmbientSettings& settings,
                       UpdateSettingsCallback callback) override;

ash/public/cpp/ambient/ambient_backend_controller.h

@@ -127,9 +127,6 @@ class ASH_PUBLIC_EXPORT AmbientBackendController {
       int num_topics,
       OnScreenUpdateInfoFetchedCallback callback) = 0;
 
-  // Sets the initial settings to the backdrop server.
-  virtual void InitSettings(UpdateSettingsCallback callback) = 0;
-
   // Get ambient mode Settings from server.
   virtual void GetSettings(GetSettingsCallback callback) = 0;
 

ash/public/cpp/ambient/fake_ambient_backend_controller_impl.cc

@@ -102,13 +102,6 @@ void FakeAmbientBackendControllerImpl::FetchScreenUpdateInfo(
       FROM_HERE, base::BindOnce(std::move(callback), update));
 }
 
-void FakeAmbientBackendControllerImpl::InitSettings(
-    UpdateSettingsCallback callback) {
-  // Post task to simulate an async response.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), /*success=*/true));
-}
-
 void FakeAmbientBackendControllerImpl::GetSettings(
     GetSettingsCallback callback) {
   // Pretend to respond asynchronously.

ash/public/cpp/ambient/fake_ambient_backend_controller_impl.h

@@ -22,7 +22,6 @@ class ASH_PUBLIC_EXPORT FakeAmbientBackendControllerImpl
   void FetchScreenUpdateInfo(
       int num_topics,
       OnScreenUpdateInfoFetchedCallback callback) override;
-  void InitSettings(UpdateSettingsCallback callback) override;
   void GetSettings(GetSettingsCallback callback) override;
   void UpdateSettings(const AmbientSettings& settings,
                       UpdateSettingsCallback callback) override;

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

@@ -388,6 +388,17 @@ void AmbientModeHandler::OnSettingsAndAlbumsFetched(
   if (!topic_source) {
     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;
+      UpdateSettings();
+    }
     return;
   }