chrome/browser/download/download_path_reservation_tracker_unittest.cc - Issue 2453633006: [downloads] Move platform specific code out of DownloadTargetDeterminer.

Unified Diff: chrome/browser/download/download_path_reservation_tracker_unittest.cc

Issue 2453633006: [downloads] Move platform specific code out of DownloadTargetDeterminer. (Closed)

Patch Set: . Created 4 years, 2 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« chrome/browser/download/download_path_reservation_tracker.h ('K') | « chrome/browser/download/download_path_reservation_tracker.cc ('k') | chrome/browser/download/download_target_determiner.h » ('j') | chrome/browser/download/download_target_determiner_delegate.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/download/download_path_reservation_tracker_unittest.cc

diff --git a/chrome/browser/download/download_path_reservation_tracker_unittest.cc b/chrome/browser/download/download_path_reservation_tracker_unittest.cc

index 596740768b1bcf7c6c6eba6f08ce9b3c6dcf5062..5f5be50b1262365db13e761e0eed6f3c69c3dfb8 100644

--- a/chrome/browser/download/download_path_reservation_tracker_unittest.cc

+++ b/chrome/browser/download/download_path_reservation_tracker_unittest.cc

@@ -5,6 +5,8 @@

#include <stddef.h>

#include <stdint.h>

+#include <memory>

#include "base/files/file_path.h"

#include "base/files/file_util.h"

#include "base/files/scoped_temp_dir.h"

@@ -20,6 +22,7 @@

#include "chrome/common/features.h"

#include "content/public/test/mock_download_item.h"

#include "content/public/test/test_browser_thread.h"

+#include "net/base/filename_util.h"

#include "testing/gmock/include/gmock/gmock.h"

#include "testing/gtest/include/gtest/gtest.h"

@@ -51,7 +54,7 @@ class DownloadPathReservationTrackerTest : public testing::Test {

bool create_directory,

DownloadPathReservationTracker::FilenameConflictAction conflict_action,

base::FilePath* return_path,

- bool* return_verified);

+ DownloadTargetResult* return_result);

const base::FilePath& default_download_path() const {

return default_download_path_;

@@ -61,7 +64,8 @@ class DownloadPathReservationTrackerTest : public testing::Test {

}

// Creates a name of form 'a'*repeat + suffix

base::FilePath GetLongNamePathInDownloadsDirectory(

- size_t repeat, const base::FilePath::CharType* suffix);

+ size_t repeat,

+ const base::FilePath::CharType* suffix);

protected:

base::ScopedTempDir test_download_dir_;

@@ -72,8 +76,10 @@ class DownloadPathReservationTrackerTest : public testing::Test {

private:

void TestReservedPathCallback(base::FilePath* return_path,

- bool* return_verified, bool* did_run_callback,

- const base::FilePath& path, bool verified);

+ DownloadTargetResult* return_result,

+ const base::Closure& quit_closure,

+ const base::FilePath& path,

+ DownloadTargetResult result);

};

DownloadPathReservationTrackerTest::DownloadPathReservationTrackerTest()

@@ -99,6 +105,7 @@ MockDownloadItem* DownloadPathReservationTrackerTest::CreateDownloadItem(

.WillRepeatedly(ReturnRefOfCopy(base::FilePath()));

EXPECT_CALL(*item, GetState())

.WillRepeatedly(Return(DownloadItem::IN_PROGRESS));

+ EXPECT_CALL(*item, GetURL()).WillRepeatedly(ReturnRefOfCopy(GURL()));

return item;

}

@@ -118,31 +125,30 @@ void DownloadPathReservationTrackerTest::CallGetReservedPath(

bool create_directory,

DownloadPathReservationTracker::FilenameConflictAction conflict_action,

base::FilePath* return_path,

- bool* return_verified) {

+ DownloadTargetResult* return_result) {

// Weak pointer factory to prevent the callback from running after this

// function has returned.

base::WeakPtrFactory<DownloadPathReservationTrackerTest> weak_ptr_factory(

this);

- bool did_run_callback = false;

+ base::RunLoop run_loop;

DownloadPathReservationTracker::GetReservedPath(

- download_item,

- target_path,

- default_download_path(),

- create_directory,

+ download_item, target_path, default_download_path(), create_directory,

conflict_action,

base::Bind(&DownloadPathReservationTrackerTest::TestReservedPathCallback,

- weak_ptr_factory.GetWeakPtr(), return_path, return_verified,

- &did_run_callback));

- base::RunLoop().RunUntilIdle();

- EXPECT_TRUE(did_run_callback);

+ weak_ptr_factory.GetWeakPtr(), return_path, return_result,

+ run_loop.QuitClosure()));

+ run_loop.Run();

}

void DownloadPathReservationTrackerTest::TestReservedPathCallback(

- base::FilePath* return_path, bool* return_verified, bool* did_run_callback,

- const base::FilePath& path, bool verified) {

- *did_run_callback = true;

+ base::FilePath* return_path,

+ DownloadTargetResult* return_result,

+ const base::Closure& quit_closure,

+ const base::FilePath& path,

+ DownloadTargetResult result) {

*return_path = path;

- *return_verified = verified;

+ *return_result = result;

+ quit_closure.Run();

}

base::FilePath

@@ -170,19 +176,14 @@ TEST_F(DownloadPathReservationTrackerTest, BasicReservation) {

ASSERT_FALSE(IsPathInUse(path));

base::FilePath reserved_path;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

DownloadPathReservationTracker::FilenameConflictAction conflict_action =

- DownloadPathReservationTracker::OVERWRITE;

+ DownloadPathReservationTracker::OVERWRITE;

bool create_directory = false;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

EXPECT_TRUE(IsPathInUse(path));

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

EXPECT_EQ(path.value(), reserved_path.value());

// Destroying the item should release the reservation.

@@ -200,19 +201,14 @@ TEST_F(DownloadPathReservationTrackerTest, InterruptedDownload) {

ASSERT_FALSE(IsPathInUse(path));

base::FilePath reserved_path;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

DownloadPathReservationTracker::FilenameConflictAction conflict_action =

- DownloadPathReservationTracker::OVERWRITE;

+ DownloadPathReservationTracker::OVERWRITE;

bool create_directory = false;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

EXPECT_TRUE(IsPathInUse(path));

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

EXPECT_EQ(path.value(), reserved_path.value());

// Once the download is interrupted, the path should become available again.

@@ -229,19 +225,14 @@ TEST_F(DownloadPathReservationTrackerTest, CompleteDownload) {

ASSERT_FALSE(IsPathInUse(path));

base::FilePath reserved_path;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

DownloadPathReservationTracker::FilenameConflictAction conflict_action =

- DownloadPathReservationTracker::OVERWRITE;

+ DownloadPathReservationTracker::OVERWRITE;

bool create_directory = false;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

EXPECT_TRUE(IsPathInUse(path));

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

EXPECT_EQ(path.value(), reserved_path.value());

// Once the download completes, the path should become available again. For a

@@ -269,20 +260,15 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingFiles) {

ASSERT_TRUE(IsPathInUse(path));

base::FilePath reserved_path;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

bool create_directory = false;

DownloadPathReservationTracker::FilenameConflictAction conflict_action =

- DownloadPathReservationTracker::UNIQUIFY;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ DownloadPathReservationTracker::UNIQUIFY;

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

EXPECT_TRUE(IsPathInUse(path));

EXPECT_TRUE(IsPathInUse(reserved_path));

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

// The path should be uniquified, skipping over foo.txt but not over

// "foo (1).txt.crdownload"

EXPECT_EQ(

@@ -296,6 +282,33 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingFiles) {

EXPECT_FALSE(IsPathInUse(reserved_path));

}

+// If there are conflicting files on the file system, an overwriting reservation

+// should succeed without altering the target path.

+TEST_F(DownloadPathReservationTrackerTest, ConflictingFiles_Overwrite) {

+ std::unique_ptr<MockDownloadItem> item(CreateDownloadItem(1));

+ base::FilePath path(

+ GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo.txt")));

+ // Create a file at |path|.

+ ASSERT_EQ(0, base::WriteFile(path, "", 0));

+ ASSERT_TRUE(IsPathInUse(path));

+ base::FilePath reserved_path;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

+ bool create_directory = false;

+ DownloadPathReservationTracker::FilenameConflictAction conflict_action =

+ DownloadPathReservationTracker::OVERWRITE;

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

+ EXPECT_TRUE(IsPathInUse(path));

+ EXPECT_TRUE(IsPathInUse(reserved_path));

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

+ EXPECT_EQ(path.value(), reserved_path.value());

+ SetDownloadItemState(item.get(), DownloadItem::COMPLETE);

+ item.reset();

+ base::RunLoop().RunUntilIdle();

// Multiple reservations for the same path should uniquify around each other.

TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {

std::unique_ptr<MockDownloadItem> item1(CreateDownloadItem(1));

@@ -307,34 +320,23 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {

ASSERT_FALSE(IsPathInUse(uniquified_path));

base::FilePath reserved_path1;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

bool create_directory = false;

DownloadPathReservationTracker::FilenameConflictAction conflict_action =

DownloadPathReservationTracker::UNIQUIFY;

- CallGetReservedPath(

- item1.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path1,

- &verified);

+ CallGetReservedPath(item1.get(), path, create_directory, conflict_action,

+ &reserved_path1, &result);

EXPECT_TRUE(IsPathInUse(path));

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

{

// Requesting a reservation for the same path with uniquification results in

// a uniquified path.

std::unique_ptr<MockDownloadItem> item2(CreateDownloadItem(2));

base::FilePath reserved_path2;

- CallGetReservedPath(

- item2.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path2,

- &verified);

+ CallGetReservedPath(item2.get(), path, create_directory, conflict_action,

+ &reserved_path2, &result);

EXPECT_TRUE(IsPathInUse(path));

EXPECT_TRUE(IsPathInUse(uniquified_path));

EXPECT_EQ(uniquified_path.value(), reserved_path2.value());

@@ -349,13 +351,8 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {

// for the same path should result in the same uniquified path.

std::unique_ptr<MockDownloadItem> item2(CreateDownloadItem(2));

base::FilePath reserved_path2;

- CallGetReservedPath(

- item2.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path2,

- &verified);

+ CallGetReservedPath(item2.get(), path, create_directory, conflict_action,

+ &reserved_path2, &result);

EXPECT_TRUE(IsPathInUse(path));

EXPECT_TRUE(IsPathInUse(uniquified_path));

EXPECT_EQ(uniquified_path.value(), reserved_path2.value());

@@ -368,13 +365,8 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) {

std::unique_ptr<MockDownloadItem> item3(CreateDownloadItem(2));

base::FilePath reserved_path3;

conflict_action = DownloadPathReservationTracker::OVERWRITE;

- CallGetReservedPath(

- item3.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path3,

- &verified);

+ CallGetReservedPath(item3.get(), path, create_directory, conflict_action,

+ &reserved_path3, &result);

EXPECT_TRUE(IsPathInUse(path));

EXPECT_FALSE(IsPathInUse(uniquified_path));

@@ -397,12 +389,12 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingCaseReservations) {

GetPathInDownloadsDirectory(FILE_PATH_LITERAL("Foo.txt"));

base::FilePath first_reservation;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::PATH_NOT_WRITEABLE;

CallGetReservedPath(item1.get(), path_foo, false,

DownloadPathReservationTracker::UNIQUIFY,

- &first_reservation, &verified);

+ &first_reservation, &result);

EXPECT_TRUE(IsPathInUse(path_foo));

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

EXPECT_EQ(path_foo, first_reservation);

// Foo should also be in use at this point.

@@ -411,8 +403,8 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingCaseReservations) {

base::FilePath second_reservation;

CallGetReservedPath(item2.get(), path_Foo, false,

DownloadPathReservationTracker::UNIQUIFY,

- &second_reservation, &verified);

- EXPECT_TRUE(verified);

+ &second_reservation, &result);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

EXPECT_EQ(GetPathInDownloadsDirectory(FILE_PATH_LITERAL("Foo (1).txt")),

second_reservation);

SetDownloadItemState(item1.get(), DownloadItem::COMPLETE);

@@ -436,7 +428,7 @@ TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) {

for (int i = 0; i <= DownloadPathReservationTracker::kMaxUniqueFiles; i++) {

base::FilePath reserved_path;

base::FilePath expected_path;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

if (i > 0) {

expected_path =

path.InsertBeforeExtensionASCII(base::StringPrintf(" (%d)", i));

@@ -445,30 +437,20 @@ TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) {

}

items[i].reset(CreateDownloadItem(i));

EXPECT_FALSE(IsPathInUse(expected_path));

- CallGetReservedPath(

- items[i].get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ CallGetReservedPath(items[i].get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

EXPECT_TRUE(IsPathInUse(expected_path));

EXPECT_EQ(expected_path.value(), reserved_path.value());

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

}

// The next reservation for |path| will fail to be unique.

std::unique_ptr<MockDownloadItem> item(

CreateDownloadItem(DownloadPathReservationTracker::kMaxUniqueFiles + 1));

base::FilePath reserved_path;

- bool verified = true;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

- EXPECT_FALSE(verified);

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

+ EXPECT_EQ(DownloadTargetResult::CONFLICT, result);

EXPECT_EQ(path.value(), reserved_path.value());

SetDownloadItemState(item.get(), DownloadItem::COMPLETE);

@@ -490,19 +472,14 @@ TEST_F(DownloadPathReservationTrackerTest, UnwriteableDirectory) {

base::FilePermissionRestorer restorer(dir);

EXPECT_TRUE(base::MakeFileUnwritable(dir));

base::FilePath reserved_path;

- bool verified = true;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

DownloadPathReservationTracker::FilenameConflictAction conflict_action =

- DownloadPathReservationTracker::OVERWRITE;

+ DownloadPathReservationTracker::OVERWRITE;

bool create_directory = false;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

// Verification fails.

- EXPECT_FALSE(verified);

+ EXPECT_EQ(DownloadTargetResult::PATH_NOT_WRITEABLE, result);

#if BUILDFLAG(ANDROID_JAVA_UI)

EXPECT_TRUE(reserved_path.empty());

#else

@@ -526,33 +503,23 @@ TEST_F(DownloadPathReservationTrackerTest, CreateDefaultDownloadPath) {

{

std::unique_ptr<MockDownloadItem> item(CreateDownloadItem(1));

base::FilePath reserved_path;

- bool verified = true;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

// Verification fails because the directory doesn't exist.

- EXPECT_FALSE(verified);

+ EXPECT_EQ(DownloadTargetResult::PATH_NOT_WRITEABLE, result);

SetDownloadItemState(item.get(), DownloadItem::COMPLETE);

}

ASSERT_FALSE(IsPathInUse(path));

{

std::unique_ptr<MockDownloadItem> item(CreateDownloadItem(1));

base::FilePath reserved_path;

- bool verified = true;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

set_default_download_path(dir);

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

// Verification succeeds because the directory is created.

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

EXPECT_TRUE(base::DirectoryExists(dir));

SetDownloadItemState(item.get(), DownloadItem::COMPLETE);

}

@@ -567,19 +534,14 @@ TEST_F(DownloadPathReservationTrackerTest, UpdatesToTargetPath) {

ASSERT_FALSE(IsPathInUse(path));

base::FilePath reserved_path;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

DownloadPathReservationTracker::FilenameConflictAction conflict_action =

- DownloadPathReservationTracker::OVERWRITE;

+ DownloadPathReservationTracker::OVERWRITE;

bool create_directory = false;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

EXPECT_TRUE(IsPathInUse(path));

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

EXPECT_EQ(path.value(), reserved_path.value());

// The target path is initially empty. If an OnDownloadUpdated() is issued in

@@ -626,19 +588,14 @@ TEST_F(DownloadPathReservationTrackerTest, BasicTruncation) {

ASSERT_FALSE(IsPathInUse(path));

base::FilePath reserved_path;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

DownloadPathReservationTracker::FilenameConflictAction conflict_action =

- DownloadPathReservationTracker::OVERWRITE;

+ DownloadPathReservationTracker::OVERWRITE;

bool create_directory = false;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

EXPECT_TRUE(IsPathInUse(reserved_path));

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

// The file name length is truncated to max_length.

EXPECT_EQ(max_length, reserved_path.BaseName().value().size());

// But the extension is kept unchanged.

@@ -669,19 +626,14 @@ TEST_F(DownloadPathReservationTrackerTest, TruncationConflict) {

ASSERT_EQ(0, base::WriteFile(path1, "", 0));

base::FilePath reserved_path;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::NAME_TOO_LONG;

DownloadPathReservationTracker::FilenameConflictAction conflict_action =

DownloadPathReservationTracker::UNIQUIFY;

bool create_directory = false;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

EXPECT_TRUE(IsPathInUse(reserved_path));

- EXPECT_TRUE(verified);

+ EXPECT_EQ(DownloadTargetResult::SUCCESS, result);

EXPECT_EQ(path2, reserved_path);

SetDownloadItemState(item.get(), DownloadItem::COMPLETE);

}

@@ -699,19 +651,14 @@ TEST_F(DownloadPathReservationTrackerTest, TruncationFail) {

ASSERT_FALSE(IsPathInUse(path));

base::FilePath reserved_path;

- bool verified = false;

+ DownloadTargetResult result = DownloadTargetResult::SUCCESS;

DownloadPathReservationTracker::FilenameConflictAction conflict_action =

- DownloadPathReservationTracker::OVERWRITE;

+ DownloadPathReservationTracker::OVERWRITE;

bool create_directory = false;

- CallGetReservedPath(

- item.get(),

- path,

- create_directory,

- conflict_action,

- &reserved_path,

- &verified);

+ CallGetReservedPath(item.get(), path, create_directory, conflict_action,

+ &reserved_path, &result);

// We cannot truncate a path with very long extension.

- EXPECT_FALSE(verified);

+ EXPECT_EQ(DownloadTargetResult::NAME_TOO_LONG, result);

SetDownloadItemState(item.get(), DownloadItem::COMPLETE);

}