Detect removed files and reflect the state in chrome://downloads and the download shelf

chrome/browser/download/download_manager_unittest.cc

Index: chrome/browser/download/download_manager_unittest.cc
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index 7b10e86fd593a21a0672aacc6b7b83ffd331c42a..cd8898dbd42afcc9b0535d0812d3fecd6ab93657 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -9,10 +9,12 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util-inl.h"
 #include "base/string_util.h"
+#include "base/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/download/download_file.h"
 #include "chrome/browser/download/download_file_manager.h"
 #include "chrome/browser/download/download_item.h"
+#include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_status_updater.h"
@@ -23,9 +25,11 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/test/testing_profile.h"
 #include "content/browser/browser_thread.h"
+#include "grit/generated_resources.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gmock_mutant.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/l10n/l10n_util.h"
 
 class DownloadManagerTest : public testing::Test {
  public:
@@ -404,6 +408,7 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   info->prompt_user_for_save_location = false;
   info->is_dangerous_file = false;
   info->is_dangerous_url = false;
+  info->total_bytes = static_cast<int64>(kTestDataLen);
   const FilePath new_path(FILE_PATH_LITERAL("foo.zip"));
   const FilePath cr_path(download_util::GetCrDownloadPath(new_path));
 
@@ -432,11 +437,11 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   message_loop_.RunAllPending();
   EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
 
-  OnDownloadError(0, 1024, -6);
+  int64 error_size = 3;
+  OnDownloadError(0, error_size, -6);
   message_loop_.RunAllPending();
 
   EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
-  EXPECT_EQ(DownloadItem::INTERRUPTED, download->state());
   EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
   EXPECT_TRUE(observer->hit_state(DownloadItem::INTERRUPTED));
   EXPECT_FALSE(observer->hit_state(DownloadItem::COMPLETE));
@@ -444,10 +449,17 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
   EXPECT_FALSE(observer->was_opened());
+  EXPECT_FALSE(download->file_externally_removed());
+  // Instead of checking EXPECT_EQ(download_item_model->GetStatusText(),
+  // l10n_util::GetStringFUTF16(IDS_DOWNLOAD_STATUS_INTERRUPTED, ..., ...),
+  // we check download->state(), download->received_bytes(),
+  // and download->total_bytes(), individually.

[Randy Smith (Not in Mondays), 2011/05/13 20:45:32]

I think these are all fine things to test, but testing them doesn't really seem
relevant to testing status text.  If you're testing the code that produces
status text from these values you need to examine the return from
GetStatusText().  Without checking that, you're not actually testing the
modification you made to GetStatusText().  Am I confused?

[haraken1, 2011/05/16 11:42:27]

It makes sense. I fixed it.

+  EXPECT_EQ(DownloadItem::INTERRUPTED, download->state());
+  EXPECT_EQ(download->received_bytes(), error_size);
+  EXPECT_EQ(download->total_bytes(), static_cast<int64>(kTestDataLen));
 
   download->Cancel(true);
 
-  EXPECT_EQ(DownloadItem::INTERRUPTED, download->state());
   EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
   EXPECT_TRUE(observer->hit_state(DownloadItem::INTERRUPTED));
   EXPECT_FALSE(observer->hit_state(DownloadItem::COMPLETE));
@@ -455,6 +467,10 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
   EXPECT_FALSE(observer->was_opened());
+  EXPECT_FALSE(download->file_externally_removed());
+  EXPECT_EQ(DownloadItem::INTERRUPTED, download->state());
+  EXPECT_EQ(download->received_bytes(), error_size);
+  EXPECT_EQ(download->total_bytes(), static_cast<int64>(kTestDataLen));
 }
 
 TEST_F(DownloadManagerTest, DownloadCancelTest) {
@@ -486,6 +502,8 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
 
   DownloadItem* download = GetActiveDownloadItem(0);
   ASSERT_TRUE(download != NULL);
+  scoped_ptr<DownloadItemModel> download_item_model(
+      new DownloadItemModel(download));
 
   EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state());
   scoped_ptr<ItemObserver> observer(new ItemObserver(download));
@@ -508,6 +526,10 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
   EXPECT_FALSE(observer->was_opened());
+  EXPECT_FALSE(download->file_externally_removed());
+  EXPECT_EQ(DownloadItem::CANCELLED, download->state());
+  EXPECT_EQ(download_item_model->GetStatusText(),
+            l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_CANCELED));
 
   EXPECT_FALSE(file_util::PathExists(new_path));
   EXPECT_FALSE(file_util::PathExists(cr_path));
@@ -552,6 +574,8 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
 
   DownloadItem* download = GetActiveDownloadItem(0);
   ASSERT_TRUE(download != NULL);
+  scoped_ptr<DownloadItemModel> download_item_model(
+      new DownloadItemModel(download));
 
   EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state());
   scoped_ptr<ItemObserver> observer(new ItemObserver(download));
@@ -588,7 +612,9 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
   EXPECT_FALSE(observer->was_opened());
+  EXPECT_FALSE(download->file_externally_removed());
   EXPECT_EQ(DownloadItem::COMPLETE, download->state());
+  EXPECT_EQ(download_item_model->GetStatusText(), ASCIIToUTF16(""));
 
   EXPECT_TRUE(file_util::PathExists(new_path));
   EXPECT_FALSE(file_util::PathExists(cr_path));
@@ -597,3 +623,96 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
   EXPECT_TRUE(file_util::ReadFileToString(new_path, &file_contents));
   EXPECT_EQ(std::string(kTestData), file_contents);
 }
+
+TEST_F(DownloadManagerTest, DownloadRemoveTest) {
+  using ::testing::_;
+  using ::testing::CreateFunctor;
+  using ::testing::Invoke;
+  using ::testing::Return;
+
+  // Create a temporary directory.
+  ScopedTempDir temp_dir_;
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+
+  // File names we're using.
+  const FilePath new_path(temp_dir_.path().AppendASCII("foo.txt"));
+  const FilePath cr_path(download_util::GetCrDownloadPath(new_path));
+  EXPECT_FALSE(file_util::PathExists(new_path));
+
+  // |info| will be destroyed in download_manager_.
+  DownloadCreateInfo* info(new DownloadCreateInfo);
+  info->download_id = static_cast<int>(0);
+  info->prompt_user_for_save_location = true;
+  info->is_dangerous_file = false;
+  info->is_dangerous_url = false;
+
+  download_manager_->CreateDownloadItem(info);
+
+  DownloadItem* download = GetActiveDownloadItem(0);
+  ASSERT_TRUE(download != NULL);
+  scoped_ptr<DownloadItemModel> download_item_model(
+      new DownloadItemModel(download));
+
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state());
+  scoped_ptr<ItemObserver> observer(new ItemObserver(download));
+
+  // Create and initialize the download file.  We're bypassing the first part
+  // of the download process and skipping to the part after the final file
+  // name has been chosen, so we need to initialize the download file
+  // properly.
+  DownloadFile* download_file(
+      new DownloadFile(info, download_manager_));
+  download_file->Rename(cr_path);
+  // This creates the .crdownload version of the file.
+  download_file->Initialize(false);
+  // |download_file| is owned by DownloadFileManager.
+  AddDownloadToFileManager(info->download_id, download_file);
+
+  info->path = new_path;
+  AttachDownloadItem(info);
+  message_loop_.RunAllPending();
+  EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
+
+  download_file->AppendDataToFile(kTestData, kTestDataLen);
+
+  // Finish the download.
+  OnAllDataSaved(0, kTestDataLen, "");
+  message_loop_.RunAllPending();
+
+  // Download is complete.
+  EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
+  EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
+  EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
+  EXPECT_FALSE(observer->hit_state(DownloadItem::INTERRUPTED));
+  EXPECT_TRUE(observer->hit_state(DownloadItem::COMPLETE));
+  EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
+  EXPECT_TRUE(observer->was_updated());
+  EXPECT_FALSE(observer->was_opened());
+  EXPECT_FALSE(download->file_externally_removed());
+  EXPECT_EQ(DownloadItem::COMPLETE, download->state());
+  EXPECT_EQ(download_item_model->GetStatusText(), ASCIIToUTF16(""));
+
+  EXPECT_TRUE(file_util::PathExists(new_path));
+  EXPECT_FALSE(file_util::PathExists(cr_path));
+
+  // Remove the downloaded file.
+  bool removed = file_util::Delete(new_path, false);
+  ASSERT_TRUE(removed);
+  download->OnDownloadedFileRemoved();
+  message_loop_.RunAllPending();
+
+  EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
+  EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
+  EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
+  EXPECT_FALSE(observer->hit_state(DownloadItem::INTERRUPTED));
+  EXPECT_TRUE(observer->hit_state(DownloadItem::COMPLETE));
+  EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
+  EXPECT_TRUE(observer->was_updated());
+  EXPECT_FALSE(observer->was_opened());
+  EXPECT_TRUE(download->file_externally_removed());
+  EXPECT_EQ(DownloadItem::COMPLETE, download->state());
+  EXPECT_EQ(download_item_model->GetStatusText(),
+            l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_REMOVED));
+
+  EXPECT_FALSE(file_util::PathExists(new_path));
+}