diff options
| -rw-r--r-- | chrome/browser/download/download_item_model.cc | 29 | ||||
| -rw-r--r-- | chrome/browser/download/download_item_model.h | 18 | ||||
| -rw-r--r-- | chrome/browser/download/download_item_model_unittest.cc | 69 | ||||
| -rw-r--r-- | chrome/browser/download/download_shelf.cc | 102 | ||||
| -rw-r--r-- | chrome/browser/download/download_shelf.h | 36 | ||||
| -rw-r--r-- | chrome/browser/download/download_shelf_unittest.cc | 220 | ||||
| -rw-r--r-- | chrome/browser/download/test_download_shelf.cc | 18 | ||||
| -rw-r--r-- | chrome/browser/download/test_download_shelf.h | 16 | ||||
| -rw-r--r-- | chrome/browser/ui/browser.cc | 18 | ||||
| -rw-r--r-- | chrome/browser/ui/cocoa/download/download_item_mac.mm | 2 | ||||
| -rw-r--r-- | chrome/browser/ui/gtk/download/download_item_gtk.cc | 7 | ||||
| -rw-r--r-- | chrome/browser/ui/views/download/download_item_view.cc | 2 | ||||
| -rw-r--r-- | chrome/chrome_tests_unit.gypi | 1 |
13 files changed, 456 insertions, 82 deletions
diff --git a/chrome/browser/download/download_item_model.cc b/chrome/browser/download/download_item_model.cc index d6242b1..9b4013e 100644 --- a/chrome/browser/download/download_item_model.cc +++ b/chrome/browser/download/download_item_model.cc @@ -369,6 +369,35 @@ bool DownloadItemModel::IsMalicious() const { return false; } +bool DownloadItemModel::ShouldRemoveFromShelfWhenComplete() const { + // If the download was already opened automatically, it should be removed. + if (download_->GetAutoOpened()) + return true; + + // If the download is interrupted or cancelled, it should not be removed. + if (download_->IsInterrupted() || download_->IsCancelled()) + return false; + + // If the download is dangerous or malicious, we should display a warning on + // the shelf until the user accepts the download. + if (IsDangerous()) + return false; + + // If the download is an extension, temporary, or will be opened + // automatically, then it should be removed from the shelf on completion. + // TODO(asanka): The logic for deciding opening behavior should be in a + // central location. http://crbug.com/167702 + return (download_crx_util::IsExtensionDownload(*download_) || + download_->IsTemporary() || + download_->GetOpenWhenComplete() || + download_->ShouldOpenFileBasedOnExtension()); +} + +bool DownloadItemModel::ShouldShowDownloadStartedAnimation() const { + return !download_->IsSavePackageDownload() && + !download_crx_util::IsExtensionDownload(*download_); +} + bool DownloadItemModel::ShouldShowInShelf() const { const DownloadItemModelData* data = DownloadItemModelData::Get(download_); return !data || data->should_show_in_shelf(); diff --git a/chrome/browser/download/download_item_model.h b/chrome/browser/download/download_item_model.h index 6ac4f59..7b65fe5 100644 --- a/chrome/browser/download/download_item_model.h +++ b/chrome/browser/download/download_item_model.h @@ -79,6 +79,24 @@ class DownloadItemModel { // Is this considered a malicious download? Implies IsDangerous(). bool IsMalicious() const; + // Returns |true| if this download is expected to complete successfully and + // thereafter be removed from the shelf. Downloads that are opened + // automatically or are temporary will be removed from the shelf on successful + // completion. + // + // Returns |false| if the download is not expected to complete (interrupted, + // cancelled, dangerous, malicious), or won't be removed on completion. + // + // Since the expectation of successful completion may change, the return value + // of this function will change over the course of a download. + bool ShouldRemoveFromShelfWhenComplete() const; + + // Returns |true| if the download started animation (big download arrow + // animates down towards the shelf) should be displayed for this download. + // Downloads that were initiated via "Save As" or are extension installs don't + // show the animation. + bool ShouldShowDownloadStartedAnimation() const; + // Returns |true| if this download should be displayed in the downloads shelf. bool ShouldShowInShelf() const; diff --git a/chrome/browser/download/download_item_model_unittest.cc b/chrome/browser/download/download_item_model_unittest.cc index 123fe72..fdd971a 100644 --- a/chrome/browser/download/download_item_model_unittest.cc +++ b/chrome/browser/download/download_item_model_unittest.cc @@ -20,6 +20,7 @@ #include "ui/base/text/bytes_formatting.h" #include "ui/gfx/font.h" +using content::DownloadItem; using ::testing::Mock; using ::testing::NiceMock; using ::testing::Return; @@ -72,7 +73,7 @@ class DownloadItemModelTest : public testing::Test { ON_CALL(item_, GetOpenWhenComplete()).WillByDefault(Return(false)); ON_CALL(item_, GetFileExternallyRemoved()).WillByDefault(Return(false)); ON_CALL(item_, GetState()) - .WillByDefault(Return(content::DownloadItem::IN_PROGRESS)); + .WillByDefault(Return(DownloadItem::IN_PROGRESS)); ON_CALL(item_, GetURL()) .WillByDefault(ReturnRefOfCopy(GURL(kDefaultURL))); ON_CALL(item_, GetFileNameToReportUser()) @@ -81,7 +82,7 @@ class DownloadItemModelTest : public testing::Test { .WillByDefault(ReturnRefOfCopy(FilePath(kDefaultTargetFilePath))); ON_CALL(item_, GetTargetDisposition()) .WillByDefault( - Return(content::DownloadItem::TARGET_DISPOSITION_OVERWRITE)); + Return(DownloadItem::TARGET_DISPOSITION_OVERWRITE)); ON_CALL(item_, IsPaused()).WillByDefault(Return(false)); } @@ -90,8 +91,8 @@ class DownloadItemModelTest : public testing::Test { EXPECT_CALL(item_, GetState()) .WillRepeatedly(Return( (reason == content::DOWNLOAD_INTERRUPT_REASON_NONE) ? - content::DownloadItem::IN_PROGRESS : - content::DownloadItem::INTERRUPTED)); + DownloadItem::IN_PROGRESS : + DownloadItem::INTERRUPTED)); EXPECT_CALL(item_, IsInProgress()) .WillRepeatedly(Return( reason == content::DOWNLOAD_INTERRUPT_REASON_NONE)); @@ -269,7 +270,7 @@ TEST_F(DownloadItemModelTest, InterruptTooltip) { } TEST_F(DownloadItemModelTest, InProgressStatus) { - struct TestCase { + const struct TestCase { int64 received_bytes; // Return value of GetReceivedBytes(). int64 total_bytes; // Return value of GetTotalBytes(). bool time_remaining_known; // If TimeRemaining() is known. @@ -319,7 +320,7 @@ TEST_F(DownloadItemModelTest, InProgressStatus) { SetupDownloadItemDefaults(); for (unsigned i = 0; i < ARRAYSIZE_UNSAFE(kTestCases); i++) { - TestCase& test_case = kTestCases[i]; + const TestCase& test_case = kTestCases[i]; Mock::VerifyAndClearExpectations(&item()); Mock::VerifyAndClearExpectations(&model()); EXPECT_CALL(item(), GetReceivedBytes()) @@ -353,3 +354,59 @@ TEST_F(DownloadItemModelTest, ShouldShowInShelf) { model().SetShouldShowInShelf(true); EXPECT_TRUE(model().ShouldShowInShelf()); } + +TEST_F(DownloadItemModelTest, ShouldRemoveFromShelfWhenComplete) { + const struct TestCase { + DownloadItem::DownloadState state; + bool is_dangerous; + bool is_auto_open; // Either an extension install, temporary, open when + // complete or open based on extension. + bool expected_result; + } kTestCases[] = { + // All the valid combinations of state, is_dangerous and is_auto_open. + // + // .--- Is dangerous. + // | .--- Auto open or temporary. + // | | .--- Expected result. + { DownloadItem::IN_PROGRESS, false, false, false }, + { DownloadItem::IN_PROGRESS, false, true , true }, + { DownloadItem::IN_PROGRESS, true , false, false }, + { DownloadItem::IN_PROGRESS, true , true , false }, + { DownloadItem::COMPLETE, false, false, false }, + { DownloadItem::COMPLETE, false, true , true }, + { DownloadItem::CANCELLED, false, false, false }, + { DownloadItem::CANCELLED, false, true , false }, + { DownloadItem::CANCELLED, true , false, false }, + { DownloadItem::CANCELLED, true , true , false }, + { DownloadItem::INTERRUPTED, false, false, false }, + { DownloadItem::INTERRUPTED, false, true , false }, + { DownloadItem::INTERRUPTED, true , false, false }, + { DownloadItem::INTERRUPTED, true , true , false } + }; + + SetupDownloadItemDefaults(); + + for (unsigned i = 0; i < ARRAYSIZE_UNSAFE(kTestCases); i++) { + const TestCase& test_case = kTestCases[i]; + EXPECT_CALL(item(), GetOpenWhenComplete()) + .WillRepeatedly(Return(test_case.is_auto_open)); + EXPECT_CALL(item(), GetState()) + .WillRepeatedly(Return(test_case.state)); + EXPECT_CALL(item(), IsCancelled()) + .WillRepeatedly(Return(test_case.state == DownloadItem::CANCELLED)); + EXPECT_CALL(item(), IsComplete()) + .WillRepeatedly(Return(test_case.state == DownloadItem::COMPLETE)); + EXPECT_CALL(item(), IsInProgress()) + .WillRepeatedly(Return(test_case.state == DownloadItem::IN_PROGRESS)); + EXPECT_CALL(item(), IsInterrupted()) + .WillRepeatedly(Return(test_case.state == DownloadItem::INTERRUPTED)); + EXPECT_CALL(item(), GetSafetyState()) + .WillRepeatedly(Return(test_case.is_dangerous ? DownloadItem::DANGEROUS + : DownloadItem::SAFE)); + + EXPECT_EQ(test_case.expected_result, + model().ShouldRemoveFromShelfWhenComplete()); + Mock::VerifyAndClearExpectations(&item()); + Mock::VerifyAndClearExpectations(&model()); + } +} diff --git a/chrome/browser/download/download_shelf.cc b/chrome/browser/download/download_shelf.cc index 524ff7d..be24469 100644 --- a/chrome/browser/download/download_shelf.cc +++ b/chrome/browser/download/download_shelf.cc @@ -4,15 +4,54 @@ #include "chrome/browser/download/download_shelf.h" +#include "base/bind.h" +#include "base/callback.h" +#include "base/message_loop.h" +#include "chrome/browser/download/download_item_model.h" +#include "chrome/browser/download/download_started_animation.h" +#include "chrome/browser/platform_util.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_tabstrip.h" +#include "content/public/browser/browser_context.h" +#include "content/public/browser/download_item.h" +#include "content/public/browser/download_manager.h" +#include "content/public/browser/web_contents.h" +#include "ui/base/animation/animation.h" + +using content::DownloadItem; + +namespace { + +// Delay before we show a transient download. +const int64 kDownloadShowDelayInSeconds = 2; + +} // namespace + DownloadShelf::DownloadShelf() : should_show_on_unhide_(false), - is_hidden_(false) {} + is_hidden_(false), + ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) { +} -void DownloadShelf::AddDownload(content::DownloadItem* download) { - if (is_hidden_) - Unhide(); - Show(); - DoAddDownload(download); +DownloadShelf::~DownloadShelf() { +} + +void DownloadShelf::AddDownload(DownloadItem* download) { + DCHECK(download); + if (DownloadItemModel(download).ShouldRemoveFromShelfWhenComplete()) { + // If we are going to remove the download from the shelf upon completion, + // wait a few seconds to see if it completes quickly. If it's a small + // download, then the user won't have time to interact with it. + MessageLoop::current()->PostDelayedTask( + FROM_HERE, + base::Bind(&DownloadShelf::ShowDownloadById, + weak_ptr_factory_.GetWeakPtr(), + download->GetId()), + GetTransientDownloadShowDelay()); + } else { + ShowDownload(download); + } } void DownloadShelf::Show() { @@ -50,3 +89,54 @@ void DownloadShelf::Unhide() { DoShow(); } } + +base::TimeDelta DownloadShelf::GetTransientDownloadShowDelay() { + return base::TimeDelta::FromSeconds(kDownloadShowDelayInSeconds); +} + +content::DownloadManager* DownloadShelf::GetDownloadManager() { + DCHECK(browser()); + return content::BrowserContext::GetDownloadManager(browser()->profile()); +} + +void DownloadShelf::ShowDownload(DownloadItem* download) { + if (download->IsComplete() && + DownloadItemModel(download).ShouldRemoveFromShelfWhenComplete()) { + return; + } + + if (is_hidden_) + Unhide(); + Show(); + DoAddDownload(download); + + // browser() can be NULL for tests. + if (!browser()) + return; + + // 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. + // - The browser has an active visble WebContents. (browser isn't minimized, + // or running under a test etc.) + // - Rich animations are enabled. + content::WebContents* shelf_tab = chrome::GetActiveWebContents(browser()); + if (DownloadItemModel(download).ShouldShowDownloadStartedAnimation() && + shelf_tab && + platform_util::IsVisible(shelf_tab->GetNativeView()) && + ui::Animation::ShouldRenderRichAnimation()) { + DownloadStartedAnimation::Show(shelf_tab); + } +} + +void DownloadShelf::ShowDownloadById(int32 download_id) { + content::DownloadManager* download_manager = GetDownloadManager(); + if (!download_manager) + return; + + DownloadItem* download = download_manager->GetDownload(download_id); + if (!download) + return; + + ShowDownload(download); +} diff --git a/chrome/browser/download/download_shelf.h b/chrome/browser/download/download_shelf.h index b04b5c2..9bf0ab4 100644 --- a/chrome/browser/download/download_shelf.h +++ b/chrome/browser/download/download_shelf.h @@ -5,8 +5,12 @@ #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_SHELF_H_ #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_SHELF_H_ +#include "base/memory/weak_ptr.h" +#include "base/time.h" + namespace content { class DownloadItem; +class DownloadManager; } class Browser; @@ -16,10 +20,15 @@ class Browser; class DownloadShelf { public: DownloadShelf(); - virtual ~DownloadShelf() {} - - // A new download has started, so add it to our shelf. Also make the shelf - // visible. + virtual ~DownloadShelf(); + + // A new download has started. Add it to our shelf and show the download + // started animation. + // + // Some downloads are removed from the shelf on completion (See + // DownloadItemModel::ShouldRemoveFromShelfWhenComplete()). These transient + // downloads are added to the shelf after a delay. If the download completes + // before the delay duration, it will not be added to the shelf at all. void AddDownload(content::DownloadItem* download); // The browser view needs to know when we are going away to properly return @@ -54,9 +63,28 @@ class DownloadShelf { virtual void DoShow() = 0; virtual void DoClose() = 0; + // Time delay to wait before adding a transient download to the shelf. + // Protected virtual for testing. + virtual base::TimeDelta GetTransientDownloadShowDelay(); + + // Returns the DownloadManager associated with this DownloadShelf. All + // downloads that are shown on this shelf is expected to belong to this + // DownloadManager. Protected virtual for testing. + virtual content::DownloadManager* GetDownloadManager(); + private: + // Show the download on the shelf immediately. Also displayes the download + // started animation if necessary. + void ShowDownload(content::DownloadItem* download); + + // Similar to ShowDownload() but refers to the download using an ID. This + // download should belong to the DownloadManager returned by + // GetDownloadManager(). + void ShowDownloadById(int32 download_id); + bool should_show_on_unhide_; bool is_hidden_; + base::WeakPtrFactory<DownloadShelf> weak_ptr_factory_; }; #endif // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_SHELF_H_ diff --git a/chrome/browser/download/download_shelf_unittest.cc b/chrome/browser/download/download_shelf_unittest.cc index fd5dc84..b77326e 100644 --- a/chrome/browser/download/download_shelf_unittest.cc +++ b/chrome/browser/download/download_shelf_unittest.cc @@ -2,48 +2,186 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "testing/gtest/include/gtest/gtest.h" - #include "base/compiler_specific.h" +#include "base/memory/scoped_ptr.h" +#include "base/message_loop.h" +#include "base/run_loop.h" +#include "chrome/browser/download/download_item_model.h" #include "chrome/browser/download/test_download_shelf.h" +#include "content/public/test/mock_download_item.h" +#include "content/public/test/mock_download_manager.h" +#include "content/public/test/test_browser_thread.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +using ::testing::Return; +using ::testing::ReturnRefOfCopy; +using ::testing::SaveArg; +using ::testing::_; +using content::DownloadItem; + +namespace { + +class DownloadShelfTest : public testing::Test { + public: + DownloadShelfTest(); + + protected: + content::MockDownloadItem* download_item() { + return download_item_.get(); + } + content::MockDownloadManager* download_manager() { + return download_manager_.get(); + } + TestDownloadShelf* shelf() { + return &shelf_; + } + + private: + scoped_ptr<content::MockDownloadItem> GetInProgressMockDownload(); + + MessageLoopForUI message_loop_; + content::TestBrowserThread ui_thread_; + scoped_ptr<content::MockDownloadItem> download_item_; + scoped_refptr<content::MockDownloadManager> download_manager_; + TestDownloadShelf shelf_; +}; + +DownloadShelfTest::DownloadShelfTest() + : ui_thread_(content::BrowserThread::UI, &message_loop_) { + download_item_.reset(new ::testing::NiceMock<content::MockDownloadItem>()); + ON_CALL(*download_item_, GetAutoOpened()).WillByDefault(Return(false)); + ON_CALL(*download_item_, GetMimeType()).WillByDefault(Return("text/plain")); + ON_CALL(*download_item_, GetOpenWhenComplete()).WillByDefault(Return(false)); + ON_CALL(*download_item_, GetSafetyState()) + .WillByDefault(Return(DownloadItem::SAFE)); + ON_CALL(*download_item_, GetTargetDisposition()) + .WillByDefault(Return(DownloadItem::TARGET_DISPOSITION_OVERWRITE)); + ON_CALL(*download_item_, GetURL()) + .WillByDefault(ReturnRefOfCopy(GURL("http://example.com/foo"))); + ON_CALL(*download_item_, IsComplete()).WillByDefault(Return(false)); + ON_CALL(*download_item_, IsInProgress()).WillByDefault(Return(true)); + ON_CALL(*download_item_, IsTemporary()).WillByDefault(Return(false)); + ON_CALL(*download_item_, ShouldOpenFileBasedOnExtension()) + .WillByDefault(Return(false)); + + download_manager_ = new ::testing::NiceMock<content::MockDownloadManager>(); + ON_CALL(*download_manager_, GetDownload(_)) + .WillByDefault(Return(download_item_.get())); + + shelf_.set_download_manager(download_manager_.get()); +} + +} // namespace + +TEST_F(DownloadShelfTest, ClosesShelfWhenHidden) { + shelf()->Show(); + EXPECT_TRUE(shelf()->IsShowing()); + shelf()->Hide(); + EXPECT_FALSE(shelf()->IsShowing()); + shelf()->Unhide(); + EXPECT_TRUE(shelf()->IsShowing()); +} + +TEST_F(DownloadShelfTest, CloseWhileHiddenPreventsShowOnUnhide) { + shelf()->Show(); + shelf()->Hide(); + shelf()->Close(); + shelf()->Unhide(); + EXPECT_FALSE(shelf()->IsShowing()); +} + +TEST_F(DownloadShelfTest, UnhideDoesntShowIfNotShownOnHide) { + shelf()->Hide(); + shelf()->Unhide(); + EXPECT_FALSE(shelf()->IsShowing()); +} + +TEST_F(DownloadShelfTest, AddDownloadWhileHiddenUnhides) { + shelf()->Show(); + shelf()->Hide(); + shelf()->AddDownload(download_item()); + EXPECT_TRUE(shelf()->IsShowing()); +} + +TEST_F(DownloadShelfTest, AddDownloadWhileHiddenUnhidesAndShows) { + shelf()->Hide(); + shelf()->AddDownload(download_item()); + EXPECT_TRUE(shelf()->IsShowing()); +} + +// Normal downloads should be added synchronously and cause the shelf to show. +TEST_F(DownloadShelfTest, AddNormalDownload) { + EXPECT_FALSE(shelf()->IsShowing()); + shelf()->AddDownload(download_item()); + EXPECT_TRUE(shelf()->did_add_download()); + EXPECT_TRUE(shelf()->IsShowing()); +} + +// Add a transient download. It should not be added immediately. Instead it +// should be added after a delay. For testing, the delay is set to 0 seconds. So +// the download should be added once the message loop is flushed. +TEST_F(DownloadShelfTest, AddDelayedDownload) { + EXPECT_CALL(*download_item(), ShouldOpenFileBasedOnExtension()) + .WillRepeatedly(Return(true)); + ASSERT_TRUE(DownloadItemModel(download_item()) + .ShouldRemoveFromShelfWhenComplete()); + shelf()->AddDownload(download_item()); + + EXPECT_FALSE(shelf()->did_add_download()); + EXPECT_FALSE(shelf()->IsShowing()); + + base::RunLoop run_loop; + run_loop.RunUntilIdle(); + + EXPECT_TRUE(shelf()->did_add_download()); + EXPECT_TRUE(shelf()->IsShowing()); +} + +// Add a transient download that completes before the delay. It should not be +// displayed on the shelf. +TEST_F(DownloadShelfTest, AddDelayedCompletedDownload) { + EXPECT_CALL(*download_item(), ShouldOpenFileBasedOnExtension()) + .WillRepeatedly(Return(true)); + ASSERT_TRUE(DownloadItemModel(download_item()) + .ShouldRemoveFromShelfWhenComplete()); + shelf()->AddDownload(download_item()); + + EXPECT_FALSE(shelf()->did_add_download()); + EXPECT_FALSE(shelf()->IsShowing()); + + EXPECT_CALL(*download_item(), IsComplete()) + .WillRepeatedly(Return(true)); + + base::RunLoop run_loop; + run_loop.RunUntilIdle(); + + EXPECT_FALSE(shelf()->did_add_download()); + EXPECT_FALSE(shelf()->IsShowing()); +} + +// Add a transient download that completes and becomes non-transient before the +// delay. It should be displayed on the shelf even though it is complete. +TEST_F(DownloadShelfTest, AddDelayedCompleteNonTransientDownload) { + EXPECT_CALL(*download_item(), ShouldOpenFileBasedOnExtension()) + .WillRepeatedly(Return(true)); + ASSERT_TRUE(DownloadItemModel(download_item()) + .ShouldRemoveFromShelfWhenComplete()); + shelf()->AddDownload(download_item()); + + EXPECT_FALSE(shelf()->did_add_download()); + EXPECT_FALSE(shelf()->IsShowing()); + + EXPECT_CALL(*download_item(), IsComplete()) + .WillRepeatedly(Return(true)); + EXPECT_CALL(*download_item(), ShouldOpenFileBasedOnExtension()) + .WillRepeatedly(Return(false)); + ASSERT_FALSE(DownloadItemModel(download_item()) + .ShouldRemoveFromShelfWhenComplete()); + + base::RunLoop run_loop; + run_loop.RunUntilIdle(); -TEST(DownloadShelfTest, ClosesShelfWhenHidden) { - TestDownloadShelf shelf; - shelf.Show(); - EXPECT_TRUE(shelf.IsShowing()); - shelf.Hide(); - EXPECT_FALSE(shelf.IsShowing()); - shelf.Unhide(); - EXPECT_TRUE(shelf.IsShowing()); -} - -TEST(DownloadShelfTest, CloseWhileHiddenPreventsShowOnUnhide) { - TestDownloadShelf shelf; - shelf.Show(); - shelf.Hide(); - shelf.Close(); - shelf.Unhide(); - EXPECT_FALSE(shelf.IsShowing()); -} - -TEST(DownloadShelfTest, UnhideDoesntShowIfNotShownOnHide) { - TestDownloadShelf shelf; - shelf.Hide(); - shelf.Unhide(); - EXPECT_FALSE(shelf.IsShowing()); -} - -TEST(DownloadShelfTest, AddDownloadWhileHiddenUnhides) { - TestDownloadShelf shelf; - shelf.Show(); - shelf.Hide(); - shelf.AddDownload(NULL); - EXPECT_TRUE(shelf.IsShowing()); -} - -TEST(DownloadShelfTest, AddDownloadWhileHiddenUnhidesAndShows) { - TestDownloadShelf shelf; - shelf.Hide(); - shelf.AddDownload(NULL); - EXPECT_TRUE(shelf.IsShowing()); + EXPECT_TRUE(shelf()->did_add_download()); + EXPECT_TRUE(shelf()->IsShowing()); } diff --git a/chrome/browser/download/test_download_shelf.cc b/chrome/browser/download/test_download_shelf.cc index 9957c64..6eef694 100644 --- a/chrome/browser/download/test_download_shelf.cc +++ b/chrome/browser/download/test_download_shelf.cc @@ -4,8 +4,11 @@ #include "chrome/browser/download/test_download_shelf.h" +#include "content/public/browser/download_manager.h" + TestDownloadShelf::TestDownloadShelf() - : is_showing_(false) { + : is_showing_(false), + did_add_download_(false) { } TestDownloadShelf::~TestDownloadShelf() { @@ -23,7 +26,13 @@ Browser* TestDownloadShelf::browser() const { return NULL; } +void TestDownloadShelf::set_download_manager( + content::DownloadManager* download_manager) { + download_manager_ = download_manager; +} + void TestDownloadShelf::DoAddDownload(content::DownloadItem* download) { + did_add_download_ = true; } void TestDownloadShelf::DoShow() { @@ -34,3 +43,10 @@ void TestDownloadShelf::DoClose() { is_showing_ = false; } +base::TimeDelta TestDownloadShelf::GetTransientDownloadShowDelay() { + return base::TimeDelta(); +} + +content::DownloadManager* TestDownloadShelf::GetDownloadManager() { + return download_manager_.get(); +} diff --git a/chrome/browser/download/test_download_shelf.h b/chrome/browser/download/test_download_shelf.h index da7bb57..32d5c81 100644 --- a/chrome/browser/download/test_download_shelf.h +++ b/chrome/browser/download/test_download_shelf.h @@ -7,8 +7,13 @@ #include "base/basictypes.h" #include "base/compiler_specific.h" +#include "base/memory/ref_counted.h" #include "chrome/browser/download/download_shelf.h" +namespace content { +class DownloadManager; +} + // An implementation of DownloadShelf for testing. class TestDownloadShelf : public DownloadShelf { public: @@ -16,18 +21,27 @@ class TestDownloadShelf : public DownloadShelf { virtual ~TestDownloadShelf(); // DownloadShelf: - virtual bool IsShowing() const OVERRIDE; virtual bool IsClosing() const OVERRIDE; virtual Browser* browser() const OVERRIDE; + // Return |true| if a download was added to this shelf. + bool did_add_download() const { return did_add_download_; } + + // Set download_manager_ (and the result of calling GetDownloadManager()) + void set_download_manager(content::DownloadManager* download_manager); + protected: virtual void DoAddDownload(content::DownloadItem* download) OVERRIDE; virtual void DoShow() OVERRIDE; virtual void DoClose() OVERRIDE; + virtual base::TimeDelta GetTransientDownloadShowDelay() OVERRIDE; + virtual content::DownloadManager* GetDownloadManager() OVERRIDE; private: bool is_showing_; + bool did_add_download_; + scoped_refptr<content::DownloadManager> download_manager_; DISALLOW_COPY_AND_ASSIGN(TestDownloadShelf); }; diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc index 9feaab1..b519b0e 100644 --- a/chrome/browser/ui/browser.cc +++ b/chrome/browser/ui/browser.cc @@ -44,13 +44,10 @@ #include "chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h" #include "chrome/browser/devtools/devtools_toggle_action.h" #include "chrome/browser/devtools/devtools_window.h" -#include "chrome/browser/download/download_crx_util.h" #include "chrome/browser/download/download_item_model.h" #include "chrome/browser/download/download_service.h" #include "chrome/browser/download/download_service_factory.h" #include "chrome/browser/download/download_shelf.h" -#include "chrome/browser/download/download_started_animation.h" -#include "chrome/browser/download/download_util.h" #include "chrome/browser/extensions/browser_extension_window_controller.h" #include "chrome/browser/extensions/extension_prefs.h" #include "chrome/browser/extensions/extension_service.h" @@ -68,7 +65,6 @@ #include "chrome/browser/net/url_fixer_upper.h" #include "chrome/browser/notifications/notification_ui_manager.h" #include "chrome/browser/pepper_broker_infobar_delegate.h" -#include "chrome/browser/platform_util.h" #include "chrome/browser/prefs/incognito_mode_prefs.h" #include "chrome/browser/prefs/pref_service.h" #include "chrome/browser/printing/cloud_print/cloud_print_setup_flow.h" @@ -187,7 +183,6 @@ #include "net/base/registry_controlled_domains/registry_controlled_domain.h" #include "net/cookies/cookie_monster.h" #include "net/url_request/url_request_context.h" -#include "ui/base/animation/animation.h" #include "ui/base/dialogs/selected_file_info.h" #include "ui/base/l10n/l10n_util.h" #include "ui/gfx/point.h" @@ -1448,19 +1443,6 @@ void Browser::OnStartDownload(WebContents* source, // GetDownloadShelf creates the download shelf if it was not yet created. DownloadShelf* shelf = window()->GetDownloadShelf(); shelf->AddDownload(download); - // Don't show the animation for "Save file" downloads. - // For non-theme extensions, we don't show the download animation. - // Show animation in same window as the download shelf. Download shelf - // may not be in the same window that initiated the download. - // Don't show the animation if the selected tab is not visible (i.e. the - // window is minimized, we're in a unit test, etc.). - WebContents* shelf_tab = chrome::GetActiveWebContents(shelf->browser()); - if ((download->GetTotalBytes() > 0) && - !download_crx_util::IsExtensionDownload(*download) && - platform_util::IsVisible(shelf_tab->GetNativeView()) && - ui::Animation::ShouldRenderRichAnimation()) { - DownloadStartedAnimation::Show(shelf_tab); - } // If the download occurs in a new tab, and it's not a save page // download (started before initial navigation completed) close it. diff --git a/chrome/browser/ui/cocoa/download/download_item_mac.mm b/chrome/browser/ui/cocoa/download/download_item_mac.mm index ea3dd9b..c9f278a 100644 --- a/chrome/browser/ui/cocoa/download/download_item_mac.mm +++ b/chrome/browser/ui/cocoa/download/download_item_mac.mm @@ -47,7 +47,7 @@ void DownloadItemMac::OnDownloadUpdated(content::DownloadItem* download) { switch (download->GetState()) { case DownloadItem::COMPLETE: - if (download->GetAutoOpened()) { + if (download_model_.ShouldRemoveFromShelfWhenComplete()) { [item_controller_ remove]; // We're deleted now! return; } diff --git a/chrome/browser/ui/gtk/download/download_item_gtk.cc b/chrome/browser/ui/gtk/download/download_item_gtk.cc index e1da54d..802910e 100644 --- a/chrome/browser/ui/gtk/download/download_item_gtk.cc +++ b/chrome/browser/ui/gtk/download/download_item_gtk.cc @@ -339,9 +339,10 @@ void DownloadItemGtk::OnDownloadUpdated(DownloadItem* download_item) { complete_animation_.Show(); break; case DownloadItem::COMPLETE: - // GetAutoOpened() may change after the download's initial transition to - // COMPLETE, so we check it before the idemopotency shield below. - if (download()->GetAutoOpened()) { + // ShouldRemoveFromShelfWhenComplete() may change after the download's + // initial transition to COMPLETE, so we check it before the idemopotency + // shield below. + if (download_model_.ShouldRemoveFromShelfWhenComplete()) { parent_shelf_->RemoveDownloadItem(this); // This will delete us! return; } diff --git a/chrome/browser/ui/views/download/download_item_view.cc b/chrome/browser/ui/views/download/download_item_view.cc index 13bbd761..46896f6 100644 --- a/chrome/browser/ui/views/download/download_item_view.cc +++ b/chrome/browser/ui/views/download/download_item_view.cc @@ -270,7 +270,7 @@ void DownloadItemView::OnDownloadUpdated(DownloadItem* download_item) { LoadIcon(); break; case DownloadItem::COMPLETE: - if (download()->GetAutoOpened()) { + if (model_.ShouldRemoveFromShelfWhenComplete()) { shelf_->RemoveDownloadView(this); // This will delete us! return; } diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 5a6977e..e0240b7 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -2102,6 +2102,7 @@ # There's no Browser/BrowserList on Android. 'browser/browser_commands_unittest.cc', + 'browser/download/download_shelf_unittest.cc', 'browser/managed_mode/managed_mode_unittest.cc', 'browser/managed_mode/managed_mode_url_filter_unittest.cc', 'browser/net/gaia/gaia_oauth_fetcher_unittest.cc', |