Move download shelf from per-tab to per-window. Also disable auto-hiding of

the shelf.

BUG=9025
TEST=Download file in one tab, open new tab, and check that download shelf is
still open. Also try the shelf's close button and the "show all downloads"
link. When saving a file, the download animation should not show up.

Review URL: http://codereview.chromium.org/115740


git-svn-id: svn://svn.chromium.org/chrome/trunk/src@17595 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 91 insertions, 54 deletions

diff --git a/chrome/browser/download/download_file.cc b/chrome/browser/download/download_file.cc
index b871540..7be3411 100644
--- a/chrome/browser/download/download_file.cc
+++ b/chrome/browser/download/download_file.cc
@@ -536,17 +536,18 @@ void DownloadFileManager::OnShowDownloadInShell(const FilePath& full_path) {
 void DownloadFileManager::OnOpenDownloadInShell(const FilePath& full_path,
                                                 const GURL& url,
                                                 gfx::NativeView parent_window) {
-  DCHECK(MessageLoop::current() == file_loop_);
-
 #if defined(OS_WIN)
-  if (NULL != parent_window) {
-    win_util::SaferOpenItemViaShell(parent_window, L"", full_path,
-                                    UTF8ToWide(url.spec()));
-    return;
-  }
+    DCHECK(MessageLoop::current() == file_loop_);
+    if (NULL != parent_window) {
+      win_util::SaferOpenItemViaShell(parent_window, L"", full_path,
+                                      UTF8ToWide(url.spec()));
+    } else {
+      win_util::OpenItemViaShell(full_path);
+    }
+#else
+  // TODO(port) implement me.
+  NOTIMPLEMENTED();
 #endif
-
-  platform_util::OpenItem(full_path);
 }
 
 // The DownloadManager in the UI thread has provided a final name for the
diff --git a/chrome/browser/download/download_shelf.cc b/chrome/browser/download/download_shelf.cc
index 6962a3e..0b468c4 100644
--- a/chrome/browser/download/download_shelf.cc
+++ b/chrome/browser/download/download_shelf.cc
@@ -6,11 +6,11 @@
 
 #include "app/l10n_util.h"
 #include "base/file_util.h"
+#include "chrome/browser/browser.h"
 #include "chrome/browser/dom_ui/downloads_ui.h"
 #include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/metrics/user_metrics.h"
-#include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/common/url_constants.h"
 #include "grit/generated_resources.h"
 
@@ -22,17 +22,11 @@
 // DownloadShelf ---------------------------------------------------------------
 
 void DownloadShelf::ShowAllDownloads() {
-  Profile* profile = tab_contents_->profile();
+  Profile* profile = browser_->profile();
   if (profile)
     UserMetrics::RecordAction(L"ShowDownloads", profile);
-  tab_contents_->OpenURL(GURL(chrome::kChromeUIDownloadsURL), GURL(),
-                         SINGLETON_TAB, PageTransition::AUTO_BOOKMARK);
-}
-
-void DownloadShelf::ChangeTabContents(TabContents* old_contents,
-                                      TabContents* new_contents) {
-  DCHECK(old_contents == tab_contents_);
-  tab_contents_ = new_contents;
+  browser_->OpenURL(GURL(chrome::kChromeUIDownloadsURL), GURL(),
+                    NEW_FOREGROUND_TAB, PageTransition::AUTO_BOOKMARK);
 }
 
 // DownloadShelfContextMenu ----------------------------------------------------
diff --git a/chrome/browser/download/download_shelf.h b/chrome/browser/download/download_shelf.h
index 1b455c0..1ad0b71 100644
--- a/chrome/browser/download/download_shelf.h
+++ b/chrome/browser/download/download_shelf.h
@@ -7,47 +7,48 @@
 
 #include <string>
 
+#include "base/logging.h"
 #include "base/basictypes.h"
 
 class BaseDownloadItemModel;
+class Browser;
 class DownloadItem;
-class TabContents;
 
 // DownloadShelf is an interface for platform-specific download shelves to
 // implement. It also contains some shared logic. This class should not be
 // instantiated directly, but rather created via a call to Create().
+// It is a view object.
 class DownloadShelf {
  public:
-  explicit DownloadShelf(TabContents* tab_contents)
-      : tab_contents_(tab_contents) { }
+  explicit DownloadShelf(Browser* browser)
+      : browser_(browser) { DCHECK(browser_); }
 
   virtual ~DownloadShelf() { }
 
-  // Creates a platform-specific DownloadShelf, passing ownership to the caller.
-  static DownloadShelf* Create(TabContents* tab_contents);
-
   // A new download has started, so add it to our shelf. This object will
-  // take ownership of |download_model|.
+  // take ownership of |download_model|. Also make the shelf visible.
   virtual void AddDownload(BaseDownloadItemModel* download_model) = 0;
 
   // Invoked when the user clicks the 'show all downloads' link button.
   void ShowAllDownloads();
 
-  // Invoked when the download shelf is migrated from one tab contents to a new
-  // one.
-  void ChangeTabContents(TabContents* old_contents, TabContents* new_contents);
-
   // The browser view needs to know when we are going away to properly return
   // the resize corner size to WebKit so that we don't draw on top of it.
-  // This returns the showing state of our animation which is set to false at
-  // the beginning Show and true at the beginning of a Hide.
+  // This returns the showing state of our animation which is set to true at
+  // the beginning Show and false at the beginning of a Hide.
   virtual bool IsShowing() const = 0;
 
   // Returns whether the download shelf is showing the close animation.
   virtual bool IsClosing() const = 0;
 
+  // Opens the shelf.
+  virtual void Show() = 0;
+
+  // Closes the shelf.
+  virtual void Close() = 0;
+
  protected:
-  TabContents* tab_contents_;
+  Browser* browser_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(DownloadShelf);
diff --git a/chrome/browser/download/download_uitest.cc b/chrome/browser/download/download_uitest.cc
index 78a3683..8f7eb99 100644
--- a/chrome/browser/download/download_uitest.cc
+++ b/chrome/browser/download/download_uitest.cc
@@ -131,9 +131,7 @@ class DownloadTest : public UITest {
       // TODO(tc): check download status text
 
       // Make sure the download shelf is showing.
-      scoped_refptr<TabProxy> dl_tab(window->GetTab(0));
-      ASSERT_TRUE(dl_tab.get());
-      EXPECT_TRUE(WaitForDownloadShelfVisible(dl_tab.get()));
+      EXPECT_TRUE(WaitForDownloadShelfVisible(window.get()));
     }
 
     FilePath filename;
@@ -170,9 +168,9 @@ TEST_F(DownloadTest, DownloadMimeType) {
 
   CleanUpDownload(file);
 
-  scoped_refptr<TabProxy> tab_proxy(GetActiveTab());
-  ASSERT_TRUE(tab_proxy.get());
-  EXPECT_TRUE(WaitForDownloadShelfVisible(tab_proxy.get()));
+  scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
+  ASSERT_TRUE(browser.get());
+  EXPECT_TRUE(WaitForDownloadShelfVisible(browser.get()));
 }
 
 // Access a file with a viewable mime-type, verify that a download
@@ -196,9 +194,9 @@ TEST_F(DownloadTest, NoDownload) {
   if (file_util::PathExists(file_path))
     ASSERT_TRUE(file_util::Delete(file_path, false));
 
-  scoped_refptr<TabProxy> tab_proxy(GetActiveTab());
-  ASSERT_TRUE(tab_proxy.get());
-  EXPECT_FALSE(WaitForDownloadShelfVisible(tab_proxy.get()));
+  scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
+  ASSERT_TRUE(browser.get());
+  EXPECT_FALSE(WaitForDownloadShelfVisible(browser.get()));
 }
 
 // Download a 0-size file with a content-disposition header, verify that the
@@ -218,12 +216,54 @@ TEST_F(DownloadTest, ContentDisposition) {
 
   CleanUpDownload(download_file, file);
 
-  // Ensure the download shelf is visible on the current tab.
-  scoped_refptr<TabProxy> tab_proxy(GetActiveTab());
-  ASSERT_TRUE(tab_proxy.get());
-  EXPECT_TRUE(WaitForDownloadShelfVisible(tab_proxy.get()));
+  // Ensure the download shelf is visible on the window.
+  scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
+  ASSERT_TRUE(browser.get());
+  EXPECT_TRUE(WaitForDownloadShelfVisible(browser.get()));
 }
 
+// Test that the download shelf is per-window by starting a download in one
+// tab, opening a second tab, closing the shelf, going back to the first tab,
+// and checking that the shelf is closed.
+TEST_F(DownloadTest, PerWindowShelf) {
+  FilePath file(FILE_PATH_LITERAL("download-test3.gif"));
+  FilePath download_file(FILE_PATH_LITERAL("download-test3-attachment.gif"));
+
+  EXPECT_EQ(1, GetTabCount());
+
+  NavigateToURL(URLRequestMockHTTPJob::GetMockUrl(file.ToWStringHack()));
+  WaitUntilTabCount(1);
+
+  // Wait until the file is downloaded.
+  PlatformThread::Sleep(action_timeout_ms());
+
+  CleanUpDownload(download_file, file);
+
+  // Ensure the download shelf is visible on the window.
+  scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
+  ASSERT_TRUE(browser.get());
+  EXPECT_TRUE(WaitForDownloadShelfVisible(browser.get()));
+
+  // Open a second tab
+  browser->AppendTab(GURL(""));
+  WaitUntilTabCount(2);
+
+  // Hide shelf
+  browser->SetShelfVisible(false);
+  EXPECT_TRUE(WaitForDownloadShelfInvisible(browser.get()));
+
+  // Go to first tab
+  EXPECT_TRUE(browser->ActivateTab(0));
+  int tab_count;
+  EXPECT_TRUE(browser->GetTabCount(&tab_count));
+  ASSERT_EQ(2, tab_count);
+
+  bool shelf_visible;
+  EXPECT_TRUE(browser->IsShelfVisible(&shelf_visible));
+  ASSERT_FALSE(shelf_visible);
+}
+
+
 // UnknownSize and KnownSize are tests which depend on
 // URLRequestSlowDownloadJob to serve content in a certain way. Data will be
 // sent in two chunks where the first chunk is 35K and the second chunk is 10K.
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index 645346b..cd23fc2 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -252,9 +252,7 @@ bool SavePackage::Init() {
       FilePath(), Time::Now(), 0, -1, -1, false);
   download_->set_manager(tab_contents_->profile()->GetDownloadManager());
 #if !defined(OS_MACOSX)
-  DownloadShelf* shelf = tab_contents_->GetDownloadShelf(true);
-  shelf->AddDownload(new SavePageModel(this, download_));
-  tab_contents_->SetDownloadShelfVisible(true);
+  tab_contents_->OnStartDownload(download_);
 #else
   // TODO(port): Create a download shelf for mac.
   NOTIMPLEMENTED();
diff --git a/chrome/browser/download/save_page_uitest.cc b/chrome/browser/download/save_page_uitest.cc
index 4562d20..840a9f0 100644
--- a/chrome/browser/download/save_page_uitest.cc
+++ b/chrome/browser/download/save_page_uitest.cc
@@ -85,7 +85,8 @@ TEST_F(SavePageTest, SaveHTMLOnly) {
 
   EXPECT_TRUE(tab->SavePage(full_file_name.ToWStringHack(), dir.ToWStringHack(),
                             SavePackage::SAVE_AS_ONLY_HTML));
-  EXPECT_TRUE(WaitForDownloadShelfVisible(tab.get()));
+  scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
+  EXPECT_TRUE(WaitForDownloadShelfVisible(browser.get()));
 
   CheckFile(full_file_name, FilePath::FromWStringHack(UTF8ToWide(file_name)),
             true);
@@ -105,7 +106,8 @@ TEST_F(SavePageTest, SaveCompleteHTML) {
 
   EXPECT_TRUE(tab->SavePage(full_file_name.ToWStringHack(), dir.ToWStringHack(),
                             SavePackage::SAVE_AS_COMPLETE_HTML));
-  EXPECT_TRUE(WaitForDownloadShelfVisible(tab.get()));
+  scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
+  EXPECT_TRUE(WaitForDownloadShelfVisible(browser.get()));
 
   CheckFile(dir.AppendASCII("1.png"), FilePath(FILE_PATH_LITERAL("1.png")),
                             true);
@@ -128,7 +130,8 @@ TEST_F(SavePageTest, NoSave) {
   EXPECT_FALSE(tab->SavePage(full_file_name.ToWStringHack(),
                              dir.ToWStringHack(),
                              SavePackage::SAVE_AS_ONLY_HTML));
-  EXPECT_FALSE(WaitForDownloadShelfVisible(tab.get()));
+  scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
+  EXPECT_FALSE(WaitForDownloadShelfVisible(browser.get()));
 }
 
 TEST_F(SavePageTest, FilenameFromPageTitle) {
@@ -148,7 +151,7 @@ TEST_F(SavePageTest, FilenameFromPageTitle) {
   scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
   automation()->SavePackageShouldPromptUser(false);
   EXPECT_TRUE(browser->RunCommandAsync(IDC_SAVE_PAGE));
-  EXPECT_TRUE(WaitForDownloadShelfVisible(tab.get()));
+  EXPECT_TRUE(WaitForDownloadShelfVisible(browser.get()));
   automation()->SavePackageShouldPromptUser(true);
 
   CheckFile(dir.AppendASCII("1.png"), FilePath(FILE_PATH_LITERAL("1.png")),
@@ -180,7 +183,7 @@ TEST_F(SavePageTest, CleanFilenameFromPageTitle) {
   scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
   automation()->SavePackageShouldPromptUser(false);
   EXPECT_TRUE(browser->RunCommandAsync(IDC_SAVE_PAGE));
-  EXPECT_TRUE(WaitForDownloadShelfVisible(tab.get()));
+  EXPECT_TRUE(WaitForDownloadShelfVisible(browser.get()));
   automation()->SavePackageShouldPromptUser(true);
 
   CheckFile(full_file_name, FilePath::FromWStringHack(UTF8ToWide(file_name)),