Misc. cleanup for some downloads-related classes.

Bug: none
Change-Id: I7da24a70506ea2b029e34b3bbe4804aa4b5a6b78
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2248879
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798040}

chrome/browser/download/download_item_model_unittest.cc

@@ -178,11 +178,10 @@ TEST_F(DownloadItemModelTest, InterruptedStatus) {
                 "interrupt reason mismatch");
 
   SetupDownloadItemDefaults();
-  for (unsigned i = 0; i < base::size(kTestCases); ++i) {
-    const TestCase& test_case = kTestCases[i];
+  for (const auto& test_case : kTestCases) {
     SetupInterruptedDownloadItem(test_case.reason);
-    EXPECT_STREQ(test_case.expected_status,
-                 base::UTF16ToUTF8(model().GetStatusText()).c_str());
+    EXPECT_EQ(test_case.expected_status,
+              base::UTF16ToUTF8(model().GetStatusText()));
   }
 }
 
@@ -257,11 +256,10 @@ TEST_F(DownloadItemModelTest, InterruptTooltip) {
                 "interrupt reason mismatch");
 
   SetupDownloadItemDefaults();
-  for (unsigned i = 0; i < base::size(kTestCases); ++i) {
-    const TestCase& test_case = kTestCases[i];
+  for (const auto& test_case : kTestCases) {
     SetupInterruptedDownloadItem(test_case.reason);
-    EXPECT_STREQ(test_case.expected_tooltip,
-                 base::UTF16ToUTF8(model().GetTooltipText()).c_str());
+    EXPECT_EQ(test_case.expected_tooltip,
+              base::UTF16ToUTF8(model().GetTooltipText()));
   }
 }
 
@@ -315,8 +313,7 @@ TEST_F(DownloadItemModelTest, InProgressStatus) {
 
   SetupDownloadItemDefaults();
 
-  for (unsigned i = 0; i < base::size(kTestCases); i++) {
-    const TestCase& test_case = kTestCases[i];
+  for (const auto& test_case : kTestCases) {
     Mock::VerifyAndClearExpectations(&item());
     Mock::VerifyAndClearExpectations(&model());
     EXPECT_CALL(item(), GetReceivedBytes())
@@ -332,8 +329,8 @@ TEST_F(DownloadItemModelTest, InProgressStatus) {
     EXPECT_CALL(item(), IsPaused())
         .WillRepeatedly(Return(test_case.is_paused));
 
-    EXPECT_STREQ(test_case.expected_status,
-                 base::UTF16ToUTF8(model().GetStatusText()).c_str());
+    EXPECT_EQ(test_case.expected_status,
+              base::UTF16ToUTF8(model().GetStatusText()));
   }
 }
 
@@ -433,8 +430,7 @@ TEST_F(DownloadItemModelTest, ShouldRemoveFromShelfWhenComplete) {
 
   SetupDownloadItemDefaults();
 
-  for (unsigned i = 0; i < base::size(kTestCases); i++) {
-    const TestCase& test_case = kTestCases[i];
+  for (const auto& test_case : kTestCases) {
     EXPECT_CALL(item(), GetOpenWhenComplete())
         .WillRepeatedly(Return(test_case.is_auto_open));
     EXPECT_CALL(item(), GetState())
@@ -445,8 +441,7 @@ TEST_F(DownloadItemModelTest, ShouldRemoveFromShelfWhenComplete) {
         .WillRepeatedly(Return(test_case.auto_opened));
 
     EXPECT_EQ(test_case.expected_result,
-              model().ShouldRemoveFromShelfWhenComplete())
-        << "Test case: " << i;
+              model().ShouldRemoveFromShelfWhenComplete());
     Mock::VerifyAndClearExpectations(&item());
     Mock::VerifyAndClearExpectations(&model());
   }
@@ -490,16 +485,14 @@ TEST_F(DownloadItemModelTest, ShouldShowDropdown) {
 
   SetupDownloadItemDefaults();
 
-  for (unsigned i = 0; i < base::size(kTestCases); i++) {
-    const TestCase& test_case = kTestCases[i];
+  for (const auto& test_case : kTestCases) {
     EXPECT_CALL(item(), GetState()).WillRepeatedly(Return(test_case.state));
     EXPECT_CALL(item(), GetDangerType())
         .WillRepeatedly(Return(test_case.danger_type));
     EXPECT_CALL(item(), IsDangerous())
         .WillRepeatedly(Return(test_case.is_dangerous));
 
-    EXPECT_EQ(test_case.expected_result, model().ShouldShowDropdown())
-        << "Test case: " << i;
+    EXPECT_EQ(test_case.expected_result, model().ShouldShowDropdown());
     Mock::VerifyAndClearExpectations(&item());
     Mock::VerifyAndClearExpectations(&model());
   }

chrome/browser/download/download_shelf.cc

@@ -33,17 +33,10 @@
 #include "content/public/browser/web_contents.h"
 #include "ui/gfx/animation/animation.h"
 
-namespace {
-
-// Delay before we show a transient download.
-const int64_t kDownloadShowDelayInSeconds = 2;
-
-}  // namespace
-
 DownloadShelf::DownloadShelf(Browser* browser, Profile* profile)
     : browser_(browser), profile_(profile) {}
 
-DownloadShelf::~DownloadShelf() {}
+DownloadShelf::~DownloadShelf() = default;
 
 void DownloadShelf::AddDownload(DownloadUIModel::DownloadUIModelPtr model) {
   DCHECK(model);
@@ -96,7 +89,7 @@ void DownloadShelf::Unhide() {
 }
 
 base::TimeDelta DownloadShelf::GetTransientDownloadShowDelay() const {
-  return base::TimeDelta::FromSeconds(kDownloadShowDelayInSeconds);
+  return base::TimeDelta::FromSeconds(2);
 }
 
 void DownloadShelf::ShowDownload(DownloadUIModel::DownloadUIModelPtr download) {
@@ -108,65 +101,54 @@ void DownloadShelf::ShowDownload(DownloadUIModel::DownloadUIModelPtr download) {
            ->IsShelfEnabled())
     return;
 
-  bool should_show_download_started_animation =
-      download->ShouldShowDownloadStartedAnimation();
-
-  if (is_hidden_)
-    Unhide();
+  Unhide();
   Open();
-  DoShowDownload(std::move(download));
 
-  // browser_ can be null for tests.
-  if (!browser_)
-    return;
+  const bool should_show_download_started_animation =
+      download->ShouldShowDownloadStartedAnimation();
+  DoShowDownload(std::move(download));
 
   // Show the download started animation if:
   // - Download started animation is enabled for this download. It is disabled
   //   for "Save As" downloads and extension installs, for example.
+  // - Rich animations are enabled.
   // - The browser has an active visible WebContents. (browser isn't minimized,
   //   or running under a test etc.)
-  // - Rich animations are enabled.
-  content::WebContents* shelf_tab =
+  if (!should_show_download_started_animation ||
+      !gfx::Animation::ShouldRenderRichAnimation() || !browser_)
+    return;
+  content::WebContents* const shelf_tab =
       browser_->tab_strip_model()->GetActiveWebContents();
-  if (should_show_download_started_animation && shelf_tab &&
-      platform_util::IsVisible(shelf_tab->GetNativeView()) &&
-      gfx::Animation::ShouldRenderRichAnimation()) {
+  if (shelf_tab && platform_util::IsVisible(shelf_tab->GetNativeView()))
     DownloadStartedAnimation::Show(shelf_tab);
-  }
 }
 
 void DownloadShelf::ShowDownloadById(
     const offline_items_collection::ContentId& id) {
   if (OfflineItemUtils::IsDownload(id)) {
-    content::DownloadManager* download_manager =
-        content::BrowserContext::GetDownloadManager(profile_);
-    if (!download_manager)
-      return;
-
-    download::DownloadItem* download =
-        download_manager->GetDownloadByGuid(id.id);
-    if (!download)
-      return;
-
-    ShowDownload(DownloadItemModel::Wrap(download));
+    auto* const manager =
+        content::BrowserContext::GetDownloadManager(profile());
+    if (manager) {
+      auto* const download = manager->GetDownloadByGuid(id.id);
+      if (download)
+        ShowDownload(DownloadItemModel::Wrap(download));
+    }
   } else {
-    offline_items_collection::OfflineContentAggregator* aggregator =
-        OfflineContentAggregatorFactory::GetForKey(profile_->GetProfileKey());
-    if (!aggregator)
-      return;
-
-    aggregator->GetItemById(
-        id, base::BindOnce(&DownloadShelf::OnGetDownloadDoneForOfflineItem,
-                           weak_ptr_factory_.GetWeakPtr()));
+    auto* const aggregator =
+        OfflineContentAggregatorFactory::GetForKey(profile()->GetProfileKey());
+    if (aggregator) {
+      aggregator->GetItemById(
+          id, base::BindOnce(&DownloadShelf::OnGetDownloadDoneForOfflineItem,
+                             weak_ptr_factory_.GetWeakPtr()));
+    }
   }
 }
 
 void DownloadShelf::OnGetDownloadDoneForOfflineItem(
     const base::Optional<offline_items_collection::OfflineItem>& item) {
-  if (!item.has_value())
-    return;
-
-  OfflineItemModelManager* manager =
-      OfflineItemModelManagerFactory::GetForBrowserContext(profile_);
-  ShowDownload(OfflineItemModel::Wrap(manager, item.value()));
+  if (item.has_value()) {
+    auto* const manager =
+        OfflineItemModelManagerFactory::GetForBrowserContext(profile());
+    ShowDownload(OfflineItemModel::Wrap(manager, item.value()));
+  }
 }

chrome/browser/download/download_ui_model.cc

@@ -200,39 +200,28 @@ base::string16 DownloadUIModel::GetInterruptReasonText() const {
 }
 
 base::string16 DownloadUIModel::GetStatusText() const {
-  base::string16 status_text;
   switch (GetState()) {
     case DownloadItem::IN_PROGRESS:
-      status_text = GetInProgressStatusString();
-      break;
+      return GetInProgressStatusString();
     case DownloadItem::COMPLETE:
-      if (GetFileExternallyRemoved()) {
-        status_text = l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_REMOVED);
-      } else {
-        status_text.clear();
-      }
-      break;
-    case DownloadItem::CANCELLED:
-      status_text = l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_CANCELLED);
-      break;
+      return GetFileExternallyRemoved()
+                 ? l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_REMOVED)
+                 : base::string16();
     case DownloadItem::INTERRUPTED: {
-      FailState fail_state = GetLastFailState();
+      const FailState fail_state = GetLastFailState();
       if (fail_state != FailState::USER_CANCELED) {
-        base::string16 message =
-            OfflineItemUtils::GetFailStateMessage(fail_state);
-        status_text = l10n_util::GetStringFUTF16(
-            IDS_DOWNLOAD_STATUS_INTERRUPTED, message);
-      } else {
-        // Same as DownloadItem::CANCELLED.
-        status_text = l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_CANCELLED);
+        return l10n_util::GetStringFUTF16(
+            IDS_DOWNLOAD_STATUS_INTERRUPTED,
+            OfflineItemUtils::GetFailStateMessage(fail_state));
       }
-      break;
     }
-    default:
+      FALLTHROUGH;
+    case DownloadItem::CANCELLED:
+      return l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_CANCELLED);
+    case DownloadItem::MAX_DOWNLOAD_STATE:
       NOTREACHED();
+      return base::string16();
   }
-
-  return status_text;
 }
 
 base::string16 DownloadUIModel::GetTooltipText() const {
@@ -249,23 +238,18 @@ base::string16 DownloadUIModel::GetWarningText(const base::string16& filename,
                                                size_t* offset) const {
   *offset = std::string::npos;
   switch (GetDangerType()) {
-    case download::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL: {
+    case download::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL:
       return l10n_util::GetStringUTF16(IDS_PROMPT_MALICIOUS_DOWNLOAD_URL);
-    }
-    case download::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE: {
-      if (IsExtensionDownload()) {
-        return l10n_util::GetStringUTF16(
-            IDS_PROMPT_DANGEROUS_DOWNLOAD_EXTENSION);
-      } else {
-        return l10n_util::GetStringFUTF16(IDS_PROMPT_DANGEROUS_DOWNLOAD,
-                                          filename, offset);
-      }
-    }
+    case download::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE:
+      return IsExtensionDownload()
+                 ? l10n_util::GetStringUTF16(
+                       IDS_PROMPT_DANGEROUS_DOWNLOAD_EXTENSION)
+                 : l10n_util::GetStringFUTF16(IDS_PROMPT_DANGEROUS_DOWNLOAD,
+                                              filename, offset);
     case download::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT:
-    case download::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST: {
+    case download::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST:
       return l10n_util::GetStringFUTF16(IDS_PROMPT_MALICIOUS_DOWNLOAD_CONTENT,
                                         filename, offset);
-    }
     case download::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT: {
       bool request_ap_verdicts = false;
 #if BUILDFLAG(FULL_SAFE_BROWSING)
@@ -280,30 +264,24 @@ base::string16 DownloadUIModel::GetWarningText(const base::string16& filename,
               : IDS_PROMPT_UNCOMMON_DOWNLOAD_CONTENT,
           filename, offset);
     }
-    case download::DOWNLOAD_DANGER_TYPE_POTENTIALLY_UNWANTED: {
+    case download::DOWNLOAD_DANGER_TYPE_POTENTIALLY_UNWANTED:
       return l10n_util::GetStringFUTF16(IDS_PROMPT_DOWNLOAD_CHANGES_SETTINGS,
                                         filename, offset);
-    }
-    case download::DOWNLOAD_DANGER_TYPE_BLOCKED_TOO_LARGE: {
+    case download::DOWNLOAD_DANGER_TYPE_BLOCKED_TOO_LARGE:
       return l10n_util::GetStringFUTF16(IDS_PROMPT_DOWNLOAD_BLOCKED_TOO_LARGE,
                                         filename, offset);
-    }
-    case download::DOWNLOAD_DANGER_TYPE_BLOCKED_PASSWORD_PROTECTED: {
+    case download::DOWNLOAD_DANGER_TYPE_BLOCKED_PASSWORD_PROTECTED:
       return l10n_util::GetStringFUTF16(
           IDS_PROMPT_DOWNLOAD_BLOCKED_PASSWORD_PROTECTED, filename, offset);
-    }
-    case download::DOWNLOAD_DANGER_TYPE_SENSITIVE_CONTENT_WARNING: {
+    case download::DOWNLOAD_DANGER_TYPE_SENSITIVE_CONTENT_WARNING:
       return l10n_util::GetStringFUTF16(
           IDS_PROMPT_DOWNLOAD_SENSITIVE_CONTENT_WARNING, filename, offset);
-    }
-    case download::DOWNLOAD_DANGER_TYPE_SENSITIVE_CONTENT_BLOCK: {
+    case download::DOWNLOAD_DANGER_TYPE_SENSITIVE_CONTENT_BLOCK:
       return l10n_util::GetStringFUTF16(
           IDS_PROMPT_DOWNLOAD_SENSITIVE_CONTENT_BLOCKED, filename, offset);
-    }
-    case download::DOWNLOAD_DANGER_TYPE_PROMPT_FOR_SCANNING: {
+    case download::DOWNLOAD_DANGER_TYPE_PROMPT_FOR_SCANNING:
       return l10n_util::GetStringFUTF16(IDS_PROMPT_APP_DEEP_SCANNING, filename,
                                         offset);
-    }
     case download::DOWNLOAD_DANGER_TYPE_BLOCKED_UNSUPPORTED_FILETYPE:
     case download::DOWNLOAD_DANGER_TYPE_DEEP_SCANNED_SAFE:
     case download::DOWNLOAD_DANGER_TYPE_DEEP_SCANNED_OPENED_DANGEROUS:
@@ -312,9 +290,8 @@ base::string16 DownloadUIModel::GetWarningText(const base::string16& filename,
     case download::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT:
     case download::DOWNLOAD_DANGER_TYPE_USER_VALIDATED:
     case download::DOWNLOAD_DANGER_TYPE_WHITELISTED_BY_POLICY:
-    case download::DOWNLOAD_DANGER_TYPE_MAX: {
+    case download::DOWNLOAD_DANGER_TYPE_MAX:
       break;
-    }
   }
 
   switch (GetMixedContentStatus()) {
@@ -335,12 +312,11 @@ base::string16 DownloadUIModel::GetWarningText(const base::string16& filename,
 }
 
 base::string16 DownloadUIModel::GetWarningConfirmButtonText() const {
-  if (GetDangerType() == download::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE &&
-      IsExtensionDownload()) {
-    return l10n_util::GetStringUTF16(IDS_CONTINUE_EXTENSION_DOWNLOAD);
-  } else {
-    return l10n_util::GetStringUTF16(IDS_CONFIRM_DOWNLOAD);
-  }
+  const auto kDangerousFile = download::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE;
+  return l10n_util::GetStringUTF16(
+      (GetDangerType() == kDangerousFile && IsExtensionDownload())
+          ? IDS_CONTINUE_EXTENSION_DOWNLOAD
+          : IDS_CONFIRM_DOWNLOAD);
 }
 
 ContentId DownloadUIModel::GetContentId() const {

chrome/browser/ui/views/download/download_item_view.cc

@@ -160,8 +160,8 @@ class TransparentButton : public views::Button {
   SkColor GetInkDropBaseColor() const override {
     // This button will be used like a LabelButton, so use the same foreground
     // base color as a label button.
-    return color_utils::DeriveDefaultIconColor(GetNativeTheme()->GetSystemColor(
-        ui::NativeTheme::kColorId_LabelEnabledColor));
+    return color_utils::DeriveDefaultIconColor(views::style::GetColor(
+        *this, views::style::CONTEXT_BUTTON, views::style::STYLE_PRIMARY));
   }
 
   // Forward dragging and capture loss events, since this class doesn't have
@@ -562,12 +562,11 @@ void DownloadItemView::OnPaintBackground(gfx::Canvas* canvas) {
 
   // Draw the separator as part of the background. It will be covered by the
   // focus ring when the view has focus.
-  const int end_x = base::i18n::IsRTL() ? 0 : width() - 1;
-  const SkColor separator_color = GetThemeProvider()->GetColor(
-      ThemeProperties::COLOR_TOOLBAR_VERTICAL_SEPARATOR);
-  canvas->DrawLine(gfx::Point(end_x, kTopBottomPadding),
-                   gfx::Point(end_x, height() - kTopBottomPadding),
-                   separator_color);
+  gfx::Rect rect(0, 0, 1, height());
+  rect.Inset(0, kTopBottomPadding);
+  canvas->FillRect(GetMirroredRect(rect),
+                   GetThemeProvider()->GetColor(
+                       ThemeProperties::COLOR_TOOLBAR_VERTICAL_SEPARATOR));
 }
 
 void DownloadItemView::OnPaint(gfx::Canvas* canvas) {
@@ -584,10 +583,9 @@ void DownloadItemView::OnPaint(gfx::Canvas* canvas) {
 
   // Paint download progress.
   // TODO(pkasting): Use a child view to display this.
-  int progress_x = base::i18n::IsRTL()
-                       ? width() - kStartPadding - kProgressIndicatorSize
-                       : kStartPadding;
-  int progress_y = CenterY(kProgressIndicatorSize);
+  const int progress_x =
+      GetMirroredXWithWidthInView(kStartPadding, kProgressIndicatorSize);
+  const int progress_y = CenterY(kProgressIndicatorSize);
   const gfx::RectF progress_bounds(
       progress_x, progress_y, kProgressIndicatorSize, kProgressIndicatorSize);
   const download::DownloadItem::DownloadState state = model_->GetState();
@@ -703,7 +701,7 @@ void DownloadItemView::UpdateMode(Mode mode) {
   if (mode_ == Mode::kNormal) {
     UpdateAccessibleAlertAndTimersForNormalMode();
   } else if (is_download_warning(mode_)) {
-    download::DownloadDangerType danger_type = model_->GetDangerType();
+    const auto danger_type = model_->GetDangerType();
     RecordDangerousDownloadWarningShown(danger_type);
     announce_accessible_alert_soon_ = true;
     if (danger_type == download::DOWNLOAD_DANGER_TYPE_PROMPT_FOR_SCANNING) {
@@ -880,7 +878,7 @@ base::string16 DownloadItemView::GetInProgressAccessibleAlertText() const {
     if (remaining.is_zero())
       return base::string16();
 
-    base::string16 remaining_string =
+    const base::string16 remaining_string =
         ui::TimeFormat::Simple(ui::TimeFormat::FORMAT_REMAINING,
                                ui::TimeFormat::LENGTH_SHORT, remaining);
     return l10n_util::GetStringFUTF16(
@@ -896,11 +894,10 @@ base::string16 DownloadItemView::GetInProgressAccessibleAlertText() const {
   }
 
   // Percent remaining is also unknown, announce bytes to download.
-  base::string16 file_name =
-      model_->GetFileNameToReportUser().LossyDisplayName();
   return l10n_util::GetStringFUTF16(
       IDS_DOWNLOAD_STATUS_IN_PROGRESS_ACCESSIBLE_ALERT,
-      ui::FormatBytes(model_->GetTotalBytes()), file_name);
+      ui::FormatBytes(model_->GetTotalBytes()),
+      model_->GetFileNameToReportUser().LossyDisplayName());
 }
 
 void DownloadItemView::AnnounceAccessibleAlert() {
@@ -1104,10 +1101,7 @@ int DownloadItemView::GetLabelWidth(const views::StyledLabel& label) const {
 void DownloadItemView::SetDropdownPressed(bool pressed) {
   if (dropdown_pressed_ != pressed) {
     dropdown_pressed_ = pressed;
-    dropdown_button_->AnimateInkDrop(dropdown_pressed_
-                                         ? views::InkDropState::ACTIVATED
-                                         : views::InkDropState::DEACTIVATED,
-                                     nullptr);
+    dropdown_button_->SetHighlighted(dropdown_pressed_);
     UpdateDropdownButtonImage();
   }
 }
@@ -1171,20 +1165,16 @@ void DownloadItemView::OpenDownloadDuringAsyncScanning() {
 bool DownloadItemView::SubmitDownloadToFeedbackService(
     DownloadCommands::Command command) const {
 #if BUILDFLAG(FULL_SAFE_BROWSING)
-  safe_browsing::SafeBrowsingService* sb_service =
-      g_browser_process->safe_browsing_service();
+  auto* const sb_service = g_browser_process->safe_browsing_service();
   if (!sb_service)
     return false;
-  safe_browsing::DownloadProtectionService* dp_service =
-      sb_service->download_protection_service();
+  auto* const dp_service = sb_service->download_protection_service();
   if (!dp_service)
     return false;
   // TODO(shaktisahu): Enable feedback service for offline item.
-  if (model_->download()) {
-    return dp_service->MaybeBeginFeedbackForDownload(
-        shelf_->browser()->profile(), model_->download(), command);
-  }
-  return true;
+  return !model_->download() ||
+         dp_service->MaybeBeginFeedbackForDownload(shelf_->browser()->profile(),
+                                                   model_->download(), command);
 #else
   NOTREACHED();
   return false;

chrome/browser/ui/views/download/download_shelf_context_menu_view.cc

@@ -25,7 +25,7 @@ void DownloadShelfContextMenuView::Run(
     views::Widget* parent_widget,
     const gfx::Rect& rect,
     ui::MenuSourceType source_type,
-    const base::Closure& on_menu_closed_callback) {
+    base::RepeatingClosure on_menu_closed_callback) {
   using Position = views::MenuAnchorPosition;
   ui::MenuModel* menu_model = GetMenuModel();
   // Run() should not be getting called if the DownloadItem was destroyed.
@@ -35,7 +35,8 @@ void DownloadShelfContextMenuView::Run(
       menu_model,
       views::MenuRunner::HAS_MNEMONICS | views::MenuRunner::CONTEXT_MENU,
       base::BindRepeating(&DownloadShelfContextMenuView::OnMenuClosed,
-                          base::Unretained(this), on_menu_closed_callback));
+                          base::Unretained(this),
+                          std::move(on_menu_closed_callback)));
 
   // The menu's alignment is determined based on the UI layout.
   Position position;
@@ -48,7 +49,7 @@ void DownloadShelfContextMenuView::Run(
 }
 
 void DownloadShelfContextMenuView::OnMenuClosed(
-    const base::Closure& on_menu_closed_callback) {
+    base::RepeatingClosure on_menu_closed_callback) {
   close_time_ = base::TimeTicks::Now();
 
   // This must be run before clearing |menu_runner_| who owns the reference.

chrome/browser/ui/views/download/download_shelf_context_menu_view.h

@@ -38,11 +38,11 @@ class DownloadShelfContextMenuView : public DownloadShelfContextMenu {
   void Run(views::Widget* parent_widget,
            const gfx::Rect& rect,
            ui::MenuSourceType source_type,
-           const base::Closure& on_menu_closed_callback);
+           base::RepeatingClosure on_menu_closed_callback);
 
  private:
   // Callback for MenuRunner.
-  void OnMenuClosed(const base::Closure& on_menu_closed_callback);
+  void OnMenuClosed(base::RepeatingClosure on_menu_closed_callback);
 
   void ExecuteCommand(int command_id, int event_flags) override;
 

chrome/browser/ui/views/download/download_shelf_view.cc

@@ -45,8 +45,6 @@
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
-using download::DownloadItem;
-
 namespace {
 
 // TODO(pkasting): Replace these with LayoutProvider constants
@@ -94,9 +92,10 @@ DownloadShelfView::DownloadShelfView(Browser* browser, BrowserView* parent)
     shelf_animation_.SetSlideDuration(base::TimeDelta());
   }
 
-  GetViewAccessibility().OverrideName(
+  views::ViewAccessibility& accessibility = GetViewAccessibility();
+  accessibility.OverrideName(
       l10n_util::GetStringUTF16(IDS_ACCNAME_DOWNLOADS_BAR));
-  GetViewAccessibility().OverrideRole(ax::mojom::Role::kGroup);
+  accessibility.OverrideRole(ax::mojom::Role::kGroup);
 
   // Delay 5 seconds if the mouse leaves the shelf by way of entering another
   // window. This is much larger than the normal delay as opening a download is
@@ -139,14 +138,10 @@ gfx::Size DownloadShelfView::CalculatePreferredSize() const {
 
 void DownloadShelfView::Layout() {
   int x = kStartPadding;
-
-  int download_items_end =
+  const int download_items_end =
       std::max(0, width() - kEndPadding - close_button_->width() -
                       kCloseAndLinkPadding - show_all_view_->width());
-  // If there is not enough room to show the first download item, show the
-  // "Show all downloads" link to the left to make it more visible that there is
-  // something to see.
-  bool all_downloads_hidden =
+  const bool all_downloads_hidden =
       !download_views_.empty() &&
       (download_views_.back()->GetPreferredSize().width() >
        (download_items_end - x));
@@ -155,13 +150,16 @@ void DownloadShelfView::Layout() {
     return std::max((height - item_height) / 2, kTopPadding);
   };
 
-  show_all_view_->SetPosition({all_downloads_hidden ? x : download_items_end,
-                               center_y(show_all_view_->height())});
+  show_all_view_->SetPosition(
+      {// If none of the download items can be shown, move the link to the left
+       // to make it more obvious that there is something to see.
+       all_downloads_hidden ? x : download_items_end,
+       center_y(show_all_view_->height())});
   close_button_->SetPosition(
       {show_all_view_->bounds().right() + kCloseAndLinkPadding,
        center_y(close_button_->height())});
+
   if (all_downloads_hidden) {
-    // Let's hide all the items.
     for (auto* view : download_views_)
       view->SetVisible(false);
     return;
@@ -169,7 +167,6 @@ void DownloadShelfView::Layout() {
 
   for (auto* view : base::Reversed(download_views_)) {
     gfx::Size view_size = view->GetPreferredSize();
-
     if (view == download_views_.back()) {
       view_size = gfx::Tween::SizeValueBetween(
           new_item_animation_.GetCurrentValue(),
@@ -177,24 +174,20 @@ void DownloadShelfView::Layout() {
     }
 
     const gfx::Rect bounds({x, center_y(view_size.height())}, view_size);
-    x = bounds.right();
+    view->SetBoundsRect(bounds);
+    view->SetVisible(bounds.right() < download_items_end);
 
-    // Make sure our item can be contained within the shelf.
-    if (x < download_items_end) {
-      view->SetVisible(true);
-      view->SetBoundsRect(bounds);
-    } else {
-      view->SetVisible(false);
-    }
+    x = bounds.right();
   }
 }
 
 void DownloadShelfView::AnimationProgressed(const gfx::Animation* animation) {
   if (animation == &new_item_animation_) {
     InvalidateLayout();
-  } else if (animation == &shelf_animation_) {
+  } else {
+    DCHECK_EQ(&shelf_animation_, animation);
     // Force a re-layout of the parent, which will call back into
-    // GetPreferredSize, where we will do our animation. In the case where the
+    // GetPreferredSize(), where we will do our animation. In the case where the
     // animation is hiding, we do a full resize - the fast resizing would
     // otherwise leave blank white areas where the shelf was and where the
     // user's eye is. Thankfully bottom-resizing is a lot faster than
@@ -215,21 +208,18 @@ void DownloadShelfView::AnimationEnded(const gfx::Animation* animation) {
   if (shown || is_hidden())
     return;
 
-  // When the close animation is complete, remove all completed downloads.
-  size_t i = 0;
-  while (i < download_views_.size()) {
-    DownloadUIModel* download = download_views_[i]->model();
-    DownloadItem::DownloadState state = download->GetState();
-    bool is_transfer_done = state == DownloadItem::COMPLETE ||
-                            state == DownloadItem::CANCELLED ||
-                            state == DownloadItem::INTERRUPTED;
-    if (is_transfer_done && !download->IsDangerous()) {
-      RemoveDownloadView(download_views_[i]);
-    } else {
+  // Remove all completed downloads.
+  for (size_t i = 0; i < download_views_.size();) {
+    DownloadItemView* const view = download_views_[i];
+    DownloadUIModel* const model = view->model();
+    if ((model->GetState() == download::DownloadItem::IN_PROGRESS) ||
+        model->IsDangerous()) {
       // Treat the item as opened when we close. This way if we get shown again
       // the user need not open this item for the shelf to auto-close.
-      download->SetOpened(true);
+      model->SetOpened(true);
       ++i;
+    } else {
+      RemoveDownloadView(view);
     }
   }
 
@@ -245,14 +235,14 @@ void DownloadShelfView::AnimationEnded(const gfx::Animation* animation) {
   SetVisible(false);
 }
 
-void DownloadShelfView::ButtonPressed(
-    views::Button* button, const ui::Event& event) {
-  if (button == close_button_)
+void DownloadShelfView::ButtonPressed(views::Button* button,
+                                      const ui::Event& event) {
+  if (button == close_button_) {
     Close();
-  else if (button == show_all_view_)
+  } else {
+    DCHECK_EQ(show_all_view_, button);
     chrome::ShowDownloads(browser());
-  else
-    NOTREACHED();
+  }
 }
 
 void DownloadShelfView::MouseMovedOutOfHost() {
@@ -267,11 +257,11 @@ void DownloadShelfView::AutoClose() {
 
 void DownloadShelfView::RemoveDownloadView(View* view) {
   DCHECK(view);
-  auto i = find(download_views_.begin(), download_views_.end(), view);
+  const auto i =
+      std::find(download_views_.begin(), download_views_.end(), view);
   DCHECK(i != download_views_.end());
   download_views_.erase(i);
-  RemoveChildView(view);
-  delete view;
+  RemoveChildViewT(view);
   if (download_views_.empty())
     Close();
   else
@@ -325,7 +315,7 @@ void DownloadShelfView::DoShowDownload(
 
   if (was_empty && !shelf_animation_.is_animating() && GetVisible()) {
     // Force a re-layout of the parent to adjust height of shelf properly.
-    parent_->ToolbarSizeChanged(shelf_animation_.IsShowing());
+    parent_->ToolbarSizeChanged(true);
   }
 }
 
@@ -371,8 +361,6 @@ void DownloadShelfView::OnThemeChanged() {
 }
 
 views::View* DownloadShelfView::GetDefaultFocusableChild() {
-  if (!download_views_.empty())
-    return download_views_.back();
-
-  return show_all_view_;
+  return download_views_.empty() ? static_cast<views::View*>(show_all_view_)
+                                 : download_views_.back();
 }