Commit d3fe4f47 authored by Eugene But's avatar Eugene But Committed by Commit Bot

Run confirmation dialog before replacing in progress download.

There is no unit test for tapping OK and Cancel buttons, because
it's something that is very hard to test in unit test (buttons are
not tappable unless the UI got synchronized).

This CL adds "Start New Download?" dialog when user attemps to create a
download while another download is in progress.

UI mock: https://docs.google.com/presentation/d/1GzbAoJrpW9IAQF78afh5SZLWJWErNcC67t_ctujjEus/edit#slide=id.g2b7a689b42_0_196

Bug: 791806
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Icde65bf68d4ee0ff4181165a4d812f9e16e8de9e
Reviewed-on: https://chromium-review.googlesource.com/924750
Commit-Queue: Eugene But <eugenebut@chromium.org>
Reviewed-by: default avatarSylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537811}
parent 25672cf1
...@@ -53,6 +53,10 @@ class DownloadManagerTabHelper ...@@ -53,6 +53,10 @@ class DownloadManagerTabHelper
// Returns key for using with NetworkActivityIndicatorManager. // Returns key for using with NetworkActivityIndicatorManager.
NSString* GetNetworkActivityKey() const; NSString* GetNetworkActivityKey() const;
// Assigns |task| to |task_|; replaces the current download if exists;
// instructs the delegate that download has started.
void DidCreateDownload(std::unique_ptr<web::DownloadTask> task);
web::WebState* web_state_ = nullptr; web::WebState* web_state_ = nullptr;
__weak id<DownloadManagerTabHelperDelegate> delegate_ = nil; __weak id<DownloadManagerTabHelperDelegate> delegate_ = nil;
std::unique_ptr<web::DownloadTask> task_; std::unique_ptr<web::DownloadTask> task_;
......
...@@ -41,11 +41,28 @@ void DownloadManagerTabHelper::CreateForWebState( ...@@ -41,11 +41,28 @@ void DownloadManagerTabHelper::CreateForWebState(
void DownloadManagerTabHelper::Download( void DownloadManagerTabHelper::Download(
std::unique_ptr<web::DownloadTask> task) { std::unique_ptr<web::DownloadTask> task) {
task_ = std::move(task); __block std::unique_ptr<web::DownloadTask> block_task = std::move(task);
task_->AddObserver(this); if (!task_) {
// The task is the first download for this web state.
DidCreateDownload(std::move(block_task));
return;
}
// Another download is already in progress. Ask the user if current download
// should be replaced if new download was initiated by a link click. Otherwise
// silently drop the download to prevent web pages from spamming the user.
if (!ui::PageTransitionTypeIncludingQualifiersIs(
block_task->GetTransitionType(), ui::PAGE_TRANSITION_LINK)) {
return;
}
[delegate_ downloadManagerTabHelper:this [delegate_ downloadManagerTabHelper:this
didCreateDownload:task_.get() decidePolicyForDownload:block_task.get()
webStateIsVisible:web_state_->IsVisible()]; completionHandler:^(NewDownloadPolicy policy) {
if (policy == kNewDownloadPolicyReplace) {
DidCreateDownload(std::move(block_task));
}
}];
} }
void DownloadManagerTabHelper::WasShown(web::WebState* web_state) { void DownloadManagerTabHelper::WasShown(web::WebState* web_state) {
...@@ -97,3 +114,15 @@ NSString* DownloadManagerTabHelper::GetNetworkActivityKey() const { ...@@ -97,3 +114,15 @@ NSString* DownloadManagerTabHelper::GetNetworkActivityKey() const {
return [@"DownloadManagerTabHelper." return [@"DownloadManagerTabHelper."
stringByAppendingString:task_->GetIndentifier()]; stringByAppendingString:task_->GetIndentifier()];
} }
void DownloadManagerTabHelper::DidCreateDownload(
std::unique_ptr<web::DownloadTask> task) {
if (task_) {
task_->RemoveObserver(this);
}
task_ = std::move(task);
task_->AddObserver(this);
[delegate_ downloadManagerTabHelper:this
didCreateDownload:task_.get()
webStateIsVisible:web_state_->IsVisible()];
}
...@@ -12,6 +12,15 @@ namespace web { ...@@ -12,6 +12,15 @@ namespace web {
class DownloadTask; class DownloadTask;
} // namespace web } // namespace web
// Whether or not the new download task should replace the old download task.
typedef NS_ENUM(NSInteger, NewDownloadPolicy) {
// Old download task should be discarded and replaced with a new download
// task.
kNewDownloadPolicyReplace = 0,
// Old download task should be kept. A new download task should be discarded.
kNewDownloadPolicyDiscard,
};
// Delegate for DownloadManagerTabHelper class. // Delegate for DownloadManagerTabHelper class.
@protocol DownloadManagerTabHelperDelegate<NSObject> @protocol DownloadManagerTabHelperDelegate<NSObject>
...@@ -20,6 +29,12 @@ class DownloadTask; ...@@ -20,6 +29,12 @@ class DownloadTask;
didCreateDownload:(nonnull web::DownloadTask*)download didCreateDownload:(nonnull web::DownloadTask*)download
webStateIsVisible:(BOOL)webStateIsVisible; webStateIsVisible:(BOOL)webStateIsVisible;
// Asks the delegate whether the new download task should replace the old
// download task.
- (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper
decidePolicyForDownload:(nonnull web::DownloadTask*)download
completionHandler:(nonnull void (^)(NewDownloadPolicy))handler;
// Informs the delegate that WebState related to this download was hidden. // Informs the delegate that WebState related to this download was hidden.
- (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper - (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper
didHideDownload:(nonnull web::DownloadTask*)download; didHideDownload:(nonnull web::DownloadTask*)download;
......
...@@ -40,7 +40,7 @@ class DownloadManagerTabHelperTest : public PlatformTest { ...@@ -40,7 +40,7 @@ class DownloadManagerTabHelperTest : public PlatformTest {
FakeDownloadManagerTabHelperDelegate* delegate_; FakeDownloadManagerTabHelperDelegate* delegate_;
}; };
// Tests that created download has NotStarted statefor visible web state. // Tests that created download has NotStarted state for visible web state.
TEST_F(DownloadManagerTabHelperTest, DownloadCreationForVisibleWebState) { TEST_F(DownloadManagerTabHelperTest, DownloadCreationForVisibleWebState) {
web_state_->WasShown(); web_state_->WasShown();
ASSERT_FALSE(delegate_.state); ASSERT_FALSE(delegate_.state);
...@@ -52,6 +52,76 @@ TEST_F(DownloadManagerTabHelperTest, DownloadCreationForVisibleWebState) { ...@@ -52,6 +52,76 @@ TEST_F(DownloadManagerTabHelperTest, DownloadCreationForVisibleWebState) {
EXPECT_EQ(web::DownloadTask::State::kNotStarted, *delegate_.state); EXPECT_EQ(web::DownloadTask::State::kNotStarted, *delegate_.state);
} }
// Tests creating the second download while the first download is still in
// progress. Second download should be rejected because its transition type is
// not ui::PAGE_TRANSITION_LINK.
TEST_F(DownloadManagerTabHelperTest, DownloadRejection) {
web_state_->WasShown();
ASSERT_FALSE(delegate_.state);
auto task = std::make_unique<web::FakeDownloadTask>(GURL(kUrl), kMimeType);
task->SetDone(true);
tab_helper()->Download(std::move(task));
auto task2 = std::make_unique<web::FakeDownloadTask>(GURL(kUrl), kMimeType);
tab_helper()->Download(std::move(task2));
// The state of the old download task is kComplete.
ASSERT_TRUE(delegate_.state);
EXPECT_EQ(web::DownloadTask::State::kComplete, *delegate_.state);
}
// Tests creating the second download while the first download is still in
// progress. Second download will be rejected by the delegate.
TEST_F(DownloadManagerTabHelperTest, DownloadRejectionViaDelegate) {
web_state_->WasShown();
ASSERT_FALSE(delegate_.state);
auto task = std::make_unique<web::FakeDownloadTask>(GURL(kUrl), kMimeType);
task->SetDone(true);
tab_helper()->Download(std::move(task));
auto task2 = std::make_unique<web::FakeDownloadTask>(GURL(kUrl), kMimeType);
const web::FakeDownloadTask* task2_ptr = task2.get();
task2->SetTransitionType(ui::PAGE_TRANSITION_LINK);
tab_helper()->Download(std::move(task2));
ASSERT_TRUE(delegate_.state);
EXPECT_EQ(web::DownloadTask::State::kComplete, *delegate_.state);
EXPECT_EQ(task2_ptr, delegate_.decidingPolicyForDownload);
// Ask the delegate to discard the new download.
BOOL discarded = [delegate_ decidePolicy:kNewDownloadPolicyDiscard];
ASSERT_TRUE(discarded);
// The state of the old download task is kComplete.
ASSERT_TRUE(delegate_.state);
EXPECT_EQ(web::DownloadTask::State::kComplete, *delegate_.state);
}
// Tests creating the second download while the first download is still in
// progress. Second download will be acccepted by the delegate.
TEST_F(DownloadManagerTabHelperTest, DownloadReplacingViaDelegate) {
web_state_->WasShown();
ASSERT_FALSE(delegate_.state);
auto task = std::make_unique<web::FakeDownloadTask>(GURL(kUrl), kMimeType);
task->SetDone(true);
tab_helper()->Download(std::move(task));
auto task2 = std::make_unique<web::FakeDownloadTask>(GURL(kUrl), kMimeType);
const web::FakeDownloadTask* task2_ptr = task2.get();
task2->SetTransitionType(ui::PAGE_TRANSITION_LINK);
tab_helper()->Download(std::move(task2));
ASSERT_TRUE(delegate_.state);
EXPECT_EQ(web::DownloadTask::State::kComplete, *delegate_.state);
EXPECT_EQ(task2_ptr, delegate_.decidingPolicyForDownload);
// Ask the delegate to replace the new download.
BOOL replaced = [delegate_ decidePolicy:kNewDownloadPolicyReplace];
ASSERT_TRUE(replaced);
// The state of a new download task is kNotStarted.
ASSERT_TRUE(delegate_.state);
EXPECT_EQ(web::DownloadTask::State::kNotStarted, *delegate_.state);
}
// Tests that created download has null state for hidden web state. // Tests that created download has null state for hidden web state.
TEST_F(DownloadManagerTabHelperTest, DownloadCreationForHiddenWebState) { TEST_F(DownloadManagerTabHelperTest, DownloadCreationForHiddenWebState) {
web_state_->WasHidden(); web_state_->WasHidden();
......
...@@ -84,6 +84,24 @@ ...@@ -84,6 +84,24 @@
[self start]; [self start];
} }
- (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper
decidePolicyForDownload:(nonnull web::DownloadTask*)download
completionHandler:(nonnull void (^)(NewDownloadPolicy))handler {
// TODO(crbug.com/805533): Localize those strings.
NSString* message = @"This will stop all progress for your current download.";
__weak DownloadManagerCoordinator* weakSelf = self;
[self runConfirmationDialogWithTitle:@"Start New Download?"
message:message
completionHandler:^(BOOL confirmed) {
DownloadManagerCoordinator* strongSelf = weakSelf;
if (strongSelf) {
strongSelf->_mediator.SetDownloadTask(nullptr);
}
handler(confirmed ? kNewDownloadPolicyReplace
: kNewDownloadPolicyDiscard);
}];
}
- (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper - (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper
didHideDownload:(nonnull web::DownloadTask*)download { didHideDownload:(nonnull web::DownloadTask*)download {
if (!_downloadTask) { if (!_downloadTask) {
......
...@@ -268,6 +268,33 @@ TEST_F(DownloadManagerCoordinatorTest, CloseInProgressDownload) { ...@@ -268,6 +268,33 @@ TEST_F(DownloadManagerCoordinatorTest, CloseInProgressDownload) {
})); }));
} }
// Tests downloadManagerTabHelper:decidePolicyForDownload:completionHandler:.
// Coordinator should present the confirmation dialog.
TEST_F(DownloadManagerCoordinatorTest, DecidePolicyForDownload) {
web::FakeDownloadTask task(GURL(kTestUrl), kTestMimeType);
[coordinator_ downloadManagerTabHelper:&tab_helper_
decidePolicyForDownload:&task
completionHandler:^(NewDownloadPolicy){
}];
// Verify that UIAlert is presented.
ASSERT_TRUE([base_view_controller_.presentedViewController
isKindOfClass:[UIAlertController class]]);
UIAlertController* alert = base::mac::ObjCCast<UIAlertController>(
base_view_controller_.presentedViewController);
EXPECT_NSEQ(@"Start New Download?", alert.title);
EXPECT_NSEQ(@"This will stop all progress for your current download.",
alert.message);
// Stop to avoid holding a dangling pointer to destroyed task.
[coordinator_ stop];
// |stop| should dismiss the apert.
ASSERT_TRUE(WaitUntilConditionOrTimeout(testing::kWaitForUIElementTimeout, ^{
return !base_view_controller_.presentedViewController;
}));
}
// Tests starting the download. Verifies that download task is started and its // Tests starting the download. Verifies that download task is started and its
// file writer is configured to write into download directory. // file writer is configured to write into download directory.
TEST_F(DownloadManagerCoordinatorTest, StartDownload) { TEST_F(DownloadManagerCoordinatorTest, StartDownload) {
......
...@@ -5,6 +5,7 @@ ...@@ -5,6 +5,7 @@
#ifndef IOS_CHROME_TEST_FAKES_FAKE_DOWNLOAD_MANAGER_TAB_HELPER_DELEGATE_H_ #ifndef IOS_CHROME_TEST_FAKES_FAKE_DOWNLOAD_MANAGER_TAB_HELPER_DELEGATE_H_
#define IOS_CHROME_TEST_FAKES_FAKE_DOWNLOAD_MANAGER_TAB_HELPER_DELEGATE_H_ #define IOS_CHROME_TEST_FAKES_FAKE_DOWNLOAD_MANAGER_TAB_HELPER_DELEGATE_H_
#include "base/compiler_specific.h"
#import "ios/chrome/browser/download/download_manager_tab_helper_delegate.h" #import "ios/chrome/browser/download/download_manager_tab_helper_delegate.h"
#import "ios/web/public/download/download_task.h" #import "ios/web/public/download/download_task.h"
...@@ -16,6 +17,17 @@ ...@@ -16,6 +17,17 @@
// or when DownloadManager's WebState was hidden. // or when DownloadManager's WebState was hidden.
@property(nonatomic, readonly) web::DownloadTask::State* state; @property(nonatomic, readonly) web::DownloadTask::State* state;
// Download task for which
// downloadManagerTabHelper:decidePolicyForDownload:completionHandler: was
// called. null if decidePolicyForDownload was never called or |decidePolicy:|
// was called.
@property(nonatomic, readonly) web::DownloadTask* decidingPolicyForDownload;
// Calls downloadManagerTabHelper:decidePolicyForDownload:completionHandler:
// completion handler. Returns YES if decidePolicyForDownload: was called.
// nulls out decidingPolicyForDownload.
- (BOOL)decidePolicy:(NewDownloadPolicy)policy WARN_UNUSED_RESULT;
@end @end
#endif // IOS_CHROME_TEST_FAKES_FAKE_DOWNLOAD_MANAGER_TAB_HELPER_DELEGATE_H_ #endif // IOS_CHROME_TEST_FAKES_FAKE_DOWNLOAD_MANAGER_TAB_HELPER_DELEGATE_H_
...@@ -8,14 +8,32 @@ ...@@ -8,14 +8,32 @@
#error "This file requires ARC support." #error "This file requires ARC support."
#endif #endif
using DecidePolicyForDownloadHandler = void (^)(NewDownloadPolicy);
@implementation FakeDownloadManagerTabHelperDelegate { @implementation FakeDownloadManagerTabHelperDelegate {
std::unique_ptr<web::DownloadTask::State> _state; std::unique_ptr<web::DownloadTask::State> _state;
web::DownloadTask* _decidingPolicyForDownload;
DecidePolicyForDownloadHandler _decidePolicyForDownloadHandler;
} }
- (web::DownloadTask::State*)state { - (web::DownloadTask::State*)state {
return _state.get(); return _state.get();
} }
- (web::DownloadTask*)decidingPolicyForDownload {
return _decidingPolicyForDownload;
}
- (BOOL)decidePolicy:(NewDownloadPolicy)policy {
if (!_decidePolicyForDownloadHandler)
return NO;
_decidePolicyForDownloadHandler(policy);
_decidingPolicyForDownload = nil;
_decidePolicyForDownloadHandler = nil;
return YES;
}
- (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper - (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper
didCreateDownload:(nonnull web::DownloadTask*)download didCreateDownload:(nonnull web::DownloadTask*)download
webStateIsVisible:(BOOL)webStateIsVisible { webStateIsVisible:(BOOL)webStateIsVisible {
...@@ -24,6 +42,13 @@ ...@@ -24,6 +42,13 @@
} }
} }
- (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper
decidePolicyForDownload:(nonnull web::DownloadTask*)download
completionHandler:(nonnull void (^)(NewDownloadPolicy))handler {
_decidingPolicyForDownload = download;
_decidePolicyForDownloadHandler = handler;
}
- (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper - (void)downloadManagerTabHelper:(nonnull DownloadManagerTabHelper*)tabHelper
didHideDownload:(nonnull web::DownloadTask*)download { didHideDownload:(nonnull web::DownloadTask*)download {
_state = nullptr; _state = nullptr;
......
...@@ -49,6 +49,7 @@ class FakeDownloadTask : public DownloadTask { ...@@ -49,6 +49,7 @@ class FakeDownloadTask : public DownloadTask {
void SetPercentComplete(int percent_complete); void SetPercentComplete(int percent_complete);
void SetContentDisposition(const std::string& content_disposition); void SetContentDisposition(const std::string& content_disposition);
void SetMimeType(const std::string& mime_type); void SetMimeType(const std::string& mime_type);
void SetTransitionType(ui::PageTransition page_transition);
void SetSuggestedFilename(const base::string16& suggested_file_name); void SetSuggestedFilename(const base::string16& suggested_file_name);
private: private:
......
...@@ -138,6 +138,11 @@ void FakeDownloadTask::SetMimeType(const std::string& mime_type) { ...@@ -138,6 +138,11 @@ void FakeDownloadTask::SetMimeType(const std::string& mime_type) {
OnDownloadUpdated(); OnDownloadUpdated();
} }
void FakeDownloadTask::SetTransitionType(ui::PageTransition page_transition) {
page_transition_ = page_transition;
OnDownloadUpdated();
}
void FakeDownloadTask::SetSuggestedFilename( void FakeDownloadTask::SetSuggestedFilename(
const base::string16& suggested_file_name) { const base::string16& suggested_file_name) {
suggested_file_name_ = suggested_file_name; suggested_file_name_ = suggested_file_name;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment