diff options
7 files changed, 39 insertions, 68 deletions
diff --git a/chrome/browser/download/download_request_limiter.cc b/chrome/browser/download/download_request_limiter.cc index a37822f..39f776e 100644 --- a/chrome/browser/download/download_request_limiter.cc +++ b/chrome/browser/download/download_request_limiter.cc @@ -13,6 +13,8 @@ #include "content/browser/tab_contents/navigation_entry.h" #include "content/browser/tab_contents/tab_contents.h" #include "content/browser/tab_contents/tab_contents_delegate.h" +#include "chrome/browser/ui/blocked_content/blocked_content_tab_helper.h" +#include "chrome/browser/ui/blocked_content/blocked_content_tab_helper_delegate.h" #include "chrome/browser/ui/download/download_tab_helper.h" #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h" #include "content/common/notification_source.h" @@ -251,33 +253,38 @@ void DownloadRequestLimiter::CanDownload(int render_process_host_id, ScheduleNotification(callback, false); return; } - CanDownloadImpl(originating_tab, request_id, callback); + + CanDownloadImpl( + TabContentsWrapper::GetCurrentWrapperForContents(originating_tab), + request_id, + callback); } void DownloadRequestLimiter::CanDownloadImpl( - TabContents* originating_tab, + TabContentsWrapper* originating_tab, int request_id, Callback* callback) { + DCHECK(originating_tab); + // FYI: Chrome Frame overrides CanDownload in ExternalTabContainer in order // to cancel the download operation in chrome and let the host browser // take care of it. - TabContentsWrapper* wrapper = - TabContentsWrapper::GetCurrentWrapperForContents(originating_tab); - if (!wrapper->download_tab_helper()->CanDownload(request_id)) { + if (!originating_tab->download_tab_helper()->CanDownload(request_id)) { ScheduleNotification(callback, false); return; } // If the tab requesting the download is a constrained popup that is not // shown, treat the request as if it came from the parent. - TabContents* effective_tab = originating_tab; - if (effective_tab->delegate()) { - effective_tab = - effective_tab->delegate()->GetConstrainingContents(effective_tab); + TabContentsWrapper* effective_wrapper = originating_tab; + if (effective_wrapper->blocked_content_tab_helper()->delegate()) { + effective_wrapper = effective_wrapper->blocked_content_tab_helper()-> + delegate()->GetConstrainingContentsWrapper(effective_wrapper); } TabDownloadState* state = GetDownloadState( - &effective_tab->controller(), &originating_tab->controller(), true); + &effective_wrapper->tab_contents()->controller(), + &originating_tab->controller(), true); switch (state->download_status()) { case ALLOW_ALL_DOWNLOADS: if (state->download_count() && !(state->download_count() % @@ -297,7 +304,7 @@ void DownloadRequestLimiter::CanDownloadImpl( break; case PROMPT_BEFORE_DOWNLOAD: - state->PromptUserForDownload(effective_tab, callback); + state->PromptUserForDownload(effective_wrapper->tab_contents(), callback); state->increment_download_count(); break; diff --git a/chrome/browser/download/download_request_limiter.h b/chrome/browser/download/download_request_limiter.h index 8ce2f06..e32a81c 100644 --- a/chrome/browser/download/download_request_limiter.h +++ b/chrome/browser/download/download_request_limiter.h @@ -17,6 +17,7 @@ class DownloadRequestInfoBarDelegate; class NavigationController; class TabContents; +class TabContentsWrapper; // DownloadRequestLimiter is responsible for determining whether a download // should be allowed or not. It is designed to keep pages from downloading @@ -223,7 +224,7 @@ class DownloadRequestLimiter // Does the work of updating the download status on the UI thread and // potentially prompting the user. - void CanDownloadImpl(TabContents* originating_tab, + void CanDownloadImpl(TabContentsWrapper* originating_tab, int request_id, Callback* callback); diff --git a/chrome/browser/download/download_request_limiter_unittest.cc b/chrome/browser/download/download_request_limiter_unittest.cc index 9cdb3c3..f8fa3ff 100644 --- a/chrome/browser/download/download_request_limiter_unittest.cc +++ b/chrome/browser/download/download_request_limiter_unittest.cc @@ -7,6 +7,7 @@ #include "chrome/test/testing_profile.h" #include "content/browser/browser_thread.h" #include "content/browser/tab_contents/navigation_controller.h" +#include "content/browser/tab_contents/test_tab_contents.h" #include "testing/gtest/include/gtest/gtest.h" class DownloadRequestLimiterTest @@ -41,8 +42,7 @@ class DownloadRequestLimiterTest } void CanDownload() { - download_request_limiter_->CanDownloadImpl( - controller().tab_contents(), -1, this); + download_request_limiter_->CanDownloadImpl(contents_wrapper(), -1, this); message_loop_.RunAllPending(); } @@ -88,14 +88,12 @@ class DownloadRequestLimiterTest TEST_F(DownloadRequestLimiterTest, Allow) { // All tabs should initially start at ALLOW_ONE_DOWNLOAD. ASSERT_EQ(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // Ask if the tab can do a download. This moves to PROMPT_BEFORE_DOWNLOAD. CanDownload(); ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // We should have been told we can download. ASSERT_EQ(1, continue_count_); ASSERT_EQ(0, cancel_count_); @@ -109,8 +107,7 @@ TEST_F(DownloadRequestLimiterTest, Allow) { ASSERT_EQ(1, ask_allow_count_); ask_allow_count_ = 0; ASSERT_EQ(DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // We should have been told we can download. ASSERT_EQ(1, continue_count_); ASSERT_EQ(0, cancel_count_); @@ -121,8 +118,7 @@ TEST_F(DownloadRequestLimiterTest, Allow) { // The state is at allow_all, which means the delegate shouldn't be asked. ASSERT_EQ(0, ask_allow_count_); ASSERT_EQ(DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // We should have been told we can download. ASSERT_EQ(1, continue_count_); ASSERT_EQ(0, cancel_count_); @@ -138,8 +134,7 @@ TEST_F(DownloadRequestLimiterTest, ResetOnNavigation) { CanDownload(); ask_allow_count_ = continue_count_ = cancel_count_ = 0; ASSERT_EQ(DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // Navigate to a new URL with the same host, which shouldn't reset the allow // all state. @@ -150,21 +145,18 @@ TEST_F(DownloadRequestLimiterTest, ResetOnNavigation) { ASSERT_EQ(0, ask_allow_count_); ask_allow_count_ = continue_count_ = cancel_count_ = 0; ASSERT_EQ(DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // Do a user gesture, because we're at allow all, this shouldn't change the // state. - download_request_limiter_->OnUserGesture(controller().tab_contents()); + download_request_limiter_->OnUserGesture(contents()); ASSERT_EQ(DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // Navigate to a completely different host, which should reset the state. NavigateAndCommit(GURL("http://fooey.com")); ASSERT_EQ(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); } TEST_F(DownloadRequestLimiterTest, ResetOnUserGesture) { @@ -174,14 +166,12 @@ TEST_F(DownloadRequestLimiterTest, ResetOnUserGesture) { CanDownload(); ask_allow_count_ = continue_count_ = cancel_count_ = 0; ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // Do a user gesture, which should reset back to allow one. - download_request_limiter_->OnUserGesture(controller().tab_contents()); + download_request_limiter_->OnUserGesture(contents()); ASSERT_EQ(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // Ask twice, which triggers calling the delegate. Don't allow the download // so that we end up with not allowed. @@ -189,14 +179,12 @@ TEST_F(DownloadRequestLimiterTest, ResetOnUserGesture) { CanDownload(); CanDownload(); ASSERT_EQ(DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // A user gesture now should NOT change the state. - download_request_limiter_->OnUserGesture(controller().tab_contents()); + download_request_limiter_->OnUserGesture(contents()); ASSERT_EQ(DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); // And make sure we really can't download. ask_allow_count_ = continue_count_ = cancel_count_ = 0; CanDownload(); @@ -205,6 +193,5 @@ TEST_F(DownloadRequestLimiterTest, ResetOnUserGesture) { ASSERT_EQ(1, cancel_count_); // And the state shouldn't have changed. ASSERT_EQ(DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED, - download_request_limiter_->GetDownloadStatus( - controller().tab_contents())); + download_request_limiter_->GetDownloadStatus(contents())); } diff --git a/chrome/browser/ui/blocked_content/blocked_content_container.cc b/chrome/browser/ui/blocked_content/blocked_content_container.cc index 11204ea..0b1ca6f 100644 --- a/chrome/browser/ui/blocked_content/blocked_content_container.cc +++ b/chrome/browser/ui/blocked_content/blocked_content_container.cc @@ -50,8 +50,6 @@ void BlockedContentContainer::AddTabContents(TabContentsWrapper* tab_contents, blocked_contents_.push_back( BlockedContent(tab_contents, disposition, bounds, user_gesture)); - // TODO(avi): remove once TabContentsDelegate::GetConstrainingContents goes - // away. tab_contents->tab_contents()->set_delegate(this); tab_contents->blocked_content_tab_helper()->set_delegate(this); // Since the new tab_contents will not be shown, call WasHidden to change @@ -70,8 +68,6 @@ void BlockedContentContainer::LaunchForContents( BlockedContent content(*i); blocked_contents_.erase(i); i = blocked_contents_.end(); - // TODO(avi): remove once TabContentsDelegate::GetConstrainingContents - // goes away. tab_contents->tab_contents()->set_delegate(NULL); tab_contents->blocked_content_tab_helper()->set_delegate(NULL); // We needn't call WasRestored to change its status because the @@ -102,8 +98,6 @@ void BlockedContentContainer::Clear() { for (BlockedContents::iterator i(blocked_contents_.begin()); i != blocked_contents_.end(); ++i) { TabContentsWrapper* tab_contents = i->tab_contents; - // TODO(avi): remove once TabContentsDelegate::GetConstrainingContents goes - // away. tab_contents->tab_contents()->set_delegate(NULL); tab_contents->blocked_content_tab_helper()->set_delegate(NULL); delete tab_contents; @@ -134,8 +128,6 @@ void BlockedContentContainer::CloseContents(TabContents* source) { i != blocked_contents_.end(); ++i) { TabContentsWrapper* tab_contents = i->tab_contents; if (tab_contents->tab_contents() == source) { - // TODO(avi): remove once TabContentsDelegate::GetConstrainingContents - // goes away. tab_contents->tab_contents()->set_delegate(NULL); tab_contents->blocked_content_tab_helper()->set_delegate(NULL); blocked_contents_.erase(i); @@ -169,11 +161,6 @@ bool BlockedContentContainer::ShouldSuppressDialogs() { return true; } -TabContents* BlockedContentContainer::GetConstrainingContents( - TabContents* source) { - return owner_->tab_contents(); -} - TabContentsWrapper* BlockedContentContainer::GetConstrainingContentsWrapper( TabContentsWrapper* source) { return owner_; diff --git a/chrome/browser/ui/blocked_content/blocked_content_container.h b/chrome/browser/ui/blocked_content/blocked_content_container.h index b6ecde9..d1c2cf6 100644 --- a/chrome/browser/ui/blocked_content/blocked_content_container.h +++ b/chrome/browser/ui/blocked_content/blocked_content_container.h @@ -50,8 +50,7 @@ class BlockedContentContainer : public BlockedContentTabHelperDelegate, // Removes all blocked contents. void Clear(); - // Overridden from BlockedContentTabHelperDelegate, TabContentsDelegate: - virtual TabContents* GetConstrainingContents(TabContents* source) OVERRIDE; + // Overridden from BlockedContentTabHelperDelegate: virtual TabContentsWrapper* GetConstrainingContentsWrapper( TabContentsWrapper* source) OVERRIDE; diff --git a/content/browser/tab_contents/tab_contents_delegate.cc b/content/browser/tab_contents/tab_contents_delegate.cc index df709a7..04f6ce2 100644 --- a/content/browser/tab_contents/tab_contents_delegate.cc +++ b/content/browser/tab_contents/tab_contents_delegate.cc @@ -24,10 +24,6 @@ bool TabContentsDelegate::IsPopupOrPanel(const TabContents* source) const { return false; } -TabContents* TabContentsDelegate::GetConstrainingContents(TabContents* source) { - return source; -} - bool TabContentsDelegate::ShouldFocusConstrainedWindow() { return true; } diff --git a/content/browser/tab_contents/tab_contents_delegate.h b/content/browser/tab_contents/tab_contents_delegate.h index 0645128..3ddbbf8 100644 --- a/content/browser/tab_contents/tab_contents_delegate.h +++ b/content/browser/tab_contents/tab_contents_delegate.h @@ -118,12 +118,6 @@ class TabContentsDelegate { // or a panel window. virtual bool IsPopupOrPanel(const TabContents* source) const; - // If |source| is constrained, returns the tab containing it. Otherwise - // returns |source|. TODO(avi): Remove in favor of GetConstrainingContents on - // ContentSettingsTabHelperDelegate once uses of it in TabContents are - // removed. - virtual TabContents* GetConstrainingContents(TabContents* source); - // Returns true if constrained windows should be focused. Default is true. virtual bool ShouldFocusConstrainedWindow(); |