r137478 - in trunk/src/chrome: browser/download common/extensions/api

[benjh...@chromium.org]

Author: benjh...@chromium.org
Date: Wed May 16 12:18:13 2012
New Revision: 137478

Log:
Fix most of the downloads api in incognito: search(), pause(), resume(), cancel(), getFileIcon().

Review URL: https://chromiumcodereview.appspot.com/9425014

Modified:
trunk/src/chrome/browser/download/download_extension_api.cc
trunk/src/chrome/browser/download/download_extension_api.h
trunk/src/chrome/browser/download/download_extension_test.cc
trunk/src/chrome/common/extensions/api/experimental_downloads.json

Modified: trunk/src/chrome/browser/download/download_extension_api.cc
==============================================================================
--- trunk/src/chrome/browser/download/download_extension_api.cc (original)
+++ trunk/src/chrome/browser/download/download_extension_api.cc Wed May 16 12:18:13 2012
@@ -93,6 +93,7 @@
const char kHeaderBinaryValueKey[] = "binaryValue";
const char kHeadersKey[] = "headers";
const char kIdKey[] = "id";
+const char kIncognito[] = "incognito";
const char kLimitKey[] = "limit";
const char kMethodKey[] = "method";
const char kMimeKey[] = "mime";
@@ -196,6 +197,7 @@
(item->GetStartTime() - base::Time::UnixEpoch()).InMilliseconds());
json->SetInteger(kBytesReceivedKey, item->GetReceivedBytes());
json->SetInteger(kTotalBytesKey, item->GetTotalBytes());
+ json->SetBoolean(kIncognito, item->IsOtr());
if (item->GetState() == DownloadItem::INTERRUPTED) {
json->SetInteger(kErrorKey, static_cast<int>(item->GetLastReason()));
} else if (item->GetState() == DownloadItem::CANCELLED) {
@@ -307,6 +309,31 @@
return !item.IsTemporary();
}

+void GetManagers(
+ Profile* profile,
+ bool include_incognito,
+ DownloadManager** manager, DownloadManager** incognito_manager) {
+ *manager = DownloadServiceFactory::GetForProfile(profile)->
+ GetDownloadManager();
+ *incognito_manager = NULL;
+ if (include_incognito && profile->HasOffTheRecordProfile())
+ *incognito_manager = DownloadServiceFactory::GetForProfile(profile->
+ GetOffTheRecordProfile())->GetDownloadManager();
+}
+
+DownloadItem* GetActiveItemInternal(
+ Profile* profile,
+ bool include_incognito,
+ int id) {
+ DownloadManager* manager = NULL;
+ DownloadManager* incognito_manager = NULL;
+ GetManagers(profile, include_incognito, &manager, &incognito_manager);
+ DownloadItem* download_item = manager->GetActiveDownloadItem(id);
+ if (!download_item && incognito_manager)
+ download_item = incognito_manager->GetActiveDownloadItem(id);
+ return download_item;
+}
+
} // namespace

bool DownloadsFunctionInterface::RunImplImpl(
@@ -334,6 +361,10 @@
return function_;
}

+DownloadItem* SyncDownloadsFunction::GetActiveItem(int download_id) {
+ return GetActiveItemInternal(profile(), include_incognito(), download_id);
+}
+
AsyncDownloadsFunction::AsyncDownloadsFunction(
DownloadsFunctionInterface::DownloadsFunctionName function)
: function_(function) {
@@ -350,6 +381,10 @@
return function_;
}

+DownloadItem* AsyncDownloadsFunction::GetActiveItem(int download_id) {
+ return GetActiveItemInternal(profile(), include_incognito(), download_id);
+}
+
DownloadsDownloadFunction::DownloadsDownloadFunction()
: AsyncDownloadsFunction(DOWNLOADS_FUNCTION_DOWNLOAD) {
}
@@ -640,15 +675,21 @@
}

bool DownloadsSearchFunction::RunInternal() {
+ DownloadManager* manager = NULL;
+ DownloadManager* incognito_manager = NULL;
+ GetManagers(profile(), include_incognito(), &manager, &incognito_manager);
DownloadQuery::DownloadVector all_items, cpp_results;
- DownloadManager* manager = DownloadServiceFactory::GetForProfile(profile())
- ->GetDownloadManager();
if (has_get_id_) {
DownloadItem* item = manager->GetDownloadItem(get_id_);
- if (item != NULL)
+ if (!item && incognito_manager)
+ item = incognito_manager->GetDownloadItem(get_id_);
+ if (item)
all_items.push_back(item);
} else {
manager->GetAllDownloads(FilePath(FILE_PATH_LITERAL("")), &all_items);
+ if (incognito_manager)
+ incognito_manager->GetAllDownloads(
+ FilePath(FILE_PATH_LITERAL("")), &all_items);
}
query_->Search(all_items.begin(), all_items.end(), &cpp_results);
base::ListValue* json_results = new base::ListValue();
@@ -674,13 +715,8 @@
}

bool DownloadsPauseFunction::RunInternal() {
- DownloadManager* download_manager =
- DownloadServiceFactory::GetForProfile(profile())->GetDownloadManager();
- DownloadItem* download_item =
- download_manager->GetActiveDownloadItem(download_id_);
- DCHECK(!download_item || download_item->IsInProgress());
-
- if (!download_item) {
+ DownloadItem* download_item = GetActiveItem(download_id_);
+ if ((download_item == NULL) || !download_item->IsInProgress()) {
// This could be due to an invalid download ID, or it could be due to the
// download not being currently active.
error_ = download_extension_errors::kInvalidOperationError;
@@ -704,13 +740,8 @@
}

bool DownloadsResumeFunction::RunInternal() {
- DownloadManager* download_manager =
- DownloadServiceFactory::GetForProfile(profile())->GetDownloadManager();
- DownloadItem* download_item =
- download_manager->GetActiveDownloadItem(download_id_);
- DCHECK(!download_item || download_item->IsInProgress());
-
- if (!download_item) {
+ DownloadItem* download_item = GetActiveItem(download_id_);
+ if (download_item == NULL) {
// This could be due to an invalid download ID, or it could be due to the
// download not being currently active.
error_ = download_extension_errors::kInvalidOperationError;
@@ -734,12 +765,8 @@
}

bool DownloadsCancelFunction::RunInternal() {
- DownloadManager* download_manager =
- DownloadServiceFactory::GetForProfile(profile())->GetDownloadManager();
- DownloadItem* download_item =
- download_manager->GetActiveDownloadItem(download_id_);
-
- if (download_item)
+ DownloadItem* download_item = GetActiveItem(download_id_);
+ if (download_item != NULL)
download_item->Cancel(true);
// |download_item| can be NULL if the download ID was invalid or if the
// download is not currently active. Either way, we don't consider it a
@@ -870,10 +897,13 @@
DCHECK(icon_size_ == 16 || icon_size_ == 32);
}

- DownloadManager* download_manager =
- DownloadServiceFactory::GetForProfile(profile())->GetDownloadManager();
- DownloadItem* download_item = download_manager->GetDownloadItem(dl_id);
- if (download_item == NULL) {
+ DownloadManager* manager = NULL;
+ DownloadManager* incognito_manager = NULL;
+ GetManagers(profile(), include_incognito(), &manager, &incognito_manager);
+ DownloadItem* download_item = manager->GetDownloadItem(dl_id);
+ if (!download_item && incognito_manager)
+ download_item = incognito_manager->GetDownloadItem(dl_id);
+ if (!download_item) {
// The DownloadItem is is added to history when the path is determined. If
// the download is not in history, then we don't have a path / final
// filename and no icon.

Modified: trunk/src/chrome/browser/download/download_extension_api.h
==============================================================================
--- trunk/src/chrome/browser/download/download_extension_api.h (original)
+++ trunk/src/chrome/browser/download/download_extension_api.h Wed May 16 12:18:13 2012
@@ -93,6 +93,8 @@
// DownloadsFunctionInterface:
virtual DownloadsFunctionName function() const OVERRIDE;

+ content::DownloadItem* GetActiveItem(int download_id);
+
private:
DownloadsFunctionName function_;

@@ -111,6 +113,8 @@
// DownloadsFunctionInterface:
virtual DownloadsFunctionName function() const OVERRIDE;

+ content::DownloadItem* GetActiveItem(int download_id);
+
private:
DownloadsFunctionName function_;


Modified: trunk/src/chrome/browser/download/download_extension_test.cc
==============================================================================
--- trunk/src/chrome/browser/download/download_extension_test.cc (original)
+++ trunk/src/chrome/browser/download/download_extension_test.cc Wed May 16 12:18:13 2012
@@ -59,21 +59,23 @@
content::DownloadDangerType danger_type;
};

+ virtual Browser* current_browser() { return browser(); }
+
// InProcessBrowserTest
virtual void SetUpOnMainThread() OVERRIDE {
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&chrome_browser_net::SetUrlRequestMocksEnabled, true));
InProcessBrowserTest::SetUpOnMainThread();
- ASSERT_TRUE(CreateAndSetDownloadsDirectory(browser()));
- browser()->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload,
- false);
+ CreateAndSetDownloadsDirectory();
+ current_browser()->profile()->GetPrefs()->SetBoolean(
+ prefs::kPromptForDownload, false);
GetDownloadManager()->RemoveAllDownloads();
}

- DownloadManager* GetDownloadManager() {
+ virtual DownloadManager* GetDownloadManager() {
DownloadService* download_service =
- DownloadServiceFactory::GetForProfile(browser()->profile());
+ DownloadServiceFactory::GetForProfile(current_browser()->profile());
return download_service->GetDownloadManager();
}

@@ -124,16 +126,16 @@
CreateInProgressDownloadObserver(1));
GURL slow_download_url(URLRequestSlowDownloadJob::kUnknownSizeUrl);
ui_test_utils::NavigateToURLWithDisposition(
- browser(), slow_download_url, CURRENT_TAB,
+ current_browser(), slow_download_url, CURRENT_TAB,
ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
observer->WaitForFinished();
EXPECT_EQ(
1u, observer->NumDownloadsSeenInState(DownloadItem::IN_PROGRESS));
// We don't expect a select file dialog.
- CHECK(!observer->select_file_dialog_seen());
+ ASSERT_FALSE(observer->select_file_dialog_seen());
}
GetDownloadManager()->GetAllDownloads(FilePath(), items);
- CHECK_EQ(count, items->size());
+ ASSERT_EQ(count, items->size());
}

DownloadItem* CreateSlowTestDownload() {
@@ -147,7 +149,7 @@
return NULL;

ui_test_utils::NavigateToURLWithDisposition(
- browser(), slow_download_url, CURRENT_TAB,
+ current_browser(), slow_download_url, CURRENT_TAB,
ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);

observer->WaitForFinished();
@@ -176,7 +178,7 @@
CreateDownloadObserver(1));
GURL finish_url(URLRequestSlowDownloadJob::kFinishDownloadUrl);
ui_test_utils::NavigateToURLWithDisposition(
- browser(), finish_url, NEW_FOREGROUND_TAB,
+ current_browser(), finish_url, NEW_FOREGROUND_TAB,
ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
observer->WaitForFinished();
EXPECT_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
@@ -195,25 +197,35 @@
true);
}

+ extension_function_test_utils::RunFunctionFlags GetFlags() {
+ return current_browser()->profile()->IsOffTheRecord() ?
+ extension_function_test_utils::INCLUDE_INCOGNITO :
+ extension_function_test_utils::NONE;
+ }
+
+ // extension_function_test_utils::RunFunction*() only uses browser for its
+ // profile(), so pass it the on-record browser so that it always uses the
+ // on-record profile.
+
bool RunFunction(UIThreadExtensionFunction* function,
const std::string& args) {
// extension_function_test_utils::RunFunction() does not take
// ownership of |function|.
scoped_refptr<ExtensionFunction> function_owner(function);
return extension_function_test_utils::RunFunction(
- function, args, browser(), extension_function_test_utils::NONE);
+ function, args, browser(), GetFlags());
}

base::Value* RunFunctionAndReturnResult(UIThreadExtensionFunction* function,
const std::string& args) {
return extension_function_test_utils::RunFunctionAndReturnResult(
- function, args, browser(), extension_function_test_utils::NONE);
+ function, args, browser(), GetFlags());
}

std::string RunFunctionAndReturnError(UIThreadExtensionFunction* function,
const std::string& args) {
return extension_function_test_utils::RunFunctionAndReturnError(
- function, args, browser(), extension_function_test_utils::NONE);
+ function, args, browser(), GetFlags());
}

bool RunFunctionAndReturnString(UIThreadExtensionFunction* function,
@@ -256,18 +268,36 @@
}

private:
- bool CreateAndSetDownloadsDirectory(Browser* browser) {
- if (!downloads_directory_.CreateUniqueTempDir())
- return false;
- browser->profile()->GetPrefs()->SetFilePath(
+ void CreateAndSetDownloadsDirectory() {
+ ASSERT_TRUE(downloads_directory_.CreateUniqueTempDir());
+ current_browser()->profile()->GetPrefs()->SetFilePath(
prefs::kDownloadDefaultDirectory,
downloads_directory_.path());
- return true;
}

ScopedTempDir downloads_directory_;
};

+class DownloadExtensionTestIncognito : public DownloadExtensionTest {
+ public:
+ virtual Browser* current_browser() OVERRIDE { return current_browser_; }
+
+ virtual void SetUpOnMainThread() OVERRIDE {
+ GoOnTheRecord();
+ DownloadExtensionTest::SetUpOnMainThread();
+ incognito_browser_ = CreateIncognitoBrowser();
+ GoOffTheRecord();
+ GetDownloadManager()->RemoveAllDownloads();
+ }
+
+ void GoOnTheRecord() { current_browser_ = browser(); }
+ void GoOffTheRecord() { current_browser_ = incognito_browser_; }
+
+ private:
+ Browser* incognito_browser_;
+ Browser* current_browser_;
+};
+
class MockIconExtractorImpl : public DownloadFileIconExtractor {
public:
MockIconExtractorImpl(const FilePath& path, IconLoader::IconSize icon_size,
@@ -786,3 +816,138 @@
ASSERT_TRUE(item_value->GetString("filename", &item_name));
ASSERT_EQ(items[2]->GetFullPath().value(), item_name);
}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTestIncognito,
+ DownloadsApi_SearchIncognito) {
+ scoped_ptr<base::Value> result_value;
+ base::ListValue* result_list = NULL;
+ base::DictionaryValue* result_dict = NULL;
+ FilePath::StringType filename;
+ bool is_incognito = false;
+ std::string error;
+ std::string on_item_arg;
+ std::string off_item_arg;
+ std::string result_string;
+
+ // Set up one on-record item and one off-record item.
+
+ GoOnTheRecord();
+ DownloadItem* on_item = CreateSlowTestDownload();
+ ASSERT_TRUE(on_item);
+ ASSERT_FALSE(on_item->IsOtr());
+ on_item_arg = DownloadItemIdAsArgList(on_item);
+
+ GoOffTheRecord();
+ DownloadItem* off_item = CreateSlowTestDownload();
+ ASSERT_TRUE(off_item);
+ ASSERT_TRUE(off_item->IsOtr());
+ ASSERT_TRUE(on_item->GetFullPath() != off_item->GetFullPath());
+ off_item_arg = DownloadItemIdAsArgList(off_item);
+
+ // Extensions running in the incognito window should have access to both
+ // items because the Test extension is in spanning mode.
+ result_value.reset(RunFunctionAndReturnResult(
+ new DownloadsSearchFunction(), "[{}]"));
+ ASSERT_TRUE(result_value.get());
+ ASSERT_TRUE(result_value->GetAsList(&result_list));
+ ASSERT_EQ(2UL, result_list->GetSize());
+ ASSERT_TRUE(result_list->GetDictionary(0, &result_dict));
+ ASSERT_TRUE(result_dict->GetString("filename", &filename));
+ ASSERT_TRUE(result_dict->GetBoolean("incognito", &is_incognito));
+ EXPECT_TRUE(on_item->GetFullPath() == FilePath(filename));
+ EXPECT_FALSE(is_incognito);
+ ASSERT_TRUE(result_list->GetDictionary(1, &result_dict));
+ ASSERT_TRUE(result_dict->GetString("filename", &filename));
+ ASSERT_TRUE(result_dict->GetBoolean("incognito", &is_incognito));
+ EXPECT_TRUE(off_item->GetFullPath() == FilePath(filename));
+ EXPECT_TRUE(is_incognito);
+
+ // Extensions running in the on-record window should have access only to the
+ // on-record item.
+ GoOnTheRecord();
+ result_value.reset(RunFunctionAndReturnResult(
+ new DownloadsSearchFunction(), "[{}]"));
+ ASSERT_TRUE(result_value.get());
+ ASSERT_TRUE(result_value->GetAsList(&result_list));
+ ASSERT_EQ(1UL, result_list->GetSize());
+ ASSERT_TRUE(result_list->GetDictionary(0, &result_dict));
+ ASSERT_TRUE(result_dict->GetString("filename", &filename));
+ EXPECT_TRUE(on_item->GetFullPath() == FilePath(filename));
+ ASSERT_TRUE(result_dict->GetBoolean("incognito", &is_incognito));
+ EXPECT_FALSE(is_incognito);
+
+ // Pausing/Resuming the off-record item while on the record should return an
+ // error. Cancelling "non-existent" downloads is not an error.
+ error = RunFunctionAndReturnError(new DownloadsPauseFunction(), off_item_arg);
+ EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
+ error.c_str());
+ error = RunFunctionAndReturnError(new DownloadsResumeFunction(),
+ off_item_arg);
+ EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
+ error.c_str());
+ error = RunFunctionAndReturnError(
+ new DownloadsGetFileIconFunction(),
+ base::StringPrintf("[%d, {}]", off_item->GetId()));
+ EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
+ error.c_str());
+
+ // TODO(benjhayden): Test incognito_split_mode() extension.
+ // TODO(benjhayden): Test download(), onCreated, onChanged, onErased.
+
+ GoOffTheRecord();
+
+ // Do the FileIcon test for both the on- and off-items while off the record.
+ // NOTE(benjhayden): This does not include the FileIcon test from history,
+ // just active downloads. This shouldn't be a problem.
+ EXPECT_TRUE(RunFunctionAndReturnString(
+ new DownloadsGetFileIconFunction(),
+ base::StringPrintf("[%d, {}]", on_item->GetId()), &result_string));
+ EXPECT_TRUE(RunFunctionAndReturnString(
+ new DownloadsGetFileIconFunction(),
+ base::StringPrintf("[%d, {}]", off_item->GetId()), &result_string));
+
+ // Do the pause/resume/cancel test for both the on- and off-items while off
+ // the record.
+ EXPECT_TRUE(RunFunction(new DownloadsPauseFunction(), on_item_arg));
+ EXPECT_TRUE(on_item->IsPaused());
+ EXPECT_TRUE(RunFunction(new DownloadsPauseFunction(), on_item_arg));
+ EXPECT_TRUE(on_item->IsPaused());
+ EXPECT_TRUE(RunFunction(new DownloadsResumeFunction(), on_item_arg));
+ EXPECT_FALSE(on_item->IsPaused());
+ EXPECT_TRUE(RunFunction(new DownloadsResumeFunction(), on_item_arg));
+ EXPECT_FALSE(on_item->IsPaused());
+ EXPECT_TRUE(RunFunction(new DownloadsPauseFunction(), on_item_arg));
+ EXPECT_TRUE(on_item->IsPaused());
+ EXPECT_TRUE(RunFunction(new DownloadsCancelFunction(), on_item_arg));
+ EXPECT_TRUE(on_item->IsCancelled());
+ EXPECT_TRUE(RunFunction(new DownloadsCancelFunction(), on_item_arg));
+ EXPECT_TRUE(on_item->IsCancelled());
+ error = RunFunctionAndReturnError(new DownloadsPauseFunction(), on_item_arg);
+ EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
+ error.c_str());
+ error = RunFunctionAndReturnError(new DownloadsResumeFunction(), on_item_arg);
+ EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
+ error.c_str());
+ EXPECT_TRUE(RunFunction(new DownloadsPauseFunction(), off_item_arg));
+ EXPECT_TRUE(off_item->IsPaused());
+ EXPECT_TRUE(RunFunction(new DownloadsPauseFunction(), off_item_arg));
+ EXPECT_TRUE(off_item->IsPaused());
+ EXPECT_TRUE(RunFunction(new DownloadsResumeFunction(), off_item_arg));
+ EXPECT_FALSE(off_item->IsPaused());
+ EXPECT_TRUE(RunFunction(new DownloadsResumeFunction(), off_item_arg));
+ EXPECT_FALSE(off_item->IsPaused());
+ EXPECT_TRUE(RunFunction(new DownloadsPauseFunction(), off_item_arg));
+ EXPECT_TRUE(off_item->IsPaused());
+ EXPECT_TRUE(RunFunction(new DownloadsCancelFunction(), off_item_arg));
+ EXPECT_TRUE(off_item->IsCancelled());
+ EXPECT_TRUE(RunFunction(new DownloadsCancelFunction(), off_item_arg));
+ EXPECT_TRUE(off_item->IsCancelled());
+ error = RunFunctionAndReturnError(new DownloadsPauseFunction(),
+ off_item_arg);
+ EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
+ error.c_str());
+ error = RunFunctionAndReturnError(new DownloadsResumeFunction(),
+ off_item_arg);
+ EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
+ error.c_str());
+}

Modified: trunk/src/chrome/common/extensions/api/experimental_downloads.json
==============================================================================
--- trunk/src/chrome/common/extensions/api/experimental_downloads.json (original)
+++ trunk/src/chrome/common/extensions/api/experimental_downloads.json Wed May 16 12:18:13 2012
@@ -142,6 +142,11 @@
"type": "string",
"description": "absolute local path"
},
+ "incognito": {
+ "type": "boolean",
+ "description": "False if this download is recorded in the history, true if it is not recorded.",
+ "optional": true
+ },
"danger": {
"$ref": "DownloadDangerType",
"description": "Indication of whether this download is thought to be safe or known to be suspicious.",