Don't animate holding space previews on initial show

Adds a way to update holding space icon preview layer without an
animation, and utilizes it when the icon previews are first being shown
(i.e. when the list of previews is updated from an empty list). In this
case, tray icon will have it's own visibility animation, so there's no
need for running a separate bounce in animation within the tray icon.

Also, updates how tray icon updates are scheduled - instead of updating
the tray visibility synchronously, and then scheduling a previews
update, the logic is changed to update previews icon synchronously, too.
(the throttling became less important now that additions of multiple
items to the holding space are done in a batch, instead of one-by-one).

BUG=1166014

Change-Id: I3211672072262e9fff75b04a3316c6049099c749
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2627169
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#843358}

ash/system/holding_space/holding_space_test_api.cc

@@ -55,7 +55,6 @@ HoldingSpaceTestApi::HoldingSpaceTestApi()
                               ->shelf_widget()
                               ->status_area_widget()
                               ->holding_space_tray()) {
-  holding_space_tray_->set_use_zero_previews_update_delay_for_testing(true);
   // Holding space tests perform drag/drop so we need to disable blocking.
   auto* drag_drop_controller = ShellTestApi().drag_drop_controller();
   drag_drop_controller->set_should_block_during_drag_drop(false);
@@ -65,7 +64,6 @@ HoldingSpaceTestApi::~HoldingSpaceTestApi() {
   if (!Shell::HasInstance())
     return;
 
-  holding_space_tray_->set_use_zero_previews_update_delay_for_testing(false);
   // Enable blocking during drag/drop that was disabled for holding space tests.
   auto* drag_drop_controller = ShellTestApi().drag_drop_controller();
   drag_drop_controller->set_should_block_during_drag_drop(true);

ash/system/holding_space/holding_space_tray.cc

@@ -52,6 +52,12 @@ bool ModelContainsFinalizedItems(HoldingSpaceModel* model) {
   return false;
 }
 
+// Returns whether the holding space tray icon should show previews.
+bool ShouldShowPreviews() {
+  return IsPreviewsEnabled() && HoldingSpaceController::Get()->model() &&
+         ModelContainsFinalizedItems(HoldingSpaceController::Get()->model());
+}
+
 std::unique_ptr<views::ImageView> CreateDefaultTrayIcon() {
   auto icon = std::make_unique<views::ImageView>();
   icon->SetID(kHoldingSpaceTrayDefaultIconId);
@@ -132,7 +138,7 @@ void HoldingSpaceTray::AnchorUpdated() {
 }
 
 void HoldingSpaceTray::UpdateAfterLoginStatusChange() {
-  UpdateVisibility();
+  UpdateState();
 }
 
 bool HoldingSpaceTray::PerformAction(const ui::Event& event) {
@@ -206,11 +212,6 @@ void HoldingSpaceTray::SetVisiblePreferred(bool visible_preferred) {
     CloseBubble();
 }
 
-void HoldingSpaceTray::FirePreviewsUpdateTimerIfRunningForTesting() {
-  if (previews_update_.IsRunning())
-    previews_update_.FireNow();
-}
-
 void HoldingSpaceTray::UpdateVisibility() {
   HoldingSpaceModel* model = HoldingSpaceController::Get()->model();
   LoginStatus login_status = shelf()->GetStatusAreaWidget()->login_status();
@@ -252,32 +253,27 @@ void HoldingSpaceTray::HideBubble(const TrayBubbleView* bubble_view) {
 
 void HoldingSpaceTray::OnHoldingSpaceModelAttached(HoldingSpaceModel* model) {
   model_observer_.Observe(model);
-  UpdateVisibility();
-  UpdatePreviewsState();
+  UpdateState();
 }
 
 void HoldingSpaceTray::OnHoldingSpaceModelDetached(HoldingSpaceModel* model) {
   model_observer_.Reset();
-  UpdateVisibility();
-  UpdatePreviewsState();
+  UpdateState();
 }
 
 void HoldingSpaceTray::OnHoldingSpaceItemsAdded(
     const std::vector<const HoldingSpaceItem*>& items) {
-  UpdateVisibility();
-  UpdatePreviewsState();
+  UpdateState();
 }
 
 void HoldingSpaceTray::OnHoldingSpaceItemsRemoved(
     const std::vector<const HoldingSpaceItem*>& items) {
-  UpdateVisibility();
-  UpdatePreviewsState();
+  UpdateState();
 }
 
 void HoldingSpaceTray::OnHoldingSpaceItemFinalized(
     const HoldingSpaceItem* item) {
-  UpdateVisibility();
-  UpdatePreviewsState();
+  UpdateState();
 }
 
 void HoldingSpaceTray::ExecuteCommand(int command_id, int event_flags) {
@@ -361,7 +357,7 @@ void HoldingSpaceTray::OnWidgetDestroying(views::Widget* widget) {
 }
 
 void HoldingSpaceTray::OnActiveUserPrefServiceChanged(PrefService* prefs) {
-  UpdatePreviewsState();
+  UpdateState();
   ObservePrefService(prefs);
 }
 
@@ -373,43 +369,24 @@ void HoldingSpaceTray::ObservePrefService(PrefService* prefs) {
   // is owned by `this` so it is safe to bind with an unretained raw pointer.
   holding_space_prefs::AddPreviewsEnabledChangedCallback(
       pref_change_registrar_.get(),
-      base::BindRepeating(&HoldingSpaceTray::UpdatePreviewsState,
+      base::BindRepeating(&HoldingSpaceTray::UpdateState,
                           base::Unretained(this)));
 }
 
-void HoldingSpaceTray::UpdatePreviewsState() {
+void HoldingSpaceTray::UpdateState() {
+  UpdateVisibility();
   UpdatePreviewsVisibility();
-  SchedulePreviewsIconUpdate();
+  UpdatePreviewsIcon();
 }
 
 void HoldingSpaceTray::UpdatePreviewsVisibility() {
-  const bool show_previews =
-      IsPreviewsEnabled() && HoldingSpaceController::Get()->model() &&
-      ModelContainsFinalizedItems(HoldingSpaceController::Get()->model());
-
+  const bool show_previews = ShouldShowPreviews();
   if (PreviewsShown() == show_previews)
     return;
   default_tray_icon_->SetVisible(!show_previews);
 
   DCHECK(previews_tray_icon_);
   previews_tray_icon_->SetVisible(show_previews);
-
-  if (!show_previews)
-    previews_update_.Stop();
-}
-
-void HoldingSpaceTray::SchedulePreviewsIconUpdate() {
-  if (previews_update_.IsRunning())
-    return;
-
-  // Schedule async task with a short (somewhat arbitrary) delay to update
-  // previews so items added in quick succession are handled together.
-  base::TimeDelta delay = use_zero_previews_update_delay_
-                              ? base::TimeDelta()
-                              : base::TimeDelta::FromMilliseconds(50);
-  previews_update_.Start(FROM_HERE, delay,
-                         base::BindOnce(&HoldingSpaceTray::UpdatePreviewsIcon,
-                                        base::Unretained(this)));
 }
 
 void HoldingSpaceTray::UpdatePreviewsIcon() {

ash/system/holding_space/holding_space_tray.h

@@ -20,7 +20,6 @@
 #include "ash/system/tray/tray_background_view.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observation.h"
-#include "base/timer/timer.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/views/context_menu_controller.h"
 #include "ui/views/metadata/metadata_header_macros.h"
@@ -70,12 +69,6 @@ class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView,
   TrayBubbleView* GetBubbleView() override;
   void SetVisiblePreferred(bool visible_preferred) override;
 
-  void set_use_zero_previews_update_delay_for_testing(bool zero_delay) {
-    use_zero_previews_update_delay_ = zero_delay;
-  }
-
-  void FirePreviewsUpdateTimerIfRunningForTesting();
-
  private:
   void UpdateVisibility();
 
@@ -114,19 +107,17 @@ class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView,
   // space tray state.
   void ObservePrefService(PrefService* prefs);
 
-  // Called when the state reflected in the previews icon changes - it updates
-  // the previews icon visibility and schedules the previews icon update.
-  void UpdatePreviewsState();
+  // Updates the tray state:
+  // *   Updates the tray visibility.
+  // *   Selects whether previews or default icon should be shown.
+  // *   Updates the list of items represented in the previews icon.
+  void UpdateState();
 
   // Updates the visibility of the tray icon showing item previews.
   // If the previews are not enabled, or the holding space is empty, the default
   // holding space tray icon will be shown.
   void UpdatePreviewsVisibility();
 
-  // Schedules a task to update the list of items shown in the previews tray
-  // icon.
-  void SchedulePreviewsIconUpdate();
-
   // Calculates the set of items that should be added to the holding space
   // preview icon, and updates the icon state. No-op if previews are not
   // enabled.
@@ -156,13 +147,6 @@ class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView,
   // the user's preference.
   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
 
-  // Timer for updating previews shown in the content forward tray icon.
-  base::OneShotTimer previews_update_;
-
-  // Used in tests to shorten the timeout for updating previews in the content
-  // forward tray icon.
-  bool use_zero_previews_update_delay_ = false;
-
   base::ScopedObservation<HoldingSpaceController,
                           HoldingSpaceControllerObserver>
       controller_observer_{this};

ash/system/holding_space/holding_space_tray_icon.cc

@@ -117,6 +117,7 @@ HoldingSpaceTrayIcon::~HoldingSpaceTrayIcon() = default;
 
 void HoldingSpaceTrayIcon::Clear() {
   previews_update_weak_factory_.InvalidateWeakPtrs();
+  item_ids_.clear();
   previews_by_id_.clear();
   removed_previews_.clear();
   SetPreferredSize(CalculatePreferredSize());
@@ -161,11 +162,25 @@ void HoldingSpaceTrayIcon::InitLayout() {
   previews_container_->SetPaintToLayer(ui::LAYER_NOT_DRAWN);
 }
 
+void HoldingSpaceTrayIcon::UpdatePreviewsWithoutAnimation() {
+  for (auto& preview : previews_by_id_)
+    preview.second->UpdateWithoutAnimation();
+
+  UpdatePreviewLayerStacking();
+
+  if (resize_animation_)
+    resize_animation_.reset();
+  SetPreferredSize(CalculatePreferredSize());
+}
+
 void HoldingSpaceTrayIcon::UpdatePreviews(
     const std::vector<const HoldingSpaceItem*> items) {
   // Cancel any in progress updates.
   previews_update_weak_factory_.InvalidateWeakPtrs();
 
+  // Don't animate if transitioning from empty previews icon to prevent
+  // previews from animating while the tray icon is animating in.
+  const bool animate = !item_ids_.empty();
   item_ids_.clear();
 
   // Go over the new item list, create previews for new items, and assign new
@@ -197,6 +212,14 @@ void HoldingSpaceTrayIcon::UpdatePreviews(
       items_to_remove.push_back(preview_pair.first);
   }
 
+  if (!animate) {
+    for (auto& item_id : items_to_remove)
+      previews_by_id_.erase(item_id);
+
+    UpdatePreviewsWithoutAnimation();
+    return;
+  }
+
   if (items_to_remove.empty()) {
     OnOldItemsRemoved();
     return;
@@ -275,13 +298,7 @@ void HoldingSpaceTrayIcon::OnOldItemsRemoved() {
   ShiftExistingItems();
   AnimateInNewItems();
 
-  // Ensure that preview layers stacking matches their order in the item list.
-  for (auto& item_id : item_ids_) {
-    auto preview_it = previews_by_id_.find(item_id);
-    HoldingSpaceTrayIconPreview* preview_ptr = preview_it->second.get();
-    if (preview_ptr->layer())
-      previews_container_->layer()->StackAtBottom(preview_ptr->layer());
-  }
+  UpdatePreviewLayerStacking();
 }
 
 void HoldingSpaceTrayIcon::ShiftExistingItems() {
@@ -358,6 +375,15 @@ void HoldingSpaceTrayIcon::AnimateInNewItems() {
   }
 }
 
+void HoldingSpaceTrayIcon::UpdatePreviewLayerStacking() {
+  for (auto& item_id : item_ids_) {
+    auto preview_it = previews_by_id_.find(item_id);
+    HoldingSpaceTrayIconPreview* preview_ptr = preview_it->second.get();
+    if (preview_ptr->layer())
+      previews_container_->layer()->StackAtBottom(preview_ptr->layer());
+  }
+}
+
 BEGIN_METADATA(HoldingSpaceTrayIcon, views::View)
 END_METADATA
 

ash/system/holding_space/holding_space_tray_icon.h

@@ -69,6 +69,10 @@ class ASH_EXPORT HoldingSpaceTrayIcon : public views::View,
 
   void InitLayout();
 
+  // Updates the previews in the icon without an animation. Assumes that the
+  // target previews state has been set for all items in `previews_by_id_`.
+  void UpdatePreviewsWithoutAnimation();
+
   // Invoked when the specified preview has completed animating out. At this
   // point it is owned by `removed_previews_` and should be destroyed.
   void OnOldItemAnimatedOut(HoldingSpaceTrayIconPreview*,
@@ -85,6 +89,9 @@ class ASH_EXPORT HoldingSpaceTrayIcon : public views::View,
   // Animates new items in. Done while updating the previews shown in the icon.
   void AnimateInNewItems();
 
+  // Ensure that preview layers stacking matches their order in the item list.
+  void UpdatePreviewLayerStacking();
+
   // The shelf associated with this holding space tray icon.
   Shelf* const shelf_;
 

ash/system/holding_space/holding_space_tray_icon_preview.cc

@@ -135,6 +135,23 @@ HoldingSpaceTrayIconPreview::HoldingSpaceTrayIconPreview(
 
 HoldingSpaceTrayIconPreview::~HoldingSpaceTrayIconPreview() = default;
 
+void HoldingSpaceTrayIconPreview::UpdateWithoutAnimation() {
+  DCHECK(pending_index_.has_value());
+
+  index_ = *pending_index_;
+
+  transform_ = gfx::Transform();
+  gfx::Vector2dF translation(index_.value() * GetPreviewSize().width() / 2, 0);
+  AdjustForShelfAlignmentAndTextDirection(&translation);
+  transform_.Translate(translation);
+
+  if (NeedsLayer()) {
+    CreateLayer(transform_);
+  } else {
+    DestroyLayer();
+  }
+}
+
 void HoldingSpaceTrayIconPreview::AnimateIn(base::TimeDelta additional_delay) {
   DCHECK(transform_.IsIdentity());
   DCHECK(!index_.has_value());
@@ -342,10 +359,8 @@ void HoldingSpaceTrayIconPreview::OnDeviceScaleFactorChanged(
 }
 
 void HoldingSpaceTrayIconPreview::OnImplicitAnimationsCompleted() {
-  if (!NeedsLayer()) {
-    container_->layer()->Remove(layer_.get());
-    layer_.reset();
-  }
+  if (!NeedsLayer())
+    DestroyLayer();
 
   // NOTE: Running `animate_out_closure_` may delete `this`.
   if (animate_out_closure_)
@@ -383,6 +398,13 @@ void HoldingSpaceTrayIconPreview::CreateLayer(
   container_->layer()->Add(layer_.get());
 }
 
+void HoldingSpaceTrayIconPreview::DestroyLayer() {
+  if (!layer_)
+    return;
+  container_->layer()->Remove(layer_.get());
+  layer_.reset();
+}
+
 bool HoldingSpaceTrayIconPreview::NeedsLayer() const {
   return index_ && *index_ <= kHoldingSpaceTrayIconMaxVisiblePreviews;
 }

ash/system/holding_space/holding_space_tray_icon_preview.h

@@ -46,6 +46,10 @@ class ASH_EXPORT HoldingSpaceTrayIconPreview
       delete;
   ~HoldingSpaceTrayIconPreview() override;
 
+  // Updates the preview state to match its `pending_index_` without an
+  // animation.
+  void UpdateWithoutAnimation();
+
   // Animates this preview in. The item is animated at `*pending_index_`. This
   // will move `pending_index_` value to `index_`.
   // `additional_delay` - the delay that should be added on top of initial delay
@@ -102,6 +106,9 @@ class ASH_EXPORT HoldingSpaceTrayIconPreview
   // |initial_transform| - The transform that should be set on the layer.
   void CreateLayer(const gfx::Transform& initial_transform);
 
+  // Destroys the `layer_` for this preview, if it was previously created.
+  void DestroyLayer();
+
   // Returns whether this preview needs a layer for its current `transform_`.
   // Since we only maintain `layer_` while it appears in the viewport for the
   // holding space tray `container_`, this is used to gate creation/deletion of

ash/system/holding_space/holding_space_tray_unittest.cc

@@ -260,7 +260,6 @@ class HoldingSpaceTrayTest : public AshTestBase,
 
 TEST_P(HoldingSpaceTrayTest, ShowTrayButtonOnFirstUse) {
   StartSession(/*pre_mark_time_of_first_add=*/false);
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
 
   // The tray button should *not* be shown for users that have never added
   // anything to the holding space.
@@ -270,7 +269,6 @@ TEST_P(HoldingSpaceTrayTest, ShowTrayButtonOnFirstUse) {
   HoldingSpaceItem* item =
       AddItem(HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/fake"));
   MarkTimeOfFirstAdd();
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_TRUE(test_api()->IsShowingInShelf());
   EXPECT_EQ(!IsPreviewsFeatureEnabled(),
             IsViewVisible(test_api()->GetDefaultTrayIcon()));
@@ -327,7 +325,6 @@ TEST_P(HoldingSpaceTrayTest, HideButtonWhenModelDetached) {
 
   SwitchToSecondaryUser("user@secondary", /*client=*/nullptr,
                         /*model=*/nullptr);
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
 
   EXPECT_FALSE(test_api()->IsShowingInShelf());
   UnregisterModelForUser("user@secondary");
@@ -352,13 +349,11 @@ TEST_P(HoldingSpaceTrayTest, HideButtonOnChangeToEmptyModel) {
   HoldingSpaceModel secondary_holding_space_model;
   SwitchToSecondaryUser("user@secondary", /*client=*/nullptr,
                         /*model=*/&secondary_holding_space_model);
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_FALSE(test_api()->IsShowingInShelf());
 
   AddItemToModel(&secondary_holding_space_model,
                  HoldingSpaceItem::Type::kDownload,
                  base::FilePath("/tmp/fake_2"));
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_TRUE(test_api()->IsShowingInShelf());
   EXPECT_EQ(!IsPreviewsFeatureEnabled(),
             IsViewVisible(test_api()->GetDefaultTrayIcon()));
@@ -380,7 +375,6 @@ TEST_P(HoldingSpaceTrayTest, HideButtonOnChangeToNonEmptyModel) {
   AddItemToModel(&secondary_holding_space_model,
                  HoldingSpaceItem::Type::kDownload,
                  base::FilePath("/tmp/fake_2"));
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_FALSE(test_api()->IsShowingInShelf());
 
   SwitchToSecondaryUser("user@secondary", /*client=*/nullptr,
@@ -406,7 +400,6 @@ TEST_P(HoldingSpaceTrayTest, AddingItemShowsTrayBubble) {
   // Add a download item - the button should be shown.
   HoldingSpaceItem* item_1 =
       AddItem(HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/fake_1"));
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_TRUE(test_api()->IsShowingInShelf());
   EXPECT_EQ(!IsPreviewsFeatureEnabled(),
             IsViewVisible(test_api()->GetDefaultTrayIcon()));
@@ -415,7 +408,6 @@ TEST_P(HoldingSpaceTrayTest, AddingItemShowsTrayBubble) {
 
   // Remove the only item - the button should be hidden.
   model()->RemoveItem(item_1->id());
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_FALSE(test_api()->IsShowingInShelf());
 
   // Add a screen capture item - the button should be shown.
@@ -429,13 +421,11 @@ TEST_P(HoldingSpaceTrayTest, AddingItemShowsTrayBubble) {
 
   // Remove the only item - the button should be hidden.
   model()->RemoveItem(item_2->id());
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_FALSE(test_api()->IsShowingInShelf());
 
   // Add a pinned item - the button should be shown.
   HoldingSpaceItem* item_3 =
       AddItem(HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/fake_3"));
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_TRUE(test_api()->IsShowingInShelf());
   EXPECT_EQ(!IsPreviewsFeatureEnabled(),
             IsViewVisible(test_api()->GetDefaultTrayIcon()));
@@ -444,7 +434,6 @@ TEST_P(HoldingSpaceTrayTest, AddingItemShowsTrayBubble) {
 
   // Remove the only item - the button should be hidden.
   model()->RemoveItem(item_3->id());
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_FALSE(test_api()->IsShowingInShelf());
 }
 
@@ -468,13 +457,11 @@ TEST_P(HoldingSpaceTrayTest, TrayButtonNotShownForPartialItemsOnly) {
   EXPECT_FALSE(test_api()->IsShowingInShelf());
   AddPartiallyInitializedItem(HoldingSpaceItem::Type::kPinnedFile,
                               base::FilePath("/tmp/fake_4"));
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_FALSE(test_api()->IsShowingInShelf());
 
   // Finalize one item, and verify the tray button gets shown.
   model()->FinalizeOrRemoveItem(item_2->id(), GURL("filesystem:fake_2"));
 
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_TRUE(test_api()->IsShowingInShelf());
   EXPECT_EQ(!IsPreviewsFeatureEnabled(),
             IsViewVisible(test_api()->GetDefaultTrayIcon()));
@@ -483,7 +470,6 @@ TEST_P(HoldingSpaceTrayTest, TrayButtonNotShownForPartialItemsOnly) {
 
   // Remove the finalized item - the shelf button should get hidden.
   model()->RemoveItem(item_2->id());
-  GetTray()->FirePreviewsUpdateTimerIfRunningForTesting();
   EXPECT_FALSE(test_api()->IsShowingInShelf());
 }