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..32b0fdee7a4036a679800ca4dc0b2a635bd12217 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));
 
@@ -421,6 +426,8 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
 
   DownloadItem* download = GetActiveDownloadItem(0);
   ASSERT_TRUE(download != NULL);
+  scoped_ptr<DownloadItemModel> download_item_model(
+      new DownloadItemModel(download));

[Randy Smith (Not in Mondays), 2011/05/12 20:21:17]

It doesn't look like this is used.

[haraken1, 2011/05/13 14:08:17]

Done.

 
   EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state());
   scoped_ptr<ItemObserver> observer(new ItemObserver(download));
@@ -432,11 +439,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);

[Randy Smith (Not in Mondays), 2011/05/12 20:21:17]

Why put in this change?

[haraken1, 2011/05/13 14:08:17]

Sorry, this change is not related to this CL. I changed the value of error_size
from 1024 to 3 just because practically the error_size should be less than
kTestDataLen. 

   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 +451,17 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
   EXPECT_FALSE(observer->was_opened());
+  EXPECT_TRUE(download->file_exists());
+  // 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.
+  EXPECT_EQ(DownloadItem::INTERRUPTED, download->state());
+  EXPECT_EQ(download->received_bytes(), error_size);
+  EXPECT_EQ(download->total_bytes(), static_cast<int64>(kTestDataLen));

[Randy Smith (Not in Mondays), 2011/05/12 20:21:17]

I have no objection to you adding these tests, but I don't see the relationship
to your CL, and I wanted to make sure I wasn't missing anything.  Is there a
relationship between these changes and your CL?

[haraken1, 2011/05/13 14:08:17]

The purpose of this CL is

[1] to add TEST_F(DownloadManagerTest, DownloadRemoveTest)
[2] to add the test for DownloadItemModel::GetStatusText()

Here is the part where I would like to test DownloadItemModel::GetStatusText().
However, it is a bit complicated to directly check the returned string by 
GetStatusText(), since GetStatusText() is internally doing value format
conversion. So, instead, I inserted the code to check each value (which will be
used in the returned string) individually. 

 
   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 +469,10 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
   EXPECT_FALSE(observer->was_opened());
+  EXPECT_TRUE(download->file_exists());
+  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 +504,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 +528,10 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
   EXPECT_FALSE(observer->was_opened());
+  EXPECT_TRUE(download->file_exists());
+  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 +576,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 +614,9 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
   EXPECT_FALSE(observer->was_opened());
+  EXPECT_TRUE(download->file_exists());
   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 +625,95 @@ 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_TRUE(download->file_exists());
+  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

[Paweł Hajdan Jr., 2011/05/13 08:41:10]

nit: Dot at the end of the sentence.

[haraken1, 2011/05/13 14:08:17]

Done.

+  file_util::Delete(new_path, false);

[Paweł Hajdan Jr., 2011/05/13 08:41:10]

Please check return value of this.

[haraken1, 2011/05/13 14:08:17]

Done.

+  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_FALSE(download->file_exists());
+  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));
+}