Update criteria for holding space tray visibility in the shelf.

Previously the holding space tray would be visible in the shelf if the
user hadn't yet pinned anything or if the holding space was non-empty.

Now, the holding space tray will be hidden until the user first adds
something to their holding space. Once something has been added, the
tray will be visible until the user has pinned their first file. Once
they have pinned their first file, the holding space tray will only be
visible in the shelf if the holding space is non-empty.

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

ash/public/cpp/holding_space/holding_space_prefs.cc

@@ -19,6 +19,9 @@ namespace {
 // Boolean preference storing if holding space previews are enabled.
 constexpr char kPreviewsEnabled[] = "ash.holding_space.previews_enabled";
 
+// Time preference storing when an item was first added to holding space.
+constexpr char kTimeOfFirstAdd[] = "ash.holding_space.time_of_first_add";
+
 // Time preference storing when holding space first became available.
 constexpr char kTimeOfFirstAvailability[] =
     "ash.holding_space.time_of_first_availability";
@@ -35,6 +38,7 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(
       kPreviewsEnabled,
       features::IsTemporaryHoldingSpaceContentForwardEntryPointEnabled());
+  registry->RegisterTimePref(kTimeOfFirstAdd, base::Time::UnixEpoch());
   registry->RegisterTimePref(kTimeOfFirstAvailability, base::Time::UnixEpoch());
   registry->RegisterTimePref(kTimeOfFirstEntry, base::Time::UnixEpoch());
   registry->RegisterTimePref(kTimeOfFirstPin, base::Time::UnixEpoch());
@@ -53,6 +57,28 @@ void SetPreviewsEnabled(PrefService* prefs, bool enabled) {
   prefs->SetBoolean(kPreviewsEnabled, enabled);
 }
 
+base::Optional<base::Time> GetTimeOfFirstAdd(PrefService* prefs) {
+  // The `kTimeOfFirstAdd` preference was added after the `kTimeOfFirstPin`
+  // preference, so if the `kTimeOfFirstAdd` has not yet been marked it's
+  // possible that the user may still have pinned a file at an earlier time.
+  auto* pref = prefs->FindPreference(kTimeOfFirstAdd);
+  return pref->IsDefaultValue() ? GetTimeOfFirstPin(prefs)
+                                : util::ValueToTime(pref->GetValue());
+}
+
+bool MarkTimeOfFirstAdd(PrefService* prefs) {
+  if (prefs->FindPreference(kTimeOfFirstAdd)->IsDefaultValue()) {
+    // The `kTimeOfFirstAdd` preference was added after the `kTimeOfFirstPin`
+    // preference, so it's possible that this is not actually the first time an
+    // item has been added to holding space. If `kTimeOfFirstPin` was previously
+    // recorded, that will be more accurate than using `base::Time::Now()`.
+    prefs->SetTime(kTimeOfFirstAdd,
+                   GetTimeOfFirstPin(prefs).value_or(base::Time::Now()));
+    return true;
+  }
+  return false;
+}
+
 base::Optional<base::Time> GetTimeOfFirstAvailability(PrefService* prefs) {
   auto* pref = prefs->FindPreference(kTimeOfFirstAvailability);
   return pref->IsDefaultValue() ? base::nullopt

ash/public/cpp/holding_space/holding_space_prefs.h

@@ -34,6 +34,15 @@ ASH_PUBLIC_EXPORT bool IsPreviewsEnabled(PrefService* prefs);
 // Sets whether previews are `enabled`.
 ASH_PUBLIC_EXPORT void SetPreviewsEnabled(PrefService* prefs, bool enabled);
 
+// Returns the time when a holding space item was first added. Note that if the
+// time of first add is unmarked, `base::nullopt` is returned.
+ASH_PUBLIC_EXPORT base::Optional<base::Time> GetTimeOfFirstAdd(
+    PrefService* prefs);
+
+// Marks the time when the first holding space item was added. If the time of
+// first add was previously marked, this no-ops and returns false.
+ASH_PUBLIC_EXPORT bool MarkTimeOfFirstAdd(PrefService* prefs);
+
 // Returns the time when holding space first became available. Note that if the
 // time of first availability is unmarked, `base::nullopt` is returned.
 ASH_PUBLIC_EXPORT base::Optional<base::Time> GetTimeOfFirstAvailability(

ash/system/holding_space/holding_space_tray.cc

@@ -145,7 +145,7 @@ const char* HoldingSpaceTray::GetClassName() const {
 void HoldingSpaceTray::UpdateVisibility() {
   HoldingSpaceModel* model = HoldingSpaceController::Get()->model();
 
-  bool logged_in =
+  const bool logged_in =
       shelf()->GetStatusAreaWidget()->login_status() == LoginStatus::USER;
 
   if (!model || !logged_in) {
@@ -153,15 +153,20 @@ void HoldingSpaceTray::UpdateVisibility() {
     return;
   }
 
-  PrefService* active_pref_service =
+  PrefService* prefs =
       Shell::Get()->session_controller()->GetActivePrefService();
-  bool has_ever_pinned_item =
-      active_pref_service
-          ? holding_space_prefs::GetTimeOfFirstPin(active_pref_service)
-                .has_value()
-          : false;
-
-  SetVisiblePreferred(!has_ever_pinned_item ||
+  const bool has_ever_added_item =
+      prefs ? holding_space_prefs::GetTimeOfFirstAdd(prefs).has_value() : false;
+  const bool has_ever_pinned_item =
+      prefs ? holding_space_prefs::GetTimeOfFirstPin(prefs).has_value() : false;
+
+  // The holding space tray should not be visible in the shelf until the user
+  // has added their first item to holding space. Once an item has been added,
+  // the holding space tray will continue to be visible until the user has
+  // pinned their first file. After the user has pinned their first file, the
+  // holding space tray will only be visible in the shelf if their holding space
+  // contains finalized items.
+  SetVisiblePreferred((has_ever_added_item && !has_ever_pinned_item) ||
                       ModelContainsFinalizedItems(model));
 }
 

ash/system/holding_space/holding_space_tray_unittest.cc

@@ -104,11 +104,24 @@ class HoldingSpaceTrayTest : public AshTestBase,
     return deserialized_item_ptr;
   }
 
-  void StartSession() {
+  // The holding space tray is only visible in the shelf after the first holding
+  // space item has been added. Most tests do not care about this so, as a
+  // convenience, the time of first add will be marked prior to starting the
+  // session when `pre_mark_time_of_first_add` is true.
+  void StartSession(bool pre_mark_time_of_first_add = true) {
+    if (pre_mark_time_of_first_add)
+      MarkTimeOfFirstAdd();
+
     AccountId user_account = AccountId::FromUserEmail(kTestUser);
     GetSessionControllerClient()->SwitchActiveUser(user_account);
   }
 
+  void MarkTimeOfFirstAdd() {
+    AccountId user_account = AccountId::FromUserEmail(kTestUser);
+    holding_space_prefs::MarkTimeOfFirstAdd(
+        GetSessionControllerClient()->GetUserPrefService(user_account));
+  }
+
   void MarkTimeOfFirstPin() {
     AccountId user_account = AccountId::FromUserEmail(kTestUser);
     holding_space_prefs::MarkTimeOfFirstPin(
@@ -128,24 +141,27 @@ class HoldingSpaceTrayTest : public AshTestBase,
 };
 
 TEST_P(HoldingSpaceTrayTest, ShowTrayButtonOnFirstUse) {
-  StartSession();
+  StartSession(/*pre_mark_time_of_first_add=*/false);
+
+  // The tray button should *not* be shown for users that have never added
+  // anything to the holding space.
+  EXPECT_FALSE(test_api()->IsShowingInShelf());
 
-  // Tray item should be shown for users that have never added anything to the
-  // holding space.
+  // Add a download item. This should cause the tray button to show.
+  HoldingSpaceItem* item =
+      AddItem(HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/fake"));
+  MarkTimeOfFirstAdd();
   EXPECT_TRUE(test_api()->IsShowingInShelf());
 
-  // Show the bubble - only pinned container should be shown.
+  // Show the bubble - both the pinned container and recent files container
+  // should be shown.
   test_api()->Show();
   EXPECT_TRUE(test_api()->PinnedFilesContainerShown());
-  EXPECT_FALSE(test_api()->RecentFilesContainerShown());
-
-  // Add and remove a download item.
-  HoldingSpaceItem* item =
-      AddItem(HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/fake"));
   EXPECT_TRUE(test_api()->RecentFilesContainerShown());
-  model()->RemoveItem(item->id());
 
-  // Verify the pinned files container, and the tray button are still shown.
+  // Remove the download item and verify the pinned files container, and the
+  // tray button are still shown.
+  model()->RemoveItem(item->id());
   EXPECT_TRUE(test_api()->PinnedFilesContainerShown());
   EXPECT_FALSE(test_api()->RecentFilesContainerShown());
 

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

@@ -19,6 +19,8 @@
 #include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/extensions/component_loader.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_factory.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
 #include "chromeos/dbus/session_manager/session_manager_client.h"
 #include "components/session_manager/core/session_manager.h"
@@ -199,10 +201,21 @@ HoldingSpaceItem* HoldingSpaceBrowserTestBase::AddItem(
           /*async_bitmap_resolver=*/base::DoNothing()));
 
   auto* item_ptr = item.get();
-  HoldingSpaceController::Get()->model()->AddItem(std::move(item));
+
+  // Add holding space items through the holding space keyed service so that the
+  // time of first add will be marked in preferences. The time of first add
+  // contributes to deciding when the holding space tray is visible.
+  HoldingSpaceKeyedServiceFactory::GetInstance()
+      ->GetService(GetProfile())
+      ->AddItem(std::move(item));
+
   return item_ptr;
 }
 
+void HoldingSpaceBrowserTestBase::RemoveItem(const HoldingSpaceItem* item) {
+  HoldingSpaceController::Get()->model()->RemoveItem(item->id());
+}
+
 std::vector<views::View*> HoldingSpaceBrowserTestBase::GetDownloadChips() {
   return test_api_->GetDownloadChips();
 }

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

@@ -77,6 +77,9 @@ class HoldingSpaceBrowserTestBase : public InProcessBrowserTest {
                             HoldingSpaceItem::Type type,
                             const base::FilePath& file_path);
 
+  // Removes the specified holding space `item`.
+  void RemoveItem(const HoldingSpaceItem* item);
+
   // Returns the collection of download chips in holding space UI.
   // If holding space UI is not visible, an empty collection is returned.
   std::vector<views::View*> GetDownloadChips();

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

@@ -214,6 +214,11 @@ void HoldingSpaceKeyedService::AddScreenRecording(
 }
 
 void HoldingSpaceKeyedService::AddItem(std::unique_ptr<HoldingSpaceItem> item) {
+  // Mark the time when the user's first item was added to holding space. Note
+  // that this no-ops if this is not the user's first time adding an item.
+  // TODO(crbug.com/1131266): Record histogram.
+  holding_space_prefs::MarkTimeOfFirstAdd(profile_->GetPrefs());
+
   holding_space_model_.AddItem(std::move(item));
 }
 

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

@@ -9,13 +9,16 @@
 #include "ash/public/cpp/holding_space/holding_space_item.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/holding_space/holding_space_prefs.h"
 #include "base/scoped_observer.h"
 #include "base/test/bind.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "content/public/test/browser_test.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "ui/aura/window.h"
 #include "ui/base/dragdrop/drag_drop_types.h"
+#include "ui/compositor/scoped_animation_duration_scale_mode.h"
 #include "ui/events/base_event_utils.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/views/controls/menu/menu_controller.h"
@@ -178,12 +181,36 @@ class DropTargetView : public views::WidgetDelegateView {
   base::FilePath copied_file_path_;
 };
 
+// HoldingSpaceUiBrowserTest ---------------------------------------------------
+
+// Base class for holding space UI browser tests.
+class HoldingSpaceUiBrowserTest : public HoldingSpaceBrowserTestBase {
+ public:
+  // HoldingSpaceBrowserTestBase:
+  void SetUpOnMainThread() override {
+    HoldingSpaceBrowserTestBase::SetUpOnMainThread();
+
+    ui::ScopedAnimationDurationScaleMode scoped_animation_duration_scale_mode(
+        ui::ScopedAnimationDurationScaleMode::ZERO_DURATION);
+
+    // The holding space tray will not show until the user has added a file to
+    // holding space. Holding space UI browser tests don't need to assert that
+    // behavior since it is already asserted in ash_unittests. As a convenience,
+    // add and remove a holding space item so that the holding space tray will
+    // already be showing during test execution.
+    ASSERT_FALSE(IsShowingInShelf());
+    RemoveItem(AddDownloadFile());
+    ASSERT_TRUE(IsShowingInShelf());
+
+    // Confirm that holding space model has been emptied for test execution.
+    ASSERT_TRUE(HoldingSpaceController::Get()->model()->items().empty());
+  }
+};
+
 }  // namespace
 
 // Tests -----------------------------------------------------------------------
 
-using HoldingSpaceUiBrowserTest = HoldingSpaceBrowserTestBase;
-
 // Base class for holding space UI browser tests that test drag-and-drop.
 // Parameterized by a callback to invoke to perform a drag-and-drop.
 class HoldingSpaceUiDragAndDropBrowserTest