Split out default tray icon from HoldingSpaceTrayIcon

Leaves HoldingSpaceTrayIcon to only handle the content forward icon.
Adds another image view for the default icon, and lets HoldingSpaceTray
manage which icon is visible.

Removes holding space observation logic from HoldingSpaceTrayIcon to
avoid duplicate event handling to avoid HoldingSpaceTrayIcon getting
duplicate holding space model events - e.g. if the icon is created
or initialized during OnHoldingSpaceModelAttached, and starts observing
model changes itself, it may handle model attachment twice. Relying on
HoldingSpaceTray to notify HoldingSpaceTrayIcon about model changes
makes avoid such scenario easier.

The end goal is to make HoldingSpaceTrayIcon not depend on individual
holding space model changes, and just have an API that would specify the
list of items whose previews should be shown in the tray icon - will be
done in a follow-up.

BUG=1142572

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

ash/public/cpp/holding_space/holding_space_constants.h

@@ -56,7 +56,8 @@ enum HoldingSpaceCommandId {
 constexpr int kHoldingSpacePinnedFilesContainerId = 1;
 constexpr int kHoldingSpaceRecentFilesContainerId = 2;
 constexpr int kHoldingSpaceScreenCapturePlayIconId = 3;
-constexpr int kHoldingSpaceTrayIconId = 4;
+constexpr int kHoldingSpaceTrayDefaultIconId = 4;
+constexpr int kHoldingSpaceTrayPreviewsIconId = 5;
 
 // The maximum allowed age for files restored into the holding space model.
 // Note that this is not enforced for pinned items.

ash/public/cpp/holding_space/holding_space_test_api.h

@@ -61,8 +61,13 @@ class ASH_EXPORT HoldingSpaceTestApi {
   // Returns the holding space tray in the shelf.
   views::View* GetTray();
 
-  // Returns the holding space tray icon in the shelf.
-  views::View* GetTrayIcon();
+  // Returns the holding space tray icon view for the default, non content
+  // forward  icon.
+  views::View* GetDefaultTrayIcon();
+
+  // Returns the holding space tray icon view for the content forward icon,
+  // which displays previews of most recent items added to holding space.
+  views::View* GetPreviewsTrayIcon();
 
   // Returns whether the pinned files container is shown.
   bool PinnedFilesContainerShown() const;

ash/system/holding_space/holding_space_test_api.cc

@@ -130,8 +130,12 @@ views::View* HoldingSpaceTestApi::GetTray() {
   return holding_space_tray_;
 }
 
-views::View* HoldingSpaceTestApi::GetTrayIcon() {
-  return holding_space_tray_->GetViewByID(kHoldingSpaceTrayIconId);
+views::View* HoldingSpaceTestApi::GetDefaultTrayIcon() {
+  return holding_space_tray_->GetViewByID(kHoldingSpaceTrayDefaultIconId);
+}
+
+views::View* HoldingSpaceTestApi::GetPreviewsTrayIcon() {
+  return holding_space_tray_->GetViewByID(kHoldingSpaceTrayPreviewsIconId);
 }
 
 bool HoldingSpaceTestApi::PinnedFilesContainerShown() const {

ash/system/holding_space/holding_space_tray.cc

@@ -20,8 +20,12 @@
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/holding_space/holding_space_tray_bubble.h"
 #include "ash/system/holding_space/holding_space_tray_icon.h"
+#include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_container.h"
+#include "components/prefs/pref_change_registrar.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/paint_vector_icon.h"
+#include "ui/views/controls/image_view.h"
 #include "ui/views/controls/menu/menu_runner.h"
 
 namespace ash {
@@ -30,6 +34,13 @@ namespace {
 
 // Helpers ---------------------------------------------------------------------
 
+// Returns whether previews are enabled.
+bool IsPreviewsEnabled() {
+  auto* prefs = Shell::Get()->session_controller()->GetActivePrefService();
+  return features::IsTemporaryHoldingSpacePreviewsEnabled() && prefs &&
+         holding_space_prefs::IsPreviewsEnabled(prefs);
+}
+
 // Returns whether the holding space model contains any finalized items.
 bool ModelContainsFinalizedItems(HoldingSpaceModel* model) {
   for (const auto& item : model->items()) {
@@ -39,21 +50,45 @@ bool ModelContainsFinalizedItems(HoldingSpaceModel* model) {
   return false;
 }
 
+std::unique_ptr<views::ImageView> CreateDefaultTrayIcon() {
+  auto icon = std::make_unique<views::ImageView>();
+  icon->SetID(kHoldingSpaceTrayDefaultIconId);
+  icon->SetImage(gfx::CreateVectorIcon(
+      kHoldingSpaceIcon, kHoldingSpaceTrayIconSize,
+      AshColorProvider::Get()->GetContentLayerColor(
+          AshColorProvider::ContentLayerType::kIconColorPrimary)));
+  icon->SetTooltipText(l10n_util::GetStringUTF16(IDS_ASH_HOLDING_SPACE_TITLE));
+  icon->SetPreferredSize(gfx::Size(kTrayItemSize, kTrayItemSize));
+  return icon;
+}
+
 }  // namespace
 
 // HoldingSpaceTray ------------------------------------------------------------
 
 HoldingSpaceTray::HoldingSpaceTray(Shelf* shelf) : TrayBackgroundView(shelf) {
-  controller_observer_.Add(HoldingSpaceController::Get());
+  controller_observer_.Observe(HoldingSpaceController::Get());
   SetVisible(false);
 
-  // Context menu.
-  if (features::IsTemporaryHoldingSpacePreviewsEnabled())
-    set_context_menu_controller(this);
-
   // Icon.
-  icon_ = tray_container()->AddChildView(
-      std::make_unique<HoldingSpaceTrayIcon>(shelf));
+  default_tray_icon_ = tray_container()->AddChildView(CreateDefaultTrayIcon());
+
+  if (features::IsTemporaryHoldingSpacePreviewsEnabled()) {
+    previews_tray_icon_ = tray_container()->AddChildView(
+        std::make_unique<HoldingSpaceTrayIcon>(shelf));
+    previews_tray_icon_->SetVisible(false);
+    UpdatePreviewsVisibility();
+
+    // If previews feature is enabled, the preview icon is displayed
+    // conditionally, depending on user prefs state.
+    session_observer_.Observe(Shell::Get()->session_controller());
+    auto* prefs = Shell::Get()->session_controller()->GetActivePrefService();
+    if (prefs)
+      ObservePrefService(prefs);
+
+    // Enable context menu, which supports an action to toggle item previews.
+    set_context_menu_controller(this);
+  }
 
   // It's possible that this holding space tray was created after login, such as
   // would occur if the user connects an external display. In such situations
@@ -73,7 +108,10 @@ base::string16 HoldingSpaceTray::GetAccessibleNameForTray() {
 }
 
 void HoldingSpaceTray::HandleLocaleChange() {
-  icon_->OnLocaleChanged();
+  default_tray_icon_->SetTooltipText(
+      l10n_util::GetStringUTF16(IDS_ASH_HOLDING_SPACE_TITLE));
+  if (previews_tray_icon_)
+    previews_tray_icon_->OnLocaleChanged();
 }
 
 void HoldingSpaceTray::HideBubbleWithView(const TrayBubbleView* bubble_view) {}
@@ -114,7 +152,7 @@ void HoldingSpaceTray::CloseBubble() {
   holding_space_metrics::RecordPodAction(
       holding_space_metrics::PodAction::kClose);
 
-  widget_observer_.RemoveAll();
+  widget_observer_.Reset();
 
   bubble_.reset();
   SetIsActive(false);
@@ -135,7 +173,7 @@ void HoldingSpaceTray::ShowBubble(bool show_by_click) {
   // being destroyed. If destruction is due to a call to `CloseBubble()` we will
   // have already cleaned up state but there are cases where the bubble widget
   // is destroyed independent of a call to `CloseBubble()`, e.g. ESC key press.
-  widget_observer_.Add(bubble_->GetBubbleWidget());
+  widget_observer_.Observe(bubble_->GetBubbleWidget());
 
   SetIsActive(true);
 }
@@ -189,26 +227,41 @@ void HoldingSpaceTray::HideBubble(const TrayBubbleView* bubble_view) {
 }
 
 void HoldingSpaceTray::OnHoldingSpaceModelAttached(HoldingSpaceModel* model) {
-  model_observer_.Add(model);
+  model_observer_.Observe(model);
   UpdateVisibility();
+  if (PreviewsShown())
+    previews_tray_icon_->OnHoldingSpaceModelAttached(model);
+  UpdatePreviewsVisibility();
 }
 
 void HoldingSpaceTray::OnHoldingSpaceModelDetached(HoldingSpaceModel* model) {
-  model_observer_.Remove(model);
+  model_observer_.Reset();
   UpdateVisibility();
+  if (PreviewsShown())
+    previews_tray_icon_->OnHoldingSpaceModelDetached(model);
+  UpdatePreviewsVisibility();
 }
 
 void HoldingSpaceTray::OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) {
   UpdateVisibility();
+  if (PreviewsShown())
+    previews_tray_icon_->OnHoldingSpaceItemAdded(item);
+  UpdatePreviewsVisibility();
 }
 
 void HoldingSpaceTray::OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) {
   UpdateVisibility();
+  if (PreviewsShown())
+    previews_tray_icon_->OnHoldingSpaceItemRemoved(item);
+  UpdatePreviewsVisibility();
 }
 
 void HoldingSpaceTray::OnHoldingSpaceItemFinalized(
     const HoldingSpaceItem* item) {
   UpdateVisibility();
+  if (PreviewsShown())
+    previews_tray_icon_->OnHoldingSpaceItemFinalized(item);
+  UpdatePreviewsVisibility();
 }
 
 void HoldingSpaceTray::ExecuteCommand(int command_id, int event_flags) {
@@ -291,4 +344,46 @@ void HoldingSpaceTray::OnWidgetDestroying(views::Widget* widget) {
   CloseBubble();
 }
 
+void HoldingSpaceTray::OnActiveUserPrefServiceChanged(PrefService* prefs) {
+  UpdatePreviewsVisibility();
+  ObservePrefService(prefs);
+}
+
+void HoldingSpaceTray::ObservePrefService(PrefService* prefs) {
+  pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
+  pref_change_registrar_->Init(prefs);
+
+  // NOTE: The callback being bound is scoped to `pref_change_registrar_` which
+  // 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::UpdatePreviewsVisibility,
+                          base::Unretained(this)));
+}
+
+void HoldingSpaceTray::UpdatePreviewsVisibility() {
+  const bool show_previews =
+      IsPreviewsEnabled() && HoldingSpaceController::Get()->model() &&
+      ModelContainsFinalizedItems(HoldingSpaceController::Get()->model());
+
+  if (PreviewsShown() == show_previews)
+    return;
+
+  default_tray_icon_->SetVisible(!show_previews);
+
+  if (previews_tray_icon_) {
+    previews_tray_icon_->SetVisible(show_previews);
+
+    if (show_previews) {
+      previews_tray_icon_->Init();
+    } else {
+      previews_tray_icon_->Reset();
+    }
+  }
+}
+
+bool HoldingSpaceTray::PreviewsShown() const {
+  return previews_tray_icon_ && previews_tray_icon_->GetVisible();
+}
+
 }  // namespace ash

ash/system/holding_space/holding_space_tray.h

@@ -12,15 +12,24 @@
 #include "ash/public/cpp/holding_space/holding_space_controller_observer.h"
 #include "ash/public/cpp/holding_space/holding_space_model.h"
 #include "ash/public/cpp/holding_space/holding_space_model_observer.h"
+#include "ash/public/cpp/session/session_controller.h"
+#include "ash/public/cpp/session/session_observer.h"
+#include "ash/style/ash_color_provider.h"
 #include "ash/system/holding_space/holding_space_tray_bubble.h"
 #include "ash/system/tray/tray_background_view.h"
 #include "base/memory/weak_ptr.h"
-#include "base/scoped_observer.h"
+#include "base/scoped_observation.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/views/context_menu_controller.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 
+class PrefChangeRegistrar;
+
+namespace views {
+class ImageView;
+}  // namespace views
+
 namespace ash {
 
 class HoldingSpaceTrayIcon;
@@ -31,6 +40,7 @@ class HoldingSpaceTrayIcon;
 class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView,
                                     public HoldingSpaceControllerObserver,
                                     public HoldingSpaceModelObserver,
+                                    public SessionObserver,
                                     public ui::SimpleMenuModel::Delegate,
                                     public views::ContextMenuController,
                                     public views::WidgetObserver {
@@ -56,6 +66,11 @@ class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView,
  private:
   void UpdateVisibility();
 
+  // 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();
+
   // TrayBubbleView::Delegate:
   base::string16 GetAccessibleNameForBubble() override;
   bool ShouldEnableExtraKeyboardAccessibility() override;
@@ -70,6 +85,9 @@ class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView,
   void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
   void OnHoldingSpaceItemFinalized(const HoldingSpaceItem* item) override;
 
+  // SessionObserver:
+  void OnActiveUserPrefServiceChanged(PrefService* prefs) override;
+
   // ui::SimpleMenuModel::Delegate:
   void ExecuteCommand(int command_id, int event_flags) override;
 
@@ -82,16 +100,43 @@ class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView,
   void OnWidgetDragWillStart(views::Widget* widget) override;
   void OnWidgetDestroying(views::Widget* widget) override;
 
-  HoldingSpaceTrayIcon* icon_ = nullptr;  // Owned by `TrayContainer`.
+  // Registers pref change registrars for preferences relevant to the holding
+  // space tray state.
+  void ObservePrefService(PrefService* prefs);
+
+  // Whether previews icon is currently shown. Note that if the previews
+  // feature is disabled, this will always be false. Otherwise, previews can be
+  // enabled/ disabled by the user at runtime.
+  bool PreviewsShown() const;
+
   std::unique_ptr<HoldingSpaceTrayBubble> bubble_;
   std::unique_ptr<ui::SimpleMenuModel> context_menu_model_;
   std::unique_ptr<views::MenuRunner> context_menu_runner_;
 
-  ScopedObserver<HoldingSpaceController, HoldingSpaceControllerObserver>
+  // Default tray icon shown when there are no previews available (or the
+  // previews are disabled).
+  // Owned by views hierarchy.
+  views::ImageView* default_tray_icon_ = nullptr;
+
+  // Content forward tray icon that contains holding space item previews.
+  // Owned by views hierarchy.
+  HoldingSpaceTrayIcon* previews_tray_icon_ = nullptr;
+
+  // When the holding space previews feature is enabled, the user can enable/
+  // disable previews at runtime. This registrar is associated with the active
+  // user pref service and notifies the holding space tray icon of changes to
+  // the user's preference.
+  std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
+
+  base::ScopedObservation<HoldingSpaceController,
+                          HoldingSpaceControllerObserver>
       controller_observer_{this};
-  ScopedObserver<HoldingSpaceModel, HoldingSpaceModelObserver> model_observer_{
+  base::ScopedObservation<HoldingSpaceModel, HoldingSpaceModelObserver>
+      model_observer_{this};
+  base::ScopedObservation<SessionController, SessionObserver> session_observer_{
       this};
-  ScopedObserver<views::Widget, views::WidgetObserver> widget_observer_{this};
+  base::ScopedObservation<views::Widget, views::WidgetObserver>
+      widget_observer_{this};
 
   base::WeakPtrFactory<HoldingSpaceTray> weak_factory_{this};
 };

ash/system/holding_space/holding_space_tray_icon.cc

@@ -10,64 +10,41 @@
 #include "ash/public/cpp/holding_space/holding_space_prefs.h"
 #include "ash/public/cpp/shelf_config.h"
 #include "ash/resources/vector_icons/vector_icons.h"
-#include "ash/session/session_controller_impl.h"
 #include "ash/shelf/shelf.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/holding_space/holding_space_tray_icon_preview.h"
 #include "ash/system/tray/tray_constants.h"
 #include "base/containers/unique_ptr_adapters.h"
 #include "base/stl_util.h"
-#include "components/prefs/pref_change_registrar.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/paint_vector_icon.h"
-#include "ui/views/controls/image_view.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
 
 namespace ash {
 
-namespace {
-
-// Helpers ---------------------------------------------------------------------
-
-// Returns whether previews are enabled.
-bool IsPreviewsEnabled() {
-  auto* prefs = Shell::Get()->session_controller()->GetActivePrefService();
-  return features::IsTemporaryHoldingSpacePreviewsEnabled() && prefs &&
-         holding_space_prefs::IsPreviewsEnabled(prefs);
-}
-
-// Sets `view`'s visibility to the specified value. Note that this method will
-// only update visibility if necessary. This is to avoid propagating an event
-// up the view tree when visibility is not changed that could otherwise result
-// in layout invalidation.
-void SetVisibility(views::View* view, bool visible) {
-  if (view->GetVisible() != visible)
-    view->SetVisible(visible);
-}
-
-}  // namespace
-
 // HoldingSpaceTrayIcon --------------------------------------------------------
 
 HoldingSpaceTrayIcon::HoldingSpaceTrayIcon(Shelf* shelf) : shelf_(shelf) {
-  SetID(kHoldingSpaceTrayIconId);
+  SetID(kHoldingSpaceTrayPreviewsIconId);
   InitLayout();
+}
 
-  if (features::IsTemporaryHoldingSpacePreviewsEnabled()) {
-    controller_observer_.Add(HoldingSpaceController::Get());
-    shell_observer_.Add(Shell::Get());
-    session_observer_.Add(Shell::Get()->session_controller());
+HoldingSpaceTrayIcon::~HoldingSpaceTrayIcon() = default;
 
-    // It's possible that this holding space tray icon was created after login,
-    // such as would occur if the user connects an external display. In such
-    // situations the holding space model will already have been attached.
-    if (HoldingSpaceController::Get()->model())
-      OnHoldingSpaceModelAttached(HoldingSpaceController::Get()->model());
-  }
+void HoldingSpaceTrayIcon::Init() {
+  shell_observer_.Add(Shell::Get());
+
+  if (HoldingSpaceController::Get()->model())
+    OnHoldingSpaceModelAttached(HoldingSpaceController::Get()->model());
 }
 
-HoldingSpaceTrayIcon::~HoldingSpaceTrayIcon() = default;
+void HoldingSpaceTrayIcon::Reset() {
+  shell_observer_.RemoveAll();
+
+  previews_.clear();
+  removed_previews_.clear();
+}
 
 void HoldingSpaceTrayIcon::OnLocaleChanged() {
   TooltipTextChanged();
@@ -90,55 +67,29 @@ void HoldingSpaceTrayIcon::InitLayout() {
   SetLayoutManager(std::make_unique<views::FillLayout>());
   SetPreferredSize(gfx::Size(kTrayItemSize, kTrayItemSize));
 
-  // No previews image.
-  // NOTE: Events are disallowed on the `no_previews_image_view_` subtree so
-  // that tooltips will be retrieved from `this` instead.
-  no_previews_image_view_ = AddChildView(std::make_unique<views::ImageView>());
-  no_previews_image_view_->SetCanProcessEventsWithinSubtree(false);
-  no_previews_image_view_->SetImage(gfx::CreateVectorIcon(
-      kHoldingSpaceIcon, kHoldingSpaceTrayIconSize,
-      AshColorProvider::Get()->GetContentLayerColor(
-          AshColorProvider::ContentLayerType::kIconColorPrimary)));
-
-  if (features::IsTemporaryHoldingSpacePreviewsEnabled()) {
-    // As holding space items are added to the model, child layers will be added
-    // to `this` view's layer to represent them.
-    SetPaintToLayer();
-    layer()->SetFillsBoundsOpaquely(false);
-    return;
-  }
+  // As holding space items are added to the model, child layers will be added
+  // to `this` view's layer to represent them.
+  SetPaintToLayer();
+  layer()->SetFillsBoundsOpaquely(false);
 }
 
 void HoldingSpaceTrayIcon::OnHoldingSpaceModelAttached(
     HoldingSpaceModel* model) {
-  DCHECK(features::IsTemporaryHoldingSpacePreviewsEnabled());
-
-  model_observer_.Add(model);
   for (const std::unique_ptr<HoldingSpaceItem>& item : model->items())
     OnHoldingSpaceItemAdded(item.get());
 }
 
 void HoldingSpaceTrayIcon::OnHoldingSpaceModelDetached(
     HoldingSpaceModel* model) {
-  DCHECK(features::IsTemporaryHoldingSpacePreviewsEnabled());
-
-  model_observer_.Remove(model);
   for (const std::unique_ptr<HoldingSpaceItem>& item : model->items())
     OnHoldingSpaceItemRemoved(item.get());
 }
 
 void HoldingSpaceTrayIcon::OnHoldingSpaceItemAdded(
     const HoldingSpaceItem* item) {
-  DCHECK(features::IsTemporaryHoldingSpacePreviewsEnabled());
-
-  if (!previews_enabled_)
-    return;
-
   if (!item->IsFinalized())
     return;
 
-  SetVisibility(no_previews_image_view_, false);
-
   for (std::unique_ptr<HoldingSpaceTrayIconPreview>& preview : previews_)
     preview->AnimateShift();
 
@@ -151,11 +102,6 @@ void HoldingSpaceTrayIcon::OnHoldingSpaceItemAdded(
 
 void HoldingSpaceTrayIcon::OnHoldingSpaceItemRemoved(
     const HoldingSpaceItem* item) {
-  DCHECK(features::IsTemporaryHoldingSpacePreviewsEnabled());
-
-  if (!previews_enabled_)
-    return;
-
   if (!item->IsFinalized())
     return;
 
@@ -185,18 +131,11 @@ void HoldingSpaceTrayIcon::OnHoldingSpaceItemRemoved(
 
 void HoldingSpaceTrayIcon::OnHoldingSpaceItemFinalized(
     const HoldingSpaceItem* item) {
-  DCHECK(features::IsTemporaryHoldingSpacePreviewsEnabled());
-
-  if (!previews_enabled_)
-    return;
-
   if (previews_.empty()) {
     OnHoldingSpaceItemAdded(item);
     return;
   }
 
-  SetVisibility(no_previews_image_view_, false);
-
   size_t index = 0;
   for (const auto& candidate :
        HoldingSpaceController::Get()->model()->items()) {
@@ -221,11 +160,6 @@ void HoldingSpaceTrayIcon::OnHoldingSpaceItemFinalized(
 void HoldingSpaceTrayIcon::OnShelfAlignmentChanged(
     aura::Window* root_window,
     ShelfAlignment old_alignment) {
-  DCHECK(features::IsTemporaryHoldingSpacePreviewsEnabled());
-
-  if (!previews_enabled_)
-    return;
-
   removed_previews_.clear();
 
   for (const auto& preview : previews_)
@@ -234,28 +168,7 @@ void HoldingSpaceTrayIcon::OnShelfAlignmentChanged(
   UpdatePreferredSize();
 }
 
-void HoldingSpaceTrayIcon::OnActiveUserPrefServiceChanged(PrefService* prefs) {
-  DCHECK(features::IsTemporaryHoldingSpacePreviewsEnabled());
-
-  pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
-  pref_change_registrar_->Init(prefs);
-
-  // NOTE: The callback being bound is scoped to `pref_change_registrar_` which
-  // 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(&HoldingSpaceTrayIcon::UpdatePreviewsEnabled,
-                          base::Unretained(this)));
-
-  UpdatePreviewsEnabled();
-}
-
 void HoldingSpaceTrayIcon::UpdatePreferredSize() {
-  if (!previews_enabled_) {
-    SetPreferredSize(gfx::Size(kTrayItemSize, kTrayItemSize));
-    return;
-  }
-
   const int num_visible_previews =
       std::min(kHoldingSpaceTrayIconMaxVisiblePreviews,
                static_cast<int>(previews_.size()));
@@ -272,51 +185,9 @@ void HoldingSpaceTrayIcon::UpdatePreferredSize() {
     SetPreferredSize(preferred_size);
 }
 
-void HoldingSpaceTrayIcon::UpdatePreviewsEnabled() {
-  DCHECK(features::IsTemporaryHoldingSpacePreviewsEnabled());
-
-  const bool previews_enabled = IsPreviewsEnabled();
-  if (previews_enabled_ == previews_enabled)
-    return;
-
-  if (previews_enabled)
-    ShowPreviews();
-  else
-    HidePreviews();
-}
-
-void HoldingSpaceTrayIcon::ShowPreviews() {
-  if (previews_enabled_)
-    return;
-
-  previews_enabled_ = true;
-
-  if (HoldingSpaceController::Get()->model()) {
-    for (const auto& item : HoldingSpaceController::Get()->model()->items())
-      OnHoldingSpaceItemAdded(item.get());
-  }
-}
-
-void HoldingSpaceTrayIcon::HidePreviews() {
-  if (!previews_enabled_)
-    return;
-
-  previews_enabled_ = false;
-
-  previews_.clear();
-  removed_previews_.clear();
-
-  SetVisibility(no_previews_image_view_, true);
-
-  UpdatePreferredSize();
-}
-
 void HoldingSpaceTrayIcon::OnHoldingSpaceTrayIconPreviewAnimatedOut(
     HoldingSpaceTrayIconPreview* preview) {
   base::EraseIf(removed_previews_, base::MatchesUniquePtr(preview));
-
-  if (previews_.empty() && removed_previews_.empty())
-    SetVisibility(no_previews_image_view_, true);
 }
 
 BEGIN_METADATA(HoldingSpaceTrayIcon, views::View)

ash/system/holding_space/holding_space_tray_icon.h

@@ -10,23 +10,13 @@
 
 #include "ash/ash_export.h"
 #include "ash/public/cpp/holding_space/holding_space_controller.h"
-#include "ash/public/cpp/holding_space/holding_space_controller_observer.h"
 #include "ash/public/cpp/holding_space/holding_space_model.h"
-#include "ash/public/cpp/holding_space/holding_space_model_observer.h"
-#include "ash/public/cpp/session/session_controller.h"
-#include "ash/public/cpp/session/session_observer.h"
 #include "ash/shell.h"
 #include "ash/shell_observer.h"
 #include "base/scoped_observer.h"
 #include "ui/views/metadata/metadata_header_macros.h"
 #include "ui/views/view.h"
 
-class PrefChangeRegistrar;
-
-namespace views {
-class ImageView;
-}  // namespace views
-
 namespace ash {
 
 class HoldingSpaceTrayIconPreview;
@@ -34,10 +24,7 @@ class Shelf;
 
 // The icon used to represent holding space in its tray in the shelf.
 class ASH_EXPORT HoldingSpaceTrayIcon : public views::View,
-                                        public HoldingSpaceControllerObserver,
-                                        public HoldingSpaceModelObserver,
-                                        public ShellObserver,
-                                        public SessionObserver {
+                                        public ShellObserver {
  public:
   METADATA_HEADER(HoldingSpaceTrayIcon);
 
@@ -49,36 +36,33 @@ class ASH_EXPORT HoldingSpaceTrayIcon : public views::View,
   // Invoked when the system locale has changed.
   void OnLocaleChanged();
 
+  // Updates the icon to match the current model state.
+  void Init();
+
+  // Clears the icon.
+  void Reset();
+
   // Returns the shelf associated with this holding space tray icon.
   Shelf* shelf() { return shelf_; }
 
+  // Called from HoldingSpaceTray when holding space model changes:
+  void OnHoldingSpaceModelAttached(HoldingSpaceModel* model);
+  void OnHoldingSpaceModelDetached(HoldingSpaceModel* model);
+  void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item);
+  void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item);
+  void OnHoldingSpaceItemFinalized(const HoldingSpaceItem* item);
+
  private:
   // views::View:
   base::string16 GetTooltipText(const gfx::Point& point) const override;
   int GetHeightForWidth(int width) const override;
 
-  // HoldingSpaceControllerObserver:
-  void OnHoldingSpaceModelAttached(HoldingSpaceModel* model) override;
-  void OnHoldingSpaceModelDetached(HoldingSpaceModel* model) override;
-
-  // HoldingSpaceModelObserver:
-  void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) override;
-  void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
-  void OnHoldingSpaceItemFinalized(const HoldingSpaceItem* item) override;
-
   // ShellObserver:
   void OnShelfAlignmentChanged(aura::Window* root_window,
                                ShelfAlignment old_alignment) override;
 
-  // SessionController:
-  void OnActiveUserPrefServiceChanged(PrefService* prefs) override;
-
   void InitLayout();
   void UpdatePreferredSize();
-  void UpdatePreviewsEnabled();
-
-  void ShowPreviews();
-  void HidePreviews();
 
   // Invoked when the specified preview has completed animating out. At this
   // point it is owned by `removed_previews_` and should be destroyed.
@@ -87,15 +71,6 @@ class ASH_EXPORT HoldingSpaceTrayIcon : public views::View,
   // The shelf associated with this holding space tray icon.
   Shelf* const shelf_;
 
-  // The child of this holding space tray icon which should be visible only if
-  // previews are disabled or there are no previews available.
-  views::ImageView* no_previews_image_view_ = nullptr;
-
-  // Whether previews are currently enabled. Note that if the previews feature
-  // is disabled, this will always be false. Otherwise, previews can be enabled/
-  // disabled by the user at runtime.
-  bool previews_enabled_ = false;
-
   // A preview is added to the tray icon to visually represent each holding
   // space item. Upon creation, previews are added to `previews_` where they
   // are owned until being animated out. Upon being animated out, previews are
@@ -104,22 +79,11 @@ class ASH_EXPORT HoldingSpaceTrayIcon : public views::View,
   std::vector<std::unique_ptr<HoldingSpaceTrayIconPreview>> previews_;
   std::vector<std::unique_ptr<HoldingSpaceTrayIconPreview>> removed_previews_;
 
-  // When the holding space previews feature is enabled, the user can enable/
-  // disable previews at runtime. This registrar is associated with the active
-  // user pref service and notifies the holding space tray icon of changes to
-  // the user's preference.
-  std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
-
-  ScopedObserver<HoldingSpaceController, HoldingSpaceControllerObserver>
-      controller_observer_{this};
-  ScopedObserver<HoldingSpaceModel, HoldingSpaceModelObserver> model_observer_{
-      this};
   ScopedObserver<Shell,
                  ShellObserver,
                  &Shell::AddShellObserver,
                  &Shell::RemoveShellObserver>
       shell_observer_{this};
-  ScopedObserver<SessionController, SessionObserver> session_observer_{this};
 };
 
 }  // namespace ash

chrome/browser/ui/ash/holding_space/holding_space_browsertest_base.cc

@@ -228,8 +228,12 @@ std::vector<views::View*> HoldingSpaceBrowserTestBase::GetScreenCaptureViews() {
   return test_api_->GetScreenCaptureViews();
 }
 
-views::View* HoldingSpaceBrowserTestBase::GetTrayIcon() {
-  return test_api_->GetTrayIcon();
+views::View* HoldingSpaceBrowserTestBase::GetDefaultTrayIcon() {
+  return test_api_->GetDefaultTrayIcon();
+}
+
+views::View* HoldingSpaceBrowserTestBase::GetPreviewsTrayIcon() {
+  return test_api_->GetPreviewsTrayIcon();
 }
 
 void HoldingSpaceBrowserTestBase::RequestAndAwaitLockScreen() {

chrome/browser/ui/ash/holding_space/holding_space_browsertest_base.h

@@ -92,8 +92,9 @@ class HoldingSpaceBrowserTestBase : public InProcessBrowserTest {
   // If holding space UI is not visible, an empty collection is returned.
   std::vector<views::View*> GetScreenCaptureViews();
 
-  // Returns the holding space tray icon in the shelf.
-  views::View* GetTrayIcon();
+  // Getter for the holding space tray icons in the shelf.
+  views::View* GetDefaultTrayIcon();
+  views::View* GetPreviewsTrayIcon();
 
   // Requests lock screen, waiting to return until session state is locked.
   void RequestAndAwaitLockScreen();

chrome/browser/ui/ash/holding_space/holding_space_ui_browsertest.cc

@@ -394,40 +394,44 @@ class HoldingSpaceUiPreviewsBrowserTest : public HoldingSpaceUiBrowserTest {
 IN_PROC_BROWSER_TEST_F(HoldingSpaceUiPreviewsBrowserTest, TogglePreviews) {
   ASSERT_TRUE(IsShowingInShelf());
 
-  auto* tray_icon = GetTrayIcon();
-  ASSERT_TRUE(tray_icon);
-  ASSERT_TRUE(tray_icon->layer());
+  // Initially, the default icon should be shown.
+  auto* default_tray_icon = GetDefaultTrayIcon();
+  ASSERT_TRUE(default_tray_icon);
+  EXPECT_TRUE(default_tray_icon->GetVisible());
 
-  // Initially the tray icon should be empty.
-  EXPECT_EQ(0u, tray_icon->layer()->children().size());
-
-  // It should have a single visible child which is the image view shown when
-  // previews are disabled or unavailable.
-  ASSERT_EQ(1u, tray_icon->children().size());
-  const views::View* no_previews_image_view = tray_icon->children()[0];
-  EXPECT_TRUE(no_previews_image_view->GetVisible());
+  auto* previews_tray_icon = GetPreviewsTrayIcon();
+  ASSERT_TRUE(previews_tray_icon);
+  ASSERT_TRUE(previews_tray_icon->layer());
+  EXPECT_FALSE(previews_tray_icon->GetVisible());
 
   // After pinning a file, we should have a single preview in the tray icon.
   AddPinnedFile();
-  EXPECT_FALSE(no_previews_image_view->GetVisible());
-  EXPECT_EQ(1u, tray_icon->layer()->children().size());
-  EXPECT_EQ(gfx::Size(32, 32), tray_icon->size());
+
+  EXPECT_FALSE(default_tray_icon->GetVisible());
+  EXPECT_TRUE(previews_tray_icon->GetVisible());
+
+  EXPECT_EQ(1u, previews_tray_icon->layer()->children().size());
+  EXPECT_EQ(gfx::Size(32, 32), previews_tray_icon->size());
 
   // After downloading a file, we should have two previews in the tray icon.
   AddDownloadFile();
-  EXPECT_FALSE(no_previews_image_view->GetVisible());
-  EXPECT_EQ(2u, tray_icon->layer()->children().size());
-  EXPECT_EQ(gfx::Size(48, 32), tray_icon->size());
+
+  EXPECT_FALSE(default_tray_icon->GetVisible());
+  EXPECT_TRUE(previews_tray_icon->GetVisible());
+  EXPECT_EQ(2u, previews_tray_icon->layer()->children().size());
+  EXPECT_EQ(gfx::Size(48, 32), previews_tray_icon->size());
 
   // After taking a screenshot, we should have three previews in the tray icon.
   AddScreenshotFile();
-  EXPECT_FALSE(no_previews_image_view->GetVisible());
-  EXPECT_EQ(3u, tray_icon->layer()->children().size());
-  EXPECT_EQ(gfx::Size(64, 32), tray_icon->size());
+
+  EXPECT_FALSE(default_tray_icon->GetVisible());
+  EXPECT_TRUE(previews_tray_icon->GetVisible());
+  EXPECT_EQ(3u, previews_tray_icon->layer()->children().size());
+  EXPECT_EQ(gfx::Size(64, 32), previews_tray_icon->size());
 
   // Right click the tray icon, and expect a context menu to be shown which will
   // allow the user to hide previews.
-  RightClick(tray_icon);
+  RightClick(previews_tray_icon);
   ASSERT_TRUE(views::MenuController::GetActiveInstance());
 
   // Use the keyboard to select the context menu item to hide previews. Doing so
@@ -438,13 +442,14 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceUiPreviewsBrowserTest, TogglePreviews) {
 
   // The tray icon should now contain no previews, but have a single child which
   // contains the static image to show when previews are disabled.
-  EXPECT_TRUE(no_previews_image_view->GetVisible());
-  EXPECT_EQ(0u, tray_icon->layer()->children().size());
-  EXPECT_EQ(gfx::Size(32, 32), tray_icon->size());
+  EXPECT_TRUE(default_tray_icon->GetVisible());
+  EXPECT_FALSE(previews_tray_icon->GetVisible());
+
+  EXPECT_EQ(gfx::Size(32, 32), default_tray_icon->size());
 
   // Right click the tray icon, and expect a context menu to be shown which will
   // allow the user to show previews.
-  RightClick(tray_icon);
+  RightClick(default_tray_icon);
   ASSERT_TRUE(views::MenuController::GetActiveInstance());
 
   // Use the keyboard to select the context menu item to show previews. Doing so
@@ -454,9 +459,11 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceUiPreviewsBrowserTest, TogglePreviews) {
   EXPECT_FALSE(views::MenuController::GetActiveInstance());
 
   // The tray icon should once again show three previews.
-  EXPECT_FALSE(no_previews_image_view->GetVisible());
-  EXPECT_EQ(3u, tray_icon->layer()->children().size());
-  EXPECT_EQ(gfx::Size(64, 32), tray_icon->size());
+  EXPECT_FALSE(default_tray_icon->GetVisible());
+  EXPECT_TRUE(previews_tray_icon->GetVisible());
+
+  EXPECT_EQ(3u, previews_tray_icon->layer()->children().size());
+  EXPECT_EQ(gfx::Size(64, 32), previews_tray_icon->size());
 }
 
 // Base class for holding space UI browser tests that take screenshots.