Reland "Refactor DownloadItemView state transitions, part 4."

This is a reland of 43713035

Accidental empty-bitmap-check-reversal undone.

Original change's description:
> Refactor DownloadItemView state transitions, part 4.
>
> Renames LoadIcon() to UpdateFilePath() and moves it to be with the other
> Update...() functions, since it doesn't unconditionally load, but rather
> updates the file path, and conditionally loads if the path has changed.
>
> With the goal of greater consistency, renames |last_download_item_path_|
> to |file_path_|, |icon_| to |file_icon_|, and OnExtractIconComplete() to
> OnFileIconLoaded().
>
> Finally, makes a few tweaks around invalidation:
> * Explicit Layout() calls should be InvalidateLayout(); this will result
>   in a SchedulePaint() if anything moves, so the explicit
>   SchedulePaint() at the end of UpdateMode() can be dropped.
> * It's not clear to me why loading the icons would schedule a paint on
>   the shelf instead of |this|.  If there are other items on the shelf
>   referring to these same icons, they have tasks of their own.  Instead,
>   just SchedulePaint() when the file icon loads (large or small).
>
> Bug: none
> Change-Id: I2a7c5af7dcbe9bc489b18bc6a58e1183477a7027
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2299180
> Commit-Queue: Peter Kasting <pkasting@chromium.org>
> Reviewed-by: Min Qin <qinmin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#788927}

Bug: none
Change-Id: Ia8a06b3c6516443d6cb2ab49fbc64240c9347808
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303449
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789502}

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

@@ -133,9 +133,6 @@ constexpr int kDefaultDownloadItemHeight = 48;
 // Amount of time between accessible alert events.
 constexpr auto kAccessibleAlertInterval = base::TimeDelta::FromSeconds(30);
 
-// The size of the file icon.
-constexpr int kFileIconSize = 24;
-
 // The offset from the file icon to the danger icon.
 constexpr int kDangerIconOffset = 8;
 
@@ -698,6 +695,7 @@ void DownloadItemView::UpdateMode(Mode mode) {
   // TODO(pkasting): Refactor further.
 
   mode_ = mode;
+  UpdateFilePath();
   UpdateLabels();
   UpdateButtons();
 
@@ -779,12 +777,29 @@ void DownloadItemView::UpdateMode(Mode mode) {
     }
   }
 
-  // We need to load the icon now that the download has the real path.
-  LoadIcon();
+  shelf_->InvalidateLayout();
+}
+
+void DownloadItemView::UpdateFilePath() {
+  const base::FilePath file_path = model_->GetTargetFilePath();
+  if (file_path_ == file_path)
+    return;
+
+  file_path_ = file_path;
+  cancelable_task_tracker_.TryCancelAll();
 
-  // Force the shelf to layout again as our size has changed.
-  shelf_->Layout();
-  shelf_->SchedulePaint();
+  // The small icon is not stored directly, but will be requested in other
+  // functions, so ask the icon manager to load it so it's cached.
+  IconManager* const im = g_browser_process->icon_manager();
+  im->LoadIcon(file_path_, IconLoader::SMALL,
+               base::BindOnce(&DownloadItemView::OnFileIconLoaded,
+                              base::Unretained(this), IconLoader::SMALL),
+               &cancelable_task_tracker_);
+
+  im->LoadIcon(file_path_, IconLoader::NORMAL,
+               base::BindOnce(&DownloadItemView::OnFileIconLoaded,
+                              base::Unretained(this), IconLoader::NORMAL),
+               &cancelable_task_tracker_);
 }
 
 void DownloadItemView::UpdateLabels() {
@@ -945,7 +960,7 @@ void DownloadItemView::DrawIcon(gfx::Canvas* canvas) {
     PaintDownloadProgress(canvas, progress_bounds, base::TimeDelta(), 100);
     canvas->Restore();
   } else if (use_new_warnings) {
-    current_icon = &icon_;
+    current_icon = &file_icon_;
   }
 
   if (!current_icon)
@@ -973,25 +988,6 @@ void DownloadItemView::DrawIcon(gfx::Canvas* canvas) {
   }
 }
 
-void DownloadItemView::LoadIcon() {
-  const base::FilePath last_download_item_path = model_->GetTargetFilePath();
-  if (last_download_item_path_ == last_download_item_path)
-    return;
-
-  last_download_item_path_ = last_download_item_path;
-
-  IconManager* im = g_browser_process->icon_manager();
-  im->LoadIcon(
-      last_download_item_path_, IconLoader::SMALL,
-      base::BindOnce(&DownloadItemView::OnExtractIconComplete,
-                     base::Unretained(this), IconLoader::IconSize::SMALL),
-      &cancelable_task_tracker_);
-  im->LoadIcon(last_download_item_path_, IconLoader::NORMAL,
-               base::BindOnce(&DownloadItemView::OnExtractIconComplete,
-                              base::Unretained(this), IconLoader::NORMAL),
-               &cancelable_task_tracker_);
-}
-
 void DownloadItemView::UpdateColorsFromTheme() {
   if (!GetThemeProvider())
     return;
@@ -1262,17 +1258,18 @@ void DownloadItemView::AnnounceAccessibleAlert() {
   announce_accessible_alert_soon_ = false;
 }
 
-void DownloadItemView::OnExtractIconComplete(IconLoader::IconSize icon_size,
-                                             gfx::Image icon_bitmap) {
+void DownloadItemView::OnFileIconLoaded(IconLoader::IconSize icon_size,
+                                        gfx::Image icon_bitmap) {
   if (!icon_bitmap.IsEmpty()) {
-    if (icon_size == IconLoader::IconSize::NORMAL) {
+    if (icon_size == IconLoader::NORMAL) {
       // We want a 24x24 icon, but on Windows only 16x16 and 32x32 are
       // available. So take the NORMAL icon and downsize it.
-      icon_ = gfx::ImageSkiaOperations::CreateResizedImage(
+      constexpr gfx::Size kFileIconSize(24, 24);
+      file_icon_ = gfx::ImageSkiaOperations::CreateResizedImage(
           *icon_bitmap.ToImageSkia(), skia::ImageOperations::RESIZE_BEST,
-          gfx::Size(kFileIconSize, kFileIconSize));
+          kFileIconSize);
     }
-    shelf_->SchedulePaint();
+    SchedulePaint();
   }
 }
 

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

@@ -134,6 +134,11 @@ class DownloadItemView : public views::View,
   // Sets the current mode to |mode| and updates UI appropriately.
   void UpdateMode(Mode mode);
 
+  // Updates the file path, and if necessary, begins loading the file icon in
+  // various sizes. This may eventually result in a callback to
+  // OnFileIconLoaded().
+  void UpdateFilePath();
+
   // Updates the visibility, text, size, etc. of all labels.
   void UpdateLabels();
 
@@ -149,7 +154,6 @@ class DownloadItemView : public views::View,
       DownloadCommands::Command download_command);
 
   void DrawIcon(gfx::Canvas* canvas);
-  void LoadIcon();
 
   // Update the button colors based on the current theme.
   void UpdateColorsFromTheme();
@@ -208,7 +212,9 @@ class DownloadItemView : public views::View,
   // reader to speak the current alert immediately.
   void AnnounceAccessibleAlert();
 
-  void OnExtractIconComplete(IconLoader::IconSize icon_size, gfx::Image icon);
+  // Sets |file_icon_| to |icon|. Called when the icon manager has loaded the
+  // normal-size icon for the current file path.
+  void OnFileIconLoaded(IconLoader::IconSize icon_size, gfx::Image icon);
 
   // Paint the common download animation progress foreground and background. If
   // |percent_done| < 0, the total size is indeterminate.
@@ -304,6 +310,8 @@ class DownloadItemView : public views::View,
   // button logic in DownloadItemView.
   views::Button* open_button_;
 
+  gfx::ImageSkia file_icon_;
+
   views::Label* file_name_label_;
   views::Label* status_label_;
   views::StyledLabel* warning_label_;
@@ -338,17 +346,13 @@ class DownloadItemView : public views::View,
   // Force the reading of the current alert text the next time it updates.
   bool announce_accessible_alert_soon_;
 
-  // The icon loaded in the download shelf is based on the file path of the
-  // item.  Store the path used, so that we can detect a change in the path
-  // and reload the icon.
-  base::FilePath last_download_item_path_;
+  // |file_icon_| is based on the path of the downloaded item.  Store the path
+  // used, so that we can detect a change in the path and reload the icon.
+  base::FilePath file_path_;
 
   // Deep scanning modal dialog confirming choice to "open now".
   TabModalConfirmDialog* open_now_modal_dialog_;
 
-  // Icon for the download.
-  gfx::ImageSkia icon_;
-
   // Method factory used to delay reenabling of the item when opening the
   // downloaded file.
   base::WeakPtrFactory<DownloadItemView> weak_ptr_factory_{this};