[M112][DownloadBubble] Pop partial view when item viewed in main view…

… finishes

This CL marks the item as "actioned on" only if it is finished when
viewed in the main view. Previously, opening the main view with an
ongoing download would mark that downloads as "actioned on", so it would
not pop the partial bubble upon completion. This is bad because the user
might forget about a large download. After this change, if the item is
still in progress when the main view is opened, it is not marked as
"actioned on" and will still trigger the partial view upon completion
(if there are no other in-progress downloads).

(cherry picked from commit fcb377e)

Bug: 1419000
Change-Id: I51057827f000fe12bfa0c2e31dda17b3bf9a8324
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4291368
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1109899}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4294542
Auto-Submit: Lily Chen <chlily@chromium.org>
Commit-Queue: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/branch-heads/5615@{#38}
Cr-Branched-From: 9c6408e-refs/heads/main@{#1109224}

chrome/browser/BUILD.gn

@@ -3611,6 +3611,8 @@ static_library("browser") {
       "download/bubble/download_bubble_controller.h",
       "download/bubble/download_bubble_prefs.cc",
       "download/bubble/download_bubble_prefs.h",
+      "download/bubble/download_bubble_ui_model_utils.cc",
+      "download/bubble/download_bubble_ui_model_utils.h",
       "download/bubble/download_display.cc",
       "download/bubble/download_display.h",
       "download/bubble/download_display_controller.cc",

chrome/browser/download/bubble/download_bubble_controller.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_index/content_index_provider_impl.h"
 #include "chrome/browser/download/bubble/download_bubble_prefs.h"
+#include "chrome/browser/download/bubble/download_bubble_ui_model_utils.h"
 #include "chrome/browser/download/bubble/download_display_controller.h"
 #include "chrome/browser/download/chrome_download_manager_delegate.h"
 #include "chrome/browser/download/download_core_service.h"
@@ -47,18 +48,6 @@ bool FindOfflineItemByContentId(const ContentId& to_find,
   return candidate.id == to_find;
 }
 
-bool DownloadUIModelIsRecent(const DownloadUIModel* model,
-                             base::Time cutoff_time) {
-  return ((model->GetStartTime().is_null() && !model->IsDone()) ||
-          model->GetStartTime() > cutoff_time);
-}
-
-bool IsPendingDeepScanning(const DownloadUIModel& model) {
-  return model.GetState() == download::DownloadItem::IN_PROGRESS &&
-         model.GetDangerType() ==
-             download::DOWNLOAD_DANGER_TYPE_PROMPT_FOR_SCANNING;
-}
-
 using DownloadUIModelPtrList = std::list<DownloadUIModelPtr>;
 
 // Sorting order is 1) Active in-progress downloads, 2) Paused in-progress
@@ -262,7 +251,7 @@ void DownloadBubbleUIController::OnItemUpdated(
   bool was_added = MaybeAddOfflineItem(item, /*is_new=*/false);
   OfflineItemModel model(offline_manager_, item);
   display_controller_->OnUpdatedItem(
-      model.IsDone(), IsPendingDeepScanning(model),
+      model.IsDone(), IsPendingDeepScanning(&model),
       was_added &&
           (browser_ == chrome::FindLastActiveWithProfile(profile_.get())));
 }
@@ -275,15 +264,15 @@ void DownloadBubbleUIController::OnDownloadUpdated(
   DownloadItemModel model(item);
   if (manager != download_notifier_.GetManager()) {
     display_controller_->OnUpdatedItem(item->IsDone(),
-                                       IsPendingDeepScanning(model),
+                                       IsPendingDeepScanning(&model),
                                        /*may_show_details=*/false);
     return;
   }
   bool may_show_details =
       model.ShouldShowInBubble() &&
       (browser_ == chrome::FindLastActiveWithProfile(profile_.get()));
   display_controller_->OnUpdatedItem(
-      item->IsDone(), IsPendingDeepScanning(model), may_show_details);
+      item->IsDone(), IsPendingDeepScanning(&model), may_show_details);
 }
 
 void DownloadBubbleUIController::PruneOfflineItems() {
@@ -361,8 +350,9 @@ std::vector<DownloadUIModelPtr> DownloadBubbleUIController::GetDownloadUIModels(
     if (!is_main_view && model->WasActionedOn()) {
       continue;
     }
-    // Partial view entries are removed if viewed on the main view.
-    if (is_main_view) {
+    // Partial view entries are removed if viewed on the main view after
+    // completion.
+    if (is_main_view && !IsModelInProgress(model.get())) {
       model->SetActionedOn(true);
     }
     items_to_return.push_back(std::move(model));

chrome/browser/download/bubble/download_bubble_controller_unittest.cc

@@ -194,6 +194,7 @@ class DownloadBubbleUIControllerTest : public testing::Test {
         .WillRepeatedly(Return(download::DownloadItem::DownloadCreationType::
                                    TYPE_ACTIVE_DOWNLOAD));
     EXPECT_CALL(item(index), IsPaused()).WillRepeatedly(Return(false));
+    EXPECT_CALL(item(index), IsDangerous()).WillRepeatedly(Return(false));
     // Functions called when checking ShouldShowDownloadStartedAnimation().
     EXPECT_CALL(item(index), IsSavePackageDownload())
         .WillRepeatedly(Return(false));
@@ -434,7 +435,7 @@ TEST_F(DownloadBubbleUIControllerTest, ListIsCappedAndMostRecent) {
 }
 
 TEST_F(DownloadBubbleUIControllerTest,
-       OpeningMainViewRemovesEntryFromPartialView) {
+       OpeningMainViewRemovesCompletedEntryFromPartialView) {
   std::vector<std::string> ids = {"Download 1", "Offline 1"};
   InitDownloadItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
                    download::DownloadItem::IN_PROGRESS, ids[0]);
@@ -443,11 +444,30 @@ TEST_F(DownloadBubbleUIControllerTest,
   EXPECT_EQ(controller().GetPartialView().size(), 2ul);
   EXPECT_EQ(second_controller().GetPartialView().size(), 2ul);
 
-  EXPECT_EQ(controller().GetMainView().size(), 2ul);
+  UpdateDownloadItem(/*item_index=*/0, DownloadState::COMPLETE);
+  // Completed offline item is removed.
+  UpdateOfflineItem(/*item_index=*/0, OfflineItemState::COMPLETE);
+  EXPECT_EQ(controller().GetMainView().size(), 1ul);
+  // Download was removed from partial view because it is completed.
   EXPECT_EQ(controller().GetPartialView().size(), 0ul);
   EXPECT_EQ(second_controller().GetPartialView().size(), 0ul);
 }
 
+TEST_F(DownloadBubbleUIControllerTest,
+       OpeningMainViewDoesNotRemoveInProgressEntryFromPartialView) {
+  std::vector<std::string> ids = {"Download 1", "Offline 1"};
+  InitDownloadItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
+                   download::DownloadItem::IN_PROGRESS, ids[0]);
+  InitOfflineItem(OfflineItemState::IN_PROGRESS, ids[1]);
+
+  EXPECT_EQ(controller().GetPartialView().size(), 2ul);
+
+  // This does not remove the entries from the partial view because the items
+  // are in progress.
+  EXPECT_EQ(controller().GetMainView().size(), 2ul);
+  EXPECT_EQ(controller().GetPartialView().size(), 2ul);
+}
+
 // Tests that no items are returned (i.e. no partial view will be shown) if it
 // is too soon since the last partial view has been shown.
 TEST_F(DownloadBubbleUIControllerTest, NoItemsReturnedForPartialViewTooSoon) {
@@ -457,7 +477,7 @@ TEST_F(DownloadBubbleUIControllerTest, NoItemsReturnedForPartialViewTooSoon) {
   // First time showing the partial view should work.
   EXPECT_CALL(display_controller(), OnNewItem(true)).Times(1);
   InitDownloadItem(FILE_PATH_LITERAL("/foo/bar1.pdf"),
-                   download::DownloadItem::IN_PROGRESS, ids[0]);
+                   download::DownloadItem::COMPLETE, ids[0]);
   EXPECT_EQ(controller().GetPartialView().size(), 1u);
 
   // No items are returned for a partial view because it is too soon.

chrome/browser/download/bubble/download_bubble_ui_model_utils.cc

@@ -0,0 +1,27 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/download/bubble/download_bubble_ui_model_utils.h"
+
+#include "base/time/time.h"
+#include "chrome/browser/download/download_ui_model.h"
+
+bool DownloadUIModelIsRecent(const DownloadUIModel* model,
+                             base::Time cutoff_time) {
+  return ((model->GetStartTime().is_null() && !model->IsDone()) ||
+          model->GetStartTime() > cutoff_time);
+}
+
+bool IsPendingDeepScanning(const DownloadUIModel* model) {
+  return model->GetState() == download::DownloadItem::IN_PROGRESS &&
+         model->GetDangerType() ==
+             download::DOWNLOAD_DANGER_TYPE_PROMPT_FOR_SCANNING;
+}
+
+bool IsModelInProgress(const DownloadUIModel* model) {
+  if (model->IsDangerous() && !IsPendingDeepScanning(model)) {
+    return false;
+  }
+  return model->GetState() == download::DownloadItem::IN_PROGRESS;
+}

chrome/browser/download/bubble/download_bubble_ui_model_utils.h

@@ -0,0 +1,24 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_DOWNLOAD_BUBBLE_DOWNLOAD_BUBBLE_UI_MODEL_UTILS_H_
+#define CHROME_BROWSER_DOWNLOAD_BUBBLE_DOWNLOAD_BUBBLE_UI_MODEL_UTILS_H_
+
+#include "base/time/time.h"
+#include "chrome/browser/download/download_ui_model.h"
+
+// Whether the download is more recent than |cutoff_time|.
+bool DownloadUIModelIsRecent(const DownloadUIModel* model,
+                             base::Time cutoff_time);
+
+// Whether the download is in progress and pending deep scanning.
+bool IsPendingDeepScanning(const DownloadUIModel* model);
+
+// Whether the download is considered in-progress from the UI's point of view.
+// Consider dangerous downloads as completed, because we don't want to encourage
+// users to interact with them. However, consider downloads pending scanning as
+// in progress, because we do want users to scan potential dangerous downloads.
+bool IsModelInProgress(const DownloadUIModel* model);
+
+#endif  // CHROME_BROWSER_DOWNLOAD_BUBBLE_DOWNLOAD_BUBBLE_UI_MODEL_UTILS_H_

chrome/browser/download/bubble/download_display_controller.cc

@@ -10,6 +10,7 @@
 #include "base/timer/timer.h"
 #include "chrome/browser/download/bubble/download_bubble_controller.h"
 #include "chrome/browser/download/bubble/download_bubble_prefs.h"
+#include "chrome/browser/download/bubble/download_bubble_ui_model_utils.h"
 #include "chrome/browser/download/bubble/download_display.h"
 #include "chrome/browser/download/bubble/download_icon_state.h"
 #include "chrome/browser/download/download_core_service.h"
@@ -61,19 +62,6 @@ struct AllDownloadUIModelsInfo {
   bool all_done = true;
 };
 
-bool IsPendingDeepScanning(const DownloadUIModel* model) {
-  return model->GetState() == download::DownloadItem::IN_PROGRESS &&
-         model->GetDangerType() ==
-             download::DOWNLOAD_DANGER_TYPE_PROMPT_FOR_SCANNING;
-}
-
-bool IsModelInProgress(const DownloadUIModel* model) {
-  if (model->IsDangerous() && !IsPendingDeepScanning(model)) {
-    return false;
-  }
-  return model->GetState() == download::DownloadItem::IN_PROGRESS;
-}
-
 AllDownloadUIModelsInfo GetAllModelsInfo(
     std::vector<std::unique_ptr<DownloadUIModel>>& all_models) {
   AllDownloadUIModelsInfo info;