Simplify DownloadShelf "show download" code flow.

By inlining GetDownload() into ShowDownloadById() and making
OnGetDownloadDoneForOfflineItem() a member, the code becomes clearer and
more direct (mostly by converting the bound ShowDownload() callback into
direct calls).

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

chrome/browser/download/download_shelf.cc

@@ -7,8 +7,8 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/callback.h"
 #include "base/location.h"
+#include "base/optional.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/download/download_core_service.h"
@@ -38,50 +38,6 @@ namespace {
 // Delay before we show a transient download.
 const int64_t kDownloadShowDelayInSeconds = 2;
 
-void OnGetDownloadDoneForOfflineItem(
-    Profile* profile,
-    base::OnceCallback<void(DownloadUIModel::DownloadUIModelPtr)> callback,
-    const base::Optional<offline_items_collection::OfflineItem>& offline_item) {
-  if (!offline_item.has_value())
-    return;
-
-  OfflineItemModelManager* manager =
-      OfflineItemModelManagerFactory::GetForBrowserContext(profile);
-  DownloadUIModel::DownloadUIModelPtr model =
-      OfflineItemModel::Wrap(manager, offline_item.value());
-
-  std::move(callback).Run(std::move(model));
-}
-
-void GetDownload(
-    Profile* profile,
-    const offline_items_collection::ContentId& id,
-    base::OnceCallback<void(DownloadUIModel::DownloadUIModelPtr)> callback) {
-  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;
-
-    DownloadUIModel::DownloadUIModelPtr model =
-        DownloadItemModel::Wrap(download);
-    std::move(callback).Run(std::move(model));
-  } else {
-    offline_items_collection::OfflineContentAggregator* aggregator =
-        OfflineContentAggregatorFactory::GetForKey(profile->GetProfileKey());
-    if (!aggregator)
-      return;
-
-    aggregator->GetItemById(id, base::BindOnce(&OnGetDownloadDoneForOfflineItem,
-                                               profile, std::move(callback)));
-  }
-}
-
 }  // namespace
 
 DownloadShelf::DownloadShelf(Browser* browser, Profile* profile)
@@ -184,7 +140,36 @@ void DownloadShelf::ShowDownload(DownloadUIModel::DownloadUIModelPtr download) {
 
 void DownloadShelf::ShowDownloadById(
     const offline_items_collection::ContentId& id) {
-  GetDownload(profile_, id,
-              base::BindOnce(&DownloadShelf::ShowDownload,
-                             weak_ptr_factory_.GetWeakPtr()));
+  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));
+  } else {
+    offline_items_collection::OfflineContentAggregator* aggregator =
+        OfflineContentAggregatorFactory::GetForKey(profile_->GetProfileKey());
+    if (!aggregator)
+      return;
+
+    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()));
 }

chrome/browser/download/download_shelf.h

@@ -14,8 +14,14 @@
 
 class Browser;
 
+namespace base {
+template <typename T>
+class Optional;
+}  // namespace base
+
 namespace offline_items_collection {
 struct ContentId;
+struct OfflineItem;
 }  // namespace offline_items_collection
 
 // This is an abstract base class for platform specific download shelf
@@ -82,6 +88,11 @@ class DownloadShelf {
   // Similar to ShowDownload() but refers to the download using an ID.
   void ShowDownloadById(const offline_items_collection::ContentId& id);
 
+  // Callback used by ShowDownloadById() to trigger ShowDownload() once |item|
+  // has been fetched.
+  void OnGetDownloadDoneForOfflineItem(
+      const base::Optional<offline_items_collection::OfflineItem>& item);
+
   Browser* const browser_;
   Profile* const profile_;
   bool should_show_on_unhide_;