[FilesBulkPinning] Remove OnSyncingStatusUpdate from pin manager

This hasn't been used for a while, let's clean it up.

Fixed: b/297442320
Test: chromeos_unittests --gtest_filter=DriveFsPinManager*
Change-Id: I8984c86fc9db232bd65c1da509363535eb1bea08
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4917488
Reviewed-by: François Degros <fdegros@chromium.org>
Commit-Queue: François Degros <fdegros@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1207342}

chromeos/ash/components/drivefs/drivefs_pin_manager.cc

@@ -145,10 +145,6 @@ ostream& operator<<(ostream& out, Quoter<Path> q) {
   return out << "'***'";
 }
 
-ostream& operator<<(ostream& out, Quoter<std::string> q) {
-  return out << "'" << (*q.value) << "'";
-}
-
 template <typename T>
 ostream& operator<<(ostream& out, Quoter<absl::optional<T>> q) {
   const absl::optional<T>& v = *q.value;
@@ -206,16 +202,6 @@ ostream& operator<<(ostream& out, Quoter<FileMetadata> q) {
   return out << "}";
 }
 
-ostream& operator<<(ostream& out, Quoter<mojom::ItemEvent> q) {
-  const mojom::ItemEvent& e = *q.value;
-  return out << "{" << Quote(e.state) << " " << PinManager::Id(e.stable_id)
-             << " " << Quote(e.path) << ", bytes_transferred: "
-             << HumanReadableSize(e.bytes_transferred)
-             << ", bytes_to_transfer: "
-             << HumanReadableSize(e.bytes_to_transfer)
-             << ", is_download: " << e.is_download << "}";
-}
-
 ostream& operator<<(ostream& out, Quoter<mojom::ProgressEvent> q) {
   const mojom::ProgressEvent& e = *q.value;
   out << "{" << PinManager::Id(e.stable_id) << " "
@@ -1335,100 +1321,9 @@ void PinManager::OnFilePinned(const Id id,
   DCHECK(!files_to_pin_.contains(id));
 }
 
-// TODO(b/297442320): Remove `OnSyncingStatusUpdate` now we entirely rely on
-// `OnItemProgress.
-void PinManager::OnSyncingStatusUpdate(const mojom::SyncingStatus& status) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  if (use_on_item_progress_) {
-    return;
-  }
-
-  for (const mojom::ItemEventPtr& event : status.item_events) {
-    DCHECK(event);
-
-    if (!InProgress(progress_.stage)) {
-      VLOG(2) << "Ignored " << Quote(*event);
-      continue;
-    }
-
-    if (OnSyncingEvent(*event)) {
-      progress_.useful_events++;
-    } else {
-      progress_.duplicated_events++;
-      VLOG(3) << "Discarded " << Quote(*event);
-    }
-  }
-
-  PinSomeFiles();
-}
-
-// TODO(b/297442320): Remove `OnSyncingEvent`.
-bool PinManager::OnSyncingEvent(mojom::ItemEvent& event) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  if (!event.is_download) {
-    // We're only interested in download events.
-    VLOG(3) << "Ignored upload-related event " << Quote(event);
-    return false;
-  }
-
-  const Id id = Id(event.stable_id);
-  const Path path = Path(event.path);
-
-  using State = mojom::ItemEvent::State;
-  switch (event.state) {
-    case State::kQueued:
-    case State::kInProgress:
-      if (!Update(id, path, event.bytes_transferred, event.bytes_to_transfer)) {
-        return false;
-      }
-
-      VLOG(3) << Quote(event.state) << " " << id << " " << Quote(path) << ": "
-              << Quote(event);
-      VLOG_IF(2, !VLOG_IS_ON(3))
-          << Quote(event.state) << " " << id << " " << Quote(path);
-      return true;
-
-    case State::kCompleted:
-      if (!Remove(id, path)) {
-        return false;
-      }
-
-      VLOG(2) << "Synced " << id << " " << Quote(path) << ": " << Quote(event);
-      VLOG_IF(1, !VLOG_IS_ON(2)) << "Synced " << id << " " << Quote(path);
-      progress_.pinned_files++;
-      UmaHistogramBoolean("FileBrowser.GoogleDrive.BulkPinning.PinnedFiles",
-                          true);
-      return true;
-
-    case State::kFailed:
-      if (!Remove(id, path, 0)) {
-        return false;
-      }
-
-      LOG(ERROR) << Quote(event.state) << " " << id << " " << Quote(path)
-                 << ": " << Quote(event);
-      progress_.failed_files++;
-      UmaHistogramBoolean("FileBrowser.GoogleDrive.BulkPinning.PinnedFiles",
-                          false);
-      return true;
-    case State::kCancelledAndDeleted:
-    case State::kCancelledAndTrashed:
-      return false;
-  }
-
-  LOG(ERROR) << "Unexpected event type: " << Quote(event);
-  return false;
-}
-
 void PinManager::OnItemProgress(const mojom::ProgressEvent& event) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (!use_on_item_progress_) {
-    return;
-  }
-
   if (!InProgress(progress_.stage)) {
     VLOG(2) << "Ignored " << Quote(event);
     return;

chromeos/ash/components/drivefs/drivefs_pin_manager.h

@@ -239,9 +239,6 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_DRIVEFS) PinManager
     return observers_.HasObserver(observer);
   }
 
-  // Processes a syncing status event. Returns true if the event was useful.
-  bool OnSyncingEvent(mojom::ItemEvent& event);
-
   // Stable ID provided by DriveFS.
   enum class Id : int64_t { kNone = 0 };
 
@@ -249,7 +246,6 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_DRIVEFS) PinManager
   void NotifyDelete(Id id, const Path& path);
 
   // DriveFsHost::Observer implementation.
-  void OnSyncingStatusUpdate(const mojom::SyncingStatus& status) override;
   void OnUnmounted() override;
   void OnFilesChanged(const std::vector<mojom::FileChange>& changes) override;
   void OnError(const mojom::DriveError& error) override;
@@ -481,10 +477,6 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_DRIVEFS) PinManager
   // Is this the first full sync after the size estimation?
   bool is_first_sync_ GUARDED_BY_CONTEXT(sequence_checker_) = false;
 
-  // Should the feature use `OnItemProgress`, if false it will fall back to
-  // `OnSyncingStatusUpdate`.
-  bool use_on_item_progress_ GUARDED_BY_CONTEXT(sequence_checker_) = true;
-
   // Stop at the `PinSomeFiles` stage during testing to perform assertions, this
   // should always be true and only overridden in browser tests.
   bool should_pin_files_for_testing_ GUARDED_BY_CONTEXT(sequence_checker_) =

chromeos/ash/components/drivefs/drivefs_pin_manager_unittest.cc

@@ -1437,386 +1437,11 @@ TEST_F(DriveFsPinManagerTest, OnMetadataForModifiedFile) {
   manager.progress_.stage = Stage::kStopped;
 }
 
-// Tests PinManager::OnSyncingEvent().
-TEST_F(DriveFsPinManagerTest, OnSyncingEvent) {
-  PinManager manager(profile_path_, mount_path_, &drivefs_, kMaxQueueSize);
-
-  DCHECK_CALLED_ON_VALID_SEQUENCE(manager.sequence_checker_);
-  manager.progress_.bytes_to_pin = 30000;
-  manager.progress_.required_space = 32768;
-
-  {
-    const Progress progress = manager.GetProgress();
-    EXPECT_EQ(progress.failed_files, 0);
-    EXPECT_EQ(progress.pinned_files, 0);
-    EXPECT_EQ(progress.pinned_bytes, 0);
-    EXPECT_EQ(progress.bytes_to_pin, 30000);
-    EXPECT_EQ(progress.required_space, 32768);
-  }
-
-  const Id id1 = Id(549);
-  const Path path1 = Path("Path 1");
-
-  const Id id2 = Id(17);
-  const Path path2 = Path("Path 2");
-
-  // Put in place a couple of files to track.
-  {
-    const auto [it, ok] = manager.files_to_track_.try_emplace(
-        id1, PinManager::File{.path = path1, .total = 10000, .pinned = true});
-    ASSERT_TRUE(ok);
-    manager.progress_.syncing_files++;
-  }
-  {
-    const auto [it, ok] = manager.files_to_track_.try_emplace(
-        id2, PinManager::File{.path = path2, .total = 20000, .pinned = true});
-    ASSERT_TRUE(ok);
-    manager.progress_.syncing_files++;
-  }
-
-  EXPECT_THAT(manager.files_to_track_, SizeIs(2));
-
-  // An event with an unknown type is ignored.
-  {
-    ItemEvent event;
-    event.is_download = true;
-    event.stable_id = static_cast<int64_t>(id2);
-    event.path = path2.value();
-    event.state = ItemEvent::State(-1);
-    event.bytes_to_transfer = -1;
-    event.bytes_transferred = -1;
-    EXPECT_FALSE(manager.OnSyncingEvent(event));
-  }
-
-  EXPECT_THAT(manager.files_to_track_, SizeIs(2));
-
-  {
-    const Progress progress = manager.GetProgress();
-    EXPECT_EQ(progress.syncing_files, 2);
-    EXPECT_EQ(progress.failed_files, 0);
-    EXPECT_EQ(progress.pinned_files, 0);
-    EXPECT_EQ(progress.pinned_bytes, 0);
-    EXPECT_EQ(progress.bytes_to_pin, 30000);
-    EXPECT_EQ(progress.required_space, 32768);
-  }
-
-  // Mark file 1 as queued.
-  {
-    ItemEvent event;
-    event.is_download = true;
-    event.stable_id = static_cast<int64_t>(id1);
-    event.path = path1.value();
-    event.state = ItemEvent::State::kQueued;
-    event.bytes_to_transfer = 10000;
-    event.bytes_transferred = 0;
-    EXPECT_FALSE(manager.OnSyncingEvent(event));
-  }
-
-  EXPECT_THAT(manager.files_to_track_, SizeIs(2));
-
-  {
-    const Progress progress = manager.GetProgress();
-    EXPECT_EQ(progress.syncing_files, 2);
-    EXPECT_EQ(progress.failed_files, 0);
-    EXPECT_EQ(progress.pinned_files, 0);
-    EXPECT_EQ(progress.pinned_bytes, 0);
-    EXPECT_EQ(progress.bytes_to_pin, 30000);
-    EXPECT_EQ(progress.required_space, 32768);
-  }
-
-  {
-    const auto it = manager.files_to_track_.find(id1);
-    ASSERT_NE(it, manager.files_to_track_.end());
-    const auto& [id, file] = *it;
-    EXPECT_EQ(id, id1);
-    EXPECT_EQ(file.path, path1);
-    EXPECT_EQ(file.total, 10000);
-    EXPECT_EQ(file.transferred, 0);
-    EXPECT_TRUE(file.pinned);
-    EXPECT_FALSE(file.in_progress);
-  }
-
-  // Mark file 1 as in progress.
-  {
-    ItemEvent event;
-    event.is_download = true;
-    event.stable_id = static_cast<int64_t>(id1);
-    event.path = path1.value();
-    event.state = ItemEvent::State::kInProgress;
-    event.bytes_to_transfer = 10000;
-    event.bytes_transferred = 5000;
-    EXPECT_TRUE(manager.OnSyncingEvent(event));
-    EXPECT_FALSE(manager.OnSyncingEvent(event));
-  }
-
-  // Upload events should be ignored.
-  {
-    ItemEvent event;
-    event.is_download = false;
-    event.stable_id = static_cast<int64_t>(id1);
-    event.path = path1.value();
-    event.state = ItemEvent::State::kInProgress;
-    event.bytes_to_transfer = 30000;
-    event.bytes_transferred = 7000;
-    EXPECT_FALSE(manager.OnSyncingEvent(event));
-  }
-
-  EXPECT_THAT(manager.files_to_track_, SizeIs(2));
-
-  {
-    const Progress progress = manager.GetProgress();
-    EXPECT_EQ(progress.syncing_files, 2);
-    EXPECT_EQ(progress.failed_files, 0);
-    EXPECT_EQ(progress.pinned_files, 0);
-    EXPECT_EQ(progress.pinned_bytes, 5000);
-    EXPECT_EQ(progress.bytes_to_pin, 30000);
-    EXPECT_EQ(progress.required_space, 24576);
-  }
-
-  {
-    const auto it = manager.files_to_track_.find(id1);
-    ASSERT_NE(it, manager.files_to_track_.end());
-    const auto& [id, file] = *it;
-    EXPECT_EQ(id, id1);
-    EXPECT_EQ(file.path, path1);
-    EXPECT_EQ(file.total, 10000);
-    EXPECT_EQ(file.transferred, 5000);
-    EXPECT_TRUE(file.pinned);
-    EXPECT_TRUE(file.in_progress);
-  }
-
-  // Mark file 1 as completed.
-  {
-    ItemEvent event;
-    event.is_download = true;
-    event.stable_id = static_cast<int64_t>(id1);
-    event.path = path1.value();
-    event.state = ItemEvent::State::kCompleted;
-    event.bytes_to_transfer = -1;
-    event.bytes_transferred = -1;
-    EXPECT_TRUE(manager.OnSyncingEvent(event));
-    EXPECT_FALSE(manager.OnSyncingEvent(event));
-  }
-
-  EXPECT_THAT(manager.files_to_track_, SizeIs(1));
-
-  {
-    const Progress progress = manager.GetProgress();
-    EXPECT_EQ(progress.syncing_files, 1);
-    EXPECT_EQ(progress.failed_files, 0);
-    EXPECT_EQ(progress.pinned_files, 1);
-    EXPECT_EQ(progress.pinned_bytes, 10000);
-    EXPECT_EQ(progress.bytes_to_pin, 30000);
-    EXPECT_EQ(progress.required_space, 20480);
-  }
-
-  {
-    const auto it = manager.files_to_track_.find(id1);
-    EXPECT_EQ(it, manager.files_to_track_.end());
-  }
-
-  // Mark file 2 as failed.
-  {
-    ItemEvent event;
-    event.is_download = true;
-    event.stable_id = static_cast<int64_t>(id2);
-    event.path = path2.value();
-    event.state = ItemEvent::State::kFailed;
-    event.bytes_to_transfer = -1;
-    event.bytes_transferred = -1;
-    EXPECT_TRUE(manager.OnSyncingEvent(event));
-    EXPECT_FALSE(manager.OnSyncingEvent(event));
-  }
-
-  EXPECT_THAT(manager.files_to_track_, IsEmpty());
-
-  {
-    const Progress progress = manager.GetProgress();
-    EXPECT_EQ(progress.syncing_files, 0);
-    EXPECT_EQ(progress.failed_files, 1);
-    EXPECT_EQ(progress.pinned_files, 1);
-    EXPECT_EQ(progress.pinned_bytes, 10000);
-    EXPECT_EQ(progress.bytes_to_pin, 10000);
-    EXPECT_EQ(progress.required_space, 0);
-  }
-
-  {
-    const auto it = manager.files_to_track_.find(id2);
-    EXPECT_EQ(it, manager.files_to_track_.end());
-  }
-}
-
-// Tests PinManager::OnSyncingStatusUpdate().
-TEST_F(DriveFsPinManagerTest, OnSyncingStatusUpdate) {
-  PinManager manager(profile_path_, mount_path_, &drivefs_, kMaxQueueSize);
-
-  DCHECK_CALLED_ON_VALID_SEQUENCE(manager.sequence_checker_);
-  manager.progress_.stage = Stage::kSyncing;
-  manager.progress_.bytes_to_pin = 30000;
-  manager.progress_.required_space = 32768;
-  manager.use_on_item_progress_ = false;
-
-  const Id id1 = Id(549);
-  const Path path1 = Path("Path 1");
-
-  const Id id2 = Id(17);
-  const Path path2 = Path("Path 2");
-
-  // Put in place a couple of files to track.
-  {
-    const auto [it, ok] = manager.files_to_track_.try_emplace(
-        id1, PinManager::File{.path = path1, .total = 10000, .pinned = true});
-    ASSERT_TRUE(ok);
-    manager.progress_.syncing_files++;
-  }
-  {
-    const auto [it, ok] = manager.files_to_track_.try_emplace(
-        id2, PinManager::File{.path = path2, .total = 20000, .pinned = true});
-    ASSERT_TRUE(ok);
-    manager.progress_.syncing_files++;
-  }
-
-  EXPECT_THAT(manager.files_to_track_, SizeIs(2));
-
-  // Expect `OnItemProgress` events are ignored.
-  {
-    ProgressEvent event;
-    event.stable_id = static_cast<int64_t>(id1);
-    event.file_path = path1;
-    event.progress = 20;
-    manager.OnItemProgress(event);
-  }
-
-  {
-    const Progress progress = manager.GetProgress();
-    EXPECT_EQ(manager.progress_.stage, Stage::kSyncing);
-    EXPECT_EQ(progress.syncing_files, 2);
-    EXPECT_EQ(progress.failed_files, 0);
-    EXPECT_EQ(progress.pinned_files, 0);
-    EXPECT_EQ(progress.pinned_bytes, 0);
-    EXPECT_EQ(progress.bytes_to_pin, 30000);
-    EXPECT_EQ(progress.required_space, 32768);
-    EXPECT_EQ(progress.useful_events, 0);
-    EXPECT_EQ(progress.duplicated_events, 0);
-  }
-
-  // Prepare a list of syncing status events.
-  SyncingStatus events;
-
-  {
-    // An event with an unknown type is ignored.
-    ItemEventPtr event = ItemEvent::New();
-    event->is_download = true;
-    event->stable_id = static_cast<int64_t>(id2);
-    event->path = path2.value();
-    event->state = ItemEvent::State(-1);
-    event->bytes_to_transfer = -1;
-    event->bytes_transferred = -1;
-    events.item_events.push_back(std::move(event));
-  }
-  {
-    // Mark file 1 as queued.
-    ItemEventPtr event = ItemEvent::New();
-    event->is_download = true;
-    event->stable_id = static_cast<int64_t>(id1);
-    event->path = path1.value();
-    event->state = ItemEvent::State::kQueued;
-    event->bytes_to_transfer = 10000;
-    event->bytes_transferred = 0;
-    events.item_events.push_back(ItemEvent::New(*event));
-    events.item_events.push_back(std::move(event));
-  }
-  {
-    // Mark file 1 as in progress.
-    ItemEventPtr event = ItemEvent::New();
-    event->is_download = true;
-    event->stable_id = static_cast<int64_t>(id1);
-    event->path = path1.value();
-    event->state = ItemEvent::State::kInProgress;
-    event->bytes_to_transfer = 10000;
-    event->bytes_transferred = 5000;
-    events.item_events.push_back(ItemEvent::New(*event));
-    events.item_events.push_back(std::move(event));
-  }
-  {
-    // Upload events should be ignored.
-    ItemEventPtr event = ItemEvent::New();
-    event->is_download = false;
-    event->stable_id = static_cast<int64_t>(id1);
-    event->path = path1.value();
-    event->state = ItemEvent::State::kInProgress;
-    event->bytes_to_transfer = 30000;
-    event->bytes_transferred = 7000;
-    events.item_events.push_back(ItemEvent::New(*event));
-    events.item_events.push_back(std::move(event));
-  }
-  {
-    // Mark file 1 as completed.
-    ItemEventPtr event = ItemEvent::New();
-    event->is_download = true;
-    event->stable_id = static_cast<int64_t>(id1);
-    event->path = path1.value();
-    event->state = ItemEvent::State::kCompleted;
-    event->bytes_to_transfer = -1;
-    event->bytes_transferred = -1;
-    events.item_events.push_back(ItemEvent::New(*event));
-    events.item_events.push_back(std::move(event));
-  }
-  {
-    // Mark file 2 as failed.
-    ItemEventPtr event = ItemEvent::New();
-    event->is_download = true;
-    event->stable_id = static_cast<int64_t>(id2);
-    event->path = path2.value();
-    event->state = ItemEvent::State::kFailed;
-    event->bytes_to_transfer = -1;
-    event->bytes_transferred = -1;
-    events.item_events.push_back(ItemEvent::New(*event));
-    events.item_events.push_back(std::move(event));
-  }
-
-  manager.OnSyncingStatusUpdate(std::as_const(events));
-
-  EXPECT_THAT(manager.files_to_track_, IsEmpty());
-
-  {
-    const Progress progress = manager.GetProgress();
-    EXPECT_EQ(manager.progress_.stage, Stage::kSyncing);
-    EXPECT_EQ(progress.syncing_files, 0);
-    EXPECT_EQ(progress.failed_files, 1);
-    EXPECT_EQ(progress.pinned_files, 1);
-    EXPECT_EQ(progress.pinned_bytes, 10000);
-    EXPECT_EQ(progress.bytes_to_pin, 10000);
-    EXPECT_EQ(progress.required_space, 0);
-    EXPECT_EQ(progress.useful_events, 3);
-    EXPECT_EQ(progress.duplicated_events, 8);
-  }
-
-  manager.Stop();
-
-  // Events received when the PinManager is stopped are ignored.
-  manager.OnSyncingStatusUpdate(std::as_const(events));
-
-  {
-    const Progress progress = manager.GetProgress();
-    EXPECT_EQ(manager.progress_.stage, Stage::kStopped);
-    EXPECT_EQ(progress.syncing_files, 0);
-    EXPECT_EQ(progress.failed_files, 1);
-    EXPECT_EQ(progress.pinned_files, 1);
-    EXPECT_EQ(progress.pinned_bytes, 10000);
-    EXPECT_EQ(progress.bytes_to_pin, 10000);
-    EXPECT_EQ(progress.required_space, 0);
-    EXPECT_EQ(progress.useful_events, 3);
-    EXPECT_EQ(progress.duplicated_events, 8);
-  }
-}
-
 // Tests PinManager::OnItemProgress().
 TEST_F(DriveFsPinManagerTest, OnItemProgress) {
   PinManager manager(profile_path_, mount_path_, &drivefs_, kMaxQueueSize);
 
   DCHECK_CALLED_ON_VALID_SEQUENCE(manager.sequence_checker_);
-  manager.use_on_item_progress_ = true;
   manager.progress_.bytes_to_pin = 30000;
   manager.progress_.required_space = 32768;
   manager.progress_.stage = Stage::kSyncing;