Move closing-blank-tab logic in Browser class to DownloadUIConttroller

chrome/browser/download/download_ui_controller.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/download/download_ui_controller.h"
 
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/stl_util.h"
 #include "chrome/browser/download/download_item_model.h"
+#include "chrome/browser/download/download_shelf.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
 
 #if defined(OS_ANDROID)
 #include "content/public/browser/android/download_controller_android.h"
 #else
 #include "chrome/browser/download/notification/download_notification_manager.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 55 matching lines @@
   Browser* browser =
       web_contents ? chrome::FindBrowserWithWebContents(web_contents) : NULL;
 
   // As a last resort, use the last active browser for this profile. Not ideal,
   // but better than not showing the download at all.
   if (browser == NULL) {
     browser = chrome::FindLastActiveWithProfile(profile_,
                                                 chrome::GetActiveDesktop());
   }
 
-  if (browser)
-    browser->ShowDownload(item);
+  if (browser && browser->window() &&
+      // Some (app downloads) are not supposed to appear on the shelf.

[msw, 2015/07/17 01:15:34]

nit: please move this comment above the start of the conditional
(you can remove either of these comments, neither adds much value)

[yoshiki, 2015/07/17 01:40:04]

Done. Removed the first comment.

+      DownloadItemModel(item).ShouldShowInShelf()) {
+    // GetDownloadShelf creates the download shelf if it was not yet created.
+    browser->window()->GetDownloadShelf()->AddDownload(item);
+  }
 }
 
 #endif  // !OS_ANDROID
 
 } // namespace
 
 DownloadUIController::Delegate::~Delegate() {
 }
 
 DownloadUIController::DownloadUIController(content::DownloadManager* manager,
@@ skipping 22 matching lines @@
 DownloadUIController::~DownloadUIController() {
 }
 
 void DownloadUIController::OnDownloadCreated(content::DownloadManager* manager,
                                              content::DownloadItem* item) {
   // SavePackage downloads are created in a state where they can be shown in the
   // browser. Call OnDownloadUpdated() once to notify the UI immediately.
   OnDownloadUpdated(manager, item);
 }
 
 void DownloadUIController::OnDownloadUpdated(content::DownloadManager* manager,

[msw, 2015/07/17 01:15:35]

OnDownloadUpdated is called (via DownloadItemImpl::UpdateObservers) for *many*
reasons; is there any reason to not keep the tab closing code in
OnNewDownloadReady, which seems like a more specific event? Beyond the change in
behavior that might lead to a regression, I worry that this code might now run
every time a download's progress advances (or similar), where it might even
introduce a [minor?] performance regression (think with N=10+ downloads...).
Sorry if my concerns are unfounded, but I'm not super familiar with this code.

[yoshiki, 2015/07/17 01:40:04]

Thank you for pointing out.

But any new performance problem is not introduced by this patch. Because
WasUINotified() check (l.145 in new code) skips the further logic if it's
non-first update. And OnNewDownloadReady is called at the last of this method,
so the number of call has not been changed by this patch.

The motivation of moving this to here is that OnNewDownloadReady is one of the
delegate methods. The delegate is specialized for kind of UI so we want to write
the common code in this class. 

[msw, 2015/07/17 02:11:33]

Ah, I totally missed that, thanks for the explanation.

                                              content::DownloadItem* item) {
   DownloadItemModel item_model(item);
 
   // Ignore if we've already notified the UI about |item| or if it isn't a new
   // download.
   if (item_model.WasUINotified() || !item_model.ShouldNotifyUI())
     return;
 
   // Wait until the target path is determined.
   if (item->GetTargetFilePath().empty())
     return;
 
   DownloadItemModel(item).SetWasUINotified(true);
+
+#if !defined(OS_ANDROID)
+  content::WebContents* web_contents = item->GetWebContents();
+  if (web_contents) {
+    Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
+    // If the download occurs in a new tab, and it's not a save page
+    // download (started before initial navigation completed) close it.
+    // Avoid calling CloseContents if the tab is not in this browser's tab strip
+    // model; this can happen if the download was initiated by something
+    // internal to Chrome, such as by the app list.
+    if (browser && web_contents->GetController().IsInitialNavigation() &&
+        browser->tab_strip_model()->count() > 1 &&
+        browser->tab_strip_model()->GetIndexOfWebContents(web_contents) !=
+            TabStripModel::kNoTab &&
+        !item->IsSavePackageDownload()) {
+      web_contents->Close();

[msw, 2015/07/17 01:15:34]

Will changing Browser::CloseContents to WebContents::Close have any impact?
(I'd expect that there isn't any meaningful difference, but thought I'd ask)

[yoshiki, 2015/07/17 01:40:04]

WebContents::Close calls Browser::CloseContents internally. The only difference
is a RenderViewHost check
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...).
Hence I think there is no diffirence. 

[msw, 2015/07/17 02:11:33]

Acknowledged.

+    }
+  }
+#endif
+
   delegate_->OnNewDownloadReady(item);
 }