ambient: fix a few polish issues

* Add fade in/out animation for ambient mode
* When showing ambient mode, fixed the first black screen issue
* When idling on lock screen and ambient mode returns, fixed the
  first image quickly flashes problem

This CL enables the default window animations, it also creates the
window only when images are successfully downloaded.

Bug: None
Change-Id: Ideb5683aefe92b5f375f06515db822bfd3cb7678
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2321714Reviewed-by: Meilin Wang <meilinw@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792432}

ash/ambient/ambient_controller.cc

@@ -37,6 +37,8 @@
 #include "components/prefs/pref_registry_simple.h"
 #include "ui/base/ui_base_types.h"
 #include "ui/views/widget/widget.h"
+#include "ui/wm/core/visibility_controller.h"
+#include "ui/wm/core/window_animations.h"
 
 #if BUILDFLAG(ENABLE_CROS_AMBIENT_MODE_BACKEND)
 #include "ash/ambient/backdrop/ambient_backend_controller_impl.h"
@@ -178,6 +180,9 @@ AmbientController::AmbientController() {
   power_manager_client->GetSwitchStates(
       base::BindOnce(&AmbientController::OnReceiveSwitchStates,
                      weak_ptr_factory_.GetWeakPtr()));
+
+  ambient_backend_model_observer_.Add(
+      ambient_photo_controller_.ambient_backend_model());
 }
 
 AmbientController::~AmbientController() {
@@ -189,19 +194,8 @@ void AmbientController::OnAmbientUiVisibilityChanged(
     AmbientUiVisibility visibility) {
   switch (visibility) {
     case AmbientUiVisibility::kShown:
-      if (!container_view_)
-        CreateWidget();
-      else
-        container_view_->GetWidget()->Show();
-
-      if (inactivity_monitor_) {
-        // Resets the monitor and cancels the timer upon shown.
-        inactivity_monitor_.reset();
-      }
-
-      DCHECK(container_view_);
-      // This will be no-op if the view is already visible.
-      container_view_->SetVisible(true);
+      // Resets the monitor and cancels the timer upon shown.
+      inactivity_monitor_.reset();
 
       if (IsChargerConnected()) {
         // Requires wake lock to prevent display from sleeping.
@@ -216,42 +210,48 @@ void AmbientController::OnAmbientUiVisibilityChanged(
       StartRefreshingImages();
       break;
     case AmbientUiVisibility::kHidden:
-      container_view_->GetWidget()->Hide();
-
-      // Has no effect if |wake_lock_| has already been released.
-      ReleaseWakeLock();
-      StopRefreshingImages();
-
-      // Creates the monitor and starts the auto-show timer upon hidden.
-      DCHECK(!inactivity_monitor_);
-      if (LockScreen::HasInstance() && autoshow_enabled_) {
-        inactivity_monitor_ = std::make_unique<InactivityMonitor>(
-            LockScreen::Get()->widget()->GetWeakPtr(),
-            base::BindOnce(&AmbientController::OnAutoShowTimeOut,
-                           weak_ptr_factory_.GetWeakPtr()));
-      }
-      break;
     case AmbientUiVisibility::kClosed:
-      DCHECK(container_view_);
-      // |CloseNow()| will close the widget synchronously to ensure its
-      // view hierarchy being destroyed before |this|.
-      container_view_->GetWidget()->CloseNow();
+      if (container_view_) {
+        container_view_->GetWidget()->Close();
+        container_view_ = nullptr;
+      }
 
+      // TODO(wutao): This will clear the image cache currently. It will not
+      // work with `kHidden` if the token has expired and ambient mode is shown
+      // again.
       StopRefreshingImages();
-      CleanUpOnClosed();
 
       // We close the Assistant UI after ambient screen not being shown to sync
       // states to |AssistantUiController|. This will be a no-op if the
       // |kAmbientAssistant| feature is disabled, or the Assistant UI has
       // already been closed.
       CloseAssistantUi();
+
+      // Should do nothing if the wake lock has already been released.
+      ReleaseWakeLock();
+
+      if (visibility == AmbientUiVisibility::kHidden) {
+        // Creates the monitor and starts the auto-show timer upon hidden.
+        DCHECK(!inactivity_monitor_);
+        if (LockScreen::HasInstance() && autoshow_enabled_) {
+          inactivity_monitor_ = std::make_unique<InactivityMonitor>(
+              LockScreen::Get()->widget()->GetWeakPtr(),
+              base::BindOnce(&AmbientController::OnAutoShowTimeOut,
+                             weak_ptr_factory_.GetWeakPtr()));
+        }
+      } else {
+        DCHECK(visibility == AmbientUiVisibility::kClosed);
+        inactivity_monitor_.reset();
+        power_status_observer_.Remove(PowerStatus::Get());
+      }
+
       break;
   }
 }
 
 void AmbientController::OnAutoShowTimeOut() {
   DCHECK(IsUiHidden(ambient_ui_model_.ui_visibility()));
-  DCHECK(!container_view_->GetWidget()->IsVisible());
+  DCHECK(!container_view_);
 
   // Show ambient screen after time out.
   ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kShown);
@@ -286,7 +286,6 @@ void AmbientController::OnLockStateChanged(bool locked) {
 
     ShowUi(AmbientUiMode::kLockScreenUi);
   } else {
-    DCHECK(container_view_);
     // Ambient screen will be destroyed along with the lock screen when user
     // logs in.
     CloseUi();
@@ -364,14 +363,6 @@ void AmbientController::RemoveAmbientViewDelegateObserver(
   delegate_.RemoveObserver(observer);
 }
 
-std::unique_ptr<AmbientContainerView> AmbientController::CreateContainerView() {
-  DCHECK(!container_view_);
-
-  auto container = std::make_unique<AmbientContainerView>(&delegate_);
-  container_view_ = container.get();
-  return container;
-}
-
 void AmbientController::ShowUi(AmbientUiMode mode) {
   // TODO(meilinw): move the eligibility check to the idle entry point once
   // implemented: b/149246117.
@@ -385,13 +376,10 @@ void AmbientController::ShowUi(AmbientUiMode mode) {
 }
 
 void AmbientController::CloseUi() {
-  DCHECK(container_view_);
-
   ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kClosed);
 }
 
 void AmbientController::HideLockScreenUi() {
-  DCHECK(container_view_);
   DCHECK(IsLockScreenUi(ambient_ui_model_.ui_mode()));
 
   ambient_ui_model_.SetUiVisibility(AmbientUiVisibility::kHidden);
@@ -490,6 +478,19 @@ AmbientBackendModel* AmbientController::GetAmbientBackendModel() {
   return ambient_photo_controller_.ambient_backend_model();
 }
 
+void AmbientController::OnImagesChanged() {
+  if (!container_view_)
+    CreateWidget();
+}
+
+std::unique_ptr<AmbientContainerView> AmbientController::CreateContainerView() {
+  DCHECK(!container_view_);
+
+  auto container = std::make_unique<AmbientContainerView>(&delegate_);
+  container_view_ = container.get();
+  return container;
+}
+
 void AmbientController::CreateWidget() {
   DCHECK(!container_view_);
 
@@ -503,22 +504,18 @@ void AmbientController::CreateWidget() {
   widget->Init(std::move(params));
   widget->SetContentsView(CreateContainerView());
 
+  widget->SetVisibilityAnimationTransition(
+      views::Widget::VisibilityTransition::ANIMATE_BOTH);
+  ::wm::SetWindowVisibilityAnimationType(
+      widget->GetNativeWindow(), ::wm::WINDOW_VISIBILITY_ANIMATION_TYPE_FADE);
+  ::wm::SetWindowVisibilityChangesAnimated(widget->GetNativeWindow());
+
   widget->Show();
 
   // Requests keyboard focus for |container_view_| to receive keyboard events.
   container_view_->RequestFocus();
 }
 
-void AmbientController::CleanUpOnClosed() {
-  // Invalidates the view pointer.
-  container_view_ = nullptr;
-  inactivity_monitor_.reset();
-  power_status_observer_.Remove(PowerStatus::Get());
-
-  // Should do nothing if the wake lock has already been released.
-  ReleaseWakeLock();
-}
-
 void AmbientController::StartRefreshingImages() {
   ambient_photo_controller_.StartScreenUpdate();
 }

ash/ambient/ambient_controller.h

@@ -38,6 +38,7 @@ class AmbientViewDelegateObserver;
 // Class to handle all ambient mode functionalities.
 class ASH_EXPORT AmbientController
     : public AmbientUiModelObserver,
+      public AmbientBackendModelObserver,
       public SessionObserver,
       public PowerStatus::Observer,
       public chromeos::PowerManagerClient::Observer {
@@ -67,10 +68,6 @@ class ASH_EXPORT AmbientController
   void AddAmbientViewDelegateObserver(AmbientViewDelegateObserver* observer);
   void RemoveAmbientViewDelegateObserver(AmbientViewDelegateObserver* observer);
 
-  // Initializes the |container_view_|. Called in |CreateWidget()| to create the
-  // contents view.
-  std::unique_ptr<AmbientContainerView> CreateContainerView();
-
   // Invoked to show/close ambient UI in |mode|.
   void ShowUi(AmbientUiMode mode);
   void CloseUi();
@@ -102,6 +99,13 @@ class ASH_EXPORT AmbientController
   class InactivityMonitor;
   friend class AmbientAshTestBase;
 
+  // AmbientBackendModelObserver overrides:
+  void OnImagesChanged() override;
+
+  // Initializes the |container_view_|. Called in |CreateWidget()| to create the
+  // contents view.
+  std::unique_ptr<AmbientContainerView> CreateContainerView();
+
   // TODO(meilinw): reuses the lock-screen widget: b/156531168, b/157175030.
   // Creates and shows a full-screen widget responsible for showing
   // the ambient UI.
@@ -114,8 +118,6 @@ class ASH_EXPORT AmbientController
   // device being inactive for a specific amount of time.
   void OnAutoShowTimeOut();
 
-  void CleanUpOnClosed();
-
   void set_backend_controller_for_testing(
       std::unique_ptr<AmbientBackendController> photo_client);
 
@@ -162,6 +164,8 @@ class ASH_EXPORT AmbientController
 
   ScopedObserver<AmbientUiModel, AmbientUiModelObserver>
       ambient_ui_model_observer_{this};
+  ScopedObserver<AmbientBackendModel, AmbientBackendModelObserver>
+      ambient_backend_model_observer_{this};
   ScopedObserver<SessionControllerImpl, SessionObserver> session_observer_{
       this};
   ScopedObserver<PowerStatus, PowerStatus::Observer> power_status_observer_{

ash/ambient/ambient_controller_unittest.cc

@@ -33,6 +33,7 @@ using AmbientControllerTest = AmbientAshTestBase;
 
 TEST_F(AmbientControllerTest, ShowAmbientScreenUponLock) {
   LockScreen();
+  FastForwardToNextImage();
 
   EXPECT_TRUE(container_view());
   EXPECT_EQ(AmbientUiModel::Get()->ui_visibility(),
@@ -45,6 +46,8 @@ TEST_F(AmbientControllerTest, ShowAmbientScreenUponLock) {
 
 TEST_F(AmbientControllerTest, HideAmbientScreen) {
   LockScreen();
+  FastForwardToNextImage();
+
   EXPECT_TRUE(container_view());
   EXPECT_EQ(AmbientUiModel::Get()->ui_visibility(),
             AmbientUiVisibility::kShown);
@@ -52,10 +55,9 @@ TEST_F(AmbientControllerTest, HideAmbientScreen) {
 
   HideAmbientScreen();
 
-  EXPECT_TRUE(container_view());
+  EXPECT_FALSE(container_view());
   EXPECT_EQ(AmbientUiModel::Get()->ui_visibility(),
             AmbientUiVisibility::kHidden);
-  EXPECT_FALSE(container_view()->GetWidget()->IsVisible());
 
   // Clean up.
   CloseAmbientScreen();
@@ -63,6 +65,8 @@ TEST_F(AmbientControllerTest, HideAmbientScreen) {
 
 TEST_F(AmbientControllerTest, CloseAmbientScreenUponUnlock) {
   LockScreen();
+  FastForwardToNextImage();
+
   EXPECT_TRUE(container_view());
   EXPECT_EQ(AmbientUiModel::Get()->ui_visibility(),
             AmbientUiVisibility::kShown);
@@ -238,7 +242,6 @@ TEST_F(AmbientControllerTest,
   base::RunLoop().RunUntilIdle();
 
   // Should release the wake lock when battery is not charging.
-  EXPECT_TRUE(ambient_controller()->IsShown());
   EXPECT_EQ(0, GetNumOfActiveWakeLocks(
                    device::mojom::WakeLockType::kPreventDisplaySleep));
 
@@ -250,12 +253,13 @@ TEST_F(AmbientControllerTest,
 
 TEST_F(AmbientControllerTest, ShouldDismissContainerViewWhenKeyPressed) {
   ShowAmbientScreen();
+  FastForwardToNextImage();
   EXPECT_TRUE(container_view()->GetWidget()->IsVisible());
 
   // Simulates a random keyboard press event.
   GetEventGenerator()->PressKey(ui::VKEY_SPACE, /*flags=*/0);
 
-  EXPECT_FALSE(container_view()->GetWidget()->IsVisible());
+  EXPECT_FALSE(container_view());
 
   // Clean up.
   CloseAmbientScreen();
@@ -263,6 +267,7 @@ TEST_F(AmbientControllerTest, ShouldDismissContainerViewWhenKeyPressed) {
 
 TEST_F(AmbientControllerTest, ShouldDismissContainerViewOnRealMouseMove) {
   ShowAmbientScreen();
+  FastForwardToNextImage();
   EXPECT_TRUE(container_view()->GetWidget()->IsVisible());
 
   // Simulates a tiny mouse move within the threshold, which should be ignored.
@@ -272,7 +277,7 @@ TEST_F(AmbientControllerTest, ShouldDismissContainerViewOnRealMouseMove) {
   // Simulates a big mouse move beyond the threshold, which should take effect
   // and dismiss the ambient.
   GetEventGenerator()->MoveMouseBy(/*x=*/15, /*y=*/15);
-  EXPECT_FALSE(container_view()->GetWidget()->IsVisible());
+  EXPECT_FALSE(container_view());
 }
 
 TEST_F(AmbientControllerTest, UpdateUiAndWakeLockWhenSystemSuspendOrResume) {
@@ -299,7 +304,7 @@ TEST_F(AmbientControllerTest, UpdateUiAndWakeLockWhenSystemSuspendOrResume) {
 
   // System suspension should hide Ui and release the wake lock acquired
   // previously.
-  EXPECT_FALSE(container_view()->GetWidget()->IsVisible());
+  EXPECT_FALSE(container_view());
   EXPECT_EQ(0, GetNumOfActiveWakeLocks(
                    device::mojom::WakeLockType::kPreventDisplaySleep));
 
@@ -308,7 +313,7 @@ TEST_F(AmbientControllerTest, UpdateUiAndWakeLockWhenSystemSuspendOrResume) {
 
   // System resume should not invoke Ui to show up, thus no wake lock needed
   // to acquire.
-  EXPECT_FALSE(container_view()->GetWidget()->IsVisible());
+  EXPECT_FALSE(container_view());
   EXPECT_EQ(0, GetNumOfActiveWakeLocks(
                    device::mojom::WakeLockType::kPreventDisplaySleep));
 }
@@ -319,6 +324,7 @@ TEST_F(AmbientControllerTest, ShouldShowAmbientScreenWhenScreenIsDimmed) {
   // Should lock the device and enter ambient mode when the screen is dimmed.
   SetScreenDimmedAndWait(true);
 
+  FastForwardToNextImage();
   EXPECT_TRUE(ambient_controller()->IsShown());
 
   // Closes ambient for clean-up.

ash/ambient/test/ambient_ash_test_base.cc

@@ -110,6 +110,8 @@ void AmbientAshTestBase::TearDown() {
 void AmbientAshTestBase::ShowAmbientScreen() {
   // The widget will be destroyed in |AshTestBase::TearDown()|.
   ambient_controller()->ShowUi(AmbientUiMode::kLockScreenUi);
+  // The UI only shows when images are downloaded to avoid showing blank screen.
+  FastForwardToNextImage();
   // Flush the message loop to finish all async calls.
   base::RunLoop().RunUntilIdle();
 }

ash/ambient/ui/glanceable_info_view.cc

@@ -74,9 +74,15 @@ GlanceableInfoView::GlanceableInfoView(AmbientViewDelegate* delegate)
     : delegate_(delegate) {
   DCHECK(delegate);
   SetID(AssistantViewID::kAmbientGlanceableInfoView);
-  delegate_->GetAmbientBackendModel()->AddObserver(this);
+  auto* backend_model = delegate_->GetAmbientBackendModel();
+  backend_model->AddObserver(this);
 
   InitLayout();
+
+  if (!backend_model->weather_condition_icon().isNull()) {
+    // already has weather info, show immediately.
+    Show();
+  }
 }
 
 GlanceableInfoView::~GlanceableInfoView() {
@@ -97,9 +103,9 @@ void GlanceableInfoView::Show() {
 
   float temperature = delegate_->GetAmbientBackendModel()->temperature();
   // TODO(b/154046129): handle Celsius format.
-  temperature_->SetText(l10n_util::GetStringFUTF16(
+  temperature_->SetText(l10n_util::GetStringFUTF16Int(
       IDS_ASH_AMBIENT_MODE_WEATHER_TEMPERATURE_IN_FAHRENHEIT,
-      base::FormatNumber(static_cast<int>(temperature))));
+      static_cast<int>(temperature)));
 }
 
 void GlanceableInfoView::InitLayout() {