[Downloads] Switch callers of GetFullPath() to GetTargetFilePath()

While a download is in progress, the path returned by GetFullPath() is
the path to the intermediate file. This file can be renamed or disappear
completely on the FILE thread. The path may also be reset to empty when
the download is interrupted.

Callers who wish to access the target path of a download should use
DownloadItem::GetTargetFilePath() instead.

BUG=7648

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@199115 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 56 insertions, 25 deletions

diff --git a/chrome/browser/chromeos/drive/download_handler.cc b/chrome/browser/chromeos/drive/download_handler.cc
index a8f879b..f6a0756 100644
--- a/chrome/browser/chromeos/drive/download_handler.cc
+++ b/chrome/browser/chromeos/drive/download_handler.cc
@@ -88,7 +88,7 @@ bool IsPersistedDriveDownload(const base::FilePath& drive_tmp_download_path,
                               DownloadItem* download) {
   // Persisted downloads are not in IN_PROGRESS state when created, while newly
   // created downloads are.
-  return drive_tmp_download_path.IsParent(download->GetFullPath()) &&
+  return drive_tmp_download_path.IsParent(download->GetTargetFilePath()) &&
       download->GetState() != DownloadItem::IN_PROGRESS;
 }
 
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index 90e9755b..5482a50 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -101,6 +101,22 @@ base::FilePath GetPlatformDownloadPath(Profile* profile,
       drive_download_handler->IsDriveDownload(download))
     return drive_download_handler->GetTargetPath(download);
 #endif
+  // The caller wants to open the download or show it in a file browser. The
+  // download could be in one of three states:
+  // - Complete: The path we want is GetTargetFilePath().
+  // - Not complete, but there's an intermediate file: GetFullPath() will be
+  //     non-empty and is the location of the intermediate file. Since no target
+  //     file exits yet, use GetFullPath(). This should only happen during
+  //     ShowDownloadInShell().
+  // - Not Complete, and there's no intermediate file: GetFullPath() will be
+  //     empty. This shouldn't happen since CanShowInFolder() returns false and
+  //     this function shouldn't have been called.
+  if (download->IsComplete()) {
+    DCHECK(!download->GetTargetFilePath().empty());
+    return download->GetTargetFilePath();
+  }
+
+  DCHECK(!download->GetFullPath().empty());
   return download->GetFullPath();
 }
 
@@ -353,11 +369,16 @@ void ChromeDownloadManagerDelegate::ChooseSavePath(
 }
 
 void ChromeDownloadManagerDelegate::OpenDownload(DownloadItem* download) {
+  DCHECK(download->IsComplete());
+  if (!download->CanOpenDownload())
+    return;
   platform_util::OpenItem(GetPlatformDownloadPath(profile_, download));
 }
 
 void ChromeDownloadManagerDelegate::ShowDownloadInShell(
     DownloadItem* download) {
+  if (!download->CanShowInFolder())
+    return;
   platform_util::ShowItemInFolder(GetPlatformDownloadPath(profile_, download));
 }
 
@@ -373,10 +394,10 @@ void ChromeDownloadManagerDelegate::CheckForFileExistence(
     return;
   }
 #endif
-  BrowserThread::PostTaskAndReplyWithResult(BrowserThread::FILE, FROM_HERE,
-                                            base::Bind(&file_util::PathExists,
-                                                       download->GetFullPath()),
-                                            callback);
+  BrowserThread::PostTaskAndReplyWithResult(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&file_util::PathExists, download->GetTargetFilePath()),
+      callback);
 }
 
 void ChromeDownloadManagerDelegate::ClearLastDownloadPath() {
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index 9489bb5..1e4a712 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -1381,9 +1381,9 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_IncognitoRegular) {
   GetDownloads(browser(), &download_items);
   ASSERT_EQ(1UL, download_items.size());
   ASSERT_EQ(base::FilePath(FILE_PATH_LITERAL("a_zip_file.zip")),
-            download_items[0]->GetFullPath().BaseName());
-  ASSERT_TRUE(file_util::PathExists(download_items[0]->GetFullPath()));
-  EXPECT_TRUE(VerifyFile(download_items[0]->GetFullPath(),
+            download_items[0]->GetTargetFilePath().BaseName());
+  ASSERT_TRUE(file_util::PathExists(download_items[0]->GetTargetFilePath()));
+  EXPECT_TRUE(VerifyFile(download_items[0]->GetTargetFilePath(),
                          original_contents, origin_file_size));
 
   // Setup an incognito window.
@@ -1410,9 +1410,9 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_IncognitoRegular) {
   GetDownloads(incognito, &download_items);
   ASSERT_EQ(1UL, download_items.size());
   ASSERT_EQ(base::FilePath(FILE_PATH_LITERAL("a_zip_file (1).zip")),
-            download_items[0]->GetFullPath().BaseName());
-  ASSERT_TRUE(file_util::PathExists(download_items[0]->GetFullPath()));
-  EXPECT_TRUE(VerifyFile(download_items[0]->GetFullPath(),
+            download_items[0]->GetTargetFilePath().BaseName());
+  ASSERT_TRUE(file_util::PathExists(download_items[0]->GetTargetFilePath()));
+  EXPECT_TRUE(VerifyFile(download_items[0]->GetTargetFilePath(),
                          original_contents, origin_file_size));
 }
 
@@ -1832,6 +1832,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, MAYBE_DownloadTest_History) {
   ASSERT_EQ(1UL, downloads.size());
   DownloadItem* item = downloads[0];
   ASSERT_EQ(file.value(), item->GetFullPath().BaseName().value());
+  ASSERT_EQ(file.value(), item->GetTargetFilePath().BaseName().value());
   ASSERT_EQ(download_url, item->GetURL());
 }
 
@@ -2631,7 +2632,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, LoadURLExternallyReferrerPolicy) {
             download_items[0]->GetOriginalUrl());
 
   // Check that the file contains the expected referrer.
-  base::FilePath file(download_items[0]->GetFullPath());
+  base::FilePath file(download_items[0]->GetTargetFilePath());
   std::string expected_contents = test_server()->GetURL(std::string()).spec();
   ASSERT_TRUE(VerifyFile(file, expected_contents, expected_contents.length()));
 }
@@ -2724,13 +2725,14 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_Renaming) {
     content::DownloadItem* item = manager->GetDownload(index);
     ASSERT_TRUE(item);
     ASSERT_TRUE(item->IsComplete());
-    base::FilePath full_path(item->GetFullPath());
+    base::FilePath target_path(item->GetTargetFilePath());
     EXPECT_EQ(std::string("a_zip_file") +
         (index == 0 ? std::string(".zip") :
                       base::StringPrintf(" (%d).zip", index)),
-              full_path.BaseName().AsUTF8Unsafe());
-    ASSERT_TRUE(file_util::PathExists(full_path));
-    ASSERT_TRUE(VerifyFile(full_path, origin_contents, origin_contents.size()));
+              target_path.BaseName().AsUTF8Unsafe());
+    ASSERT_TRUE(file_util::PathExists(target_path));
+    ASSERT_TRUE(VerifyFile(target_path, origin_contents,
+                           origin_contents.size()));
   }
 }
 
@@ -2796,7 +2798,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_CrazyFilenames) {
     EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
     GetDownloads(browser(), &download_items);
     ASSERT_EQ(1UL, download_items.size());
-    base::FilePath downloaded(download_items[0]->GetFullPath());
+    base::FilePath downloaded(download_items[0]->GetTargetFilePath());
     download_items[0]->Remove();
     download_items.clear();
     ASSERT_TRUE(CheckDownloadFullPaths(
@@ -2821,7 +2823,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_Remove) {
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
   GetDownloads(browser(), &download_items);
   ASSERT_EQ(1UL, download_items.size());
-  base::FilePath downloaded(download_items[0]->GetFullPath());
+  base::FilePath downloaded(download_items[0]->GetTargetFilePath());
 
   // Remove the DownloadItem but not the file, then check that the file still
   // exists.
@@ -2912,17 +2914,18 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, MAYBE_DownloadTest_PercentComplete) {
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
 
   // Check that the file downloaded correctly.
-  ASSERT_TRUE(file_util::PathExists(download_items[0]->GetFullPath()));
+  ASSERT_TRUE(file_util::PathExists(download_items[0]->GetTargetFilePath()));
   int64 downloaded_size = 0;
   ASSERT_TRUE(file_util::GetFileSize(
-      download_items[0]->GetFullPath(), &downloaded_size));
+      download_items[0]->GetTargetFilePath(), &downloaded_size));
 #if defined(OS_WIN)
   ASSERT_EQ(1, downloaded_size);
 #else
   ASSERT_EQ(size + 1, downloaded_size);
 #endif
   ASSERT_TRUE(file_util::DieFileDie(file_path, false));
-  ASSERT_TRUE(file_util::DieFileDie(download_items[0]->GetFullPath(), false));
+  ASSERT_TRUE(file_util::DieFileDie(download_items[0]->GetTargetFilePath(),
+                                    false));
 }
 
 IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_DenyDanger) {
diff --git a/content/browser/android/download_controller_android_impl.cc b/content/browser/android/download_controller_android_impl.cc
index cc8d640..dbea7b0 100644
--- a/content/browser/android/download_controller_android_impl.cc
+++ b/content/browser/android/download_controller_android_impl.cc
@@ -268,7 +268,7 @@ void DownloadControllerAndroidImpl::OnDownloadUpdated(DownloadItem* item) {
   ScopedJavaLocalRef<jstring> jmime_type =
       ConvertUTF8ToJavaString(env, item->GetMimeType());
   ScopedJavaLocalRef<jstring> jpath =
-      ConvertUTF8ToJavaString(env, item->GetFullPath().value());
+      ConvertUTF8ToJavaString(env, item->GetTargetFilePath().value());
 
   ScopedJavaLocalRef<jobject> view_core = GetContentViewCoreFromWebContents(
       item->GetWebContents());
diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc
index cb4cf8c..4be05ed 100644
--- a/content/browser/download/download_browsertest.cc
+++ b/content/browser/download/download_browsertest.cc
@@ -760,13 +760,13 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, MultiDownload) {
   // Verify that the files have the expected data and size.
   // |file1| should be full of '*'s, and |file2| should be the same as the
   // source file.
-  base::FilePath file1(download1->GetFullPath());
+  base::FilePath file1(download1->GetTargetFilePath());
   size_t file_size1 = URLRequestSlowDownloadJob::kFirstDownloadSize +
                       URLRequestSlowDownloadJob::kSecondDownloadSize;
   std::string expected_contents(file_size1, '*');
   ASSERT_TRUE(VerifyFile(file1, expected_contents, file_size1));
 
-  base::FilePath file2(download2->GetFullPath());
+  base::FilePath file2(download2->GetTargetFilePath());
   ASSERT_TRUE(file_util::ContentsEqual(
       file2, GetTestFilePath("download", "download-test.lib")));
 }
diff --git a/content/public/browser/download_item.h b/content/public/browser/download_item.h
index 67d1951..e3e4d22 100644
--- a/content/public/browser/download_item.h
+++ b/content/public/browser/download_item.h
@@ -201,7 +201,14 @@ class CONTENT_EXPORT DownloadItem : public base::SupportsUserData {
   // Full path to the downloaded or downloading file. This is the path to the
   // physical file, if one exists. It should be considered a hint; changes to
   // this value and renames of the file on disk are not atomic with each other.
-  // May be empty if the in-progress path hasn't been determined yet.
+  // May be empty if the in-progress path hasn't been determined yet or if the
+  // download was interrupted.
+  //
+  // DO NOT USE THIS METHOD to access the target path of the DownloadItem. Use
+  // GetTargetFilePath() instead. While the download is in progress, the
+  // intermediate file named by GetFullPath() may be renamed or disappear
+  // completely on the FILE thread. The path may also be reset to empty when the
+  // download is interrupted.
   virtual const base::FilePath& GetFullPath() const = 0;
 
   // Target path of an in-progress download. We may be downloading to a