Implemented ExternalData interface on DownloadItem and used it for SafeBrowsing.

Review URL: http://codereview.chromium.org/8571023

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

chrome/browser/download/chrome_download_manager_delegate.cc

@@ -43,6 +43,22 @@
 using content::BrowserThread;
 using safe_browsing::DownloadProtectionService;
 
+namespace {
+
+// String pointer used for identifying safebrowing data associated with
+// a download item.
+static const char safe_browsing_id[] = "Safe Browsing ID";
+
+// The state of a safebrowsing check.
+struct SafeBrowsingState : public DownloadItem::ExternalData {
+  // If true the SafeBrowsing check is not done yet.
+  bool pending;
+  // The verdict that we got from calling CheckClientDownload.
+  safe_browsing::DownloadProtectionService::DownloadCheckResult verdict;
+};
+
+}
+
 ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
     : profile_(profile),
       download_prefs_(new DownloadPrefs(profile->GetPrefs())) {
@@ -153,29 +169,27 @@ bool ChromeDownloadManagerDelegate::ShouldOpenFileBasedOnExtension(
 bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(DownloadItem* item) {
 #if defined(ENABLE_SAFE_BROWSING)
   // See if there is already a pending SafeBrowsing check for that download.
-  SafeBrowsingStateMap::iterator it = safe_browsing_state_.find(item->GetId());
-  if (it != safe_browsing_state_.end()) {
-    SafeBrowsingState state = it->second;
-    if (!state.pending) {
-      safe_browsing_state_.erase(it);
-    }
-    return !state.pending;
-  }
+  SafeBrowsingState* state = static_cast<SafeBrowsingState*>(
+      item->GetExternalData(&safe_browsing_id));
+  if (state)
+    // Don't complete the download until we have an answer.
+    return !state->pending;
+
   // Begin the safe browsing download protection check.
   DownloadProtectionService* service = GetDownloadProtectionService();
   if (service) {
     VLOG(2) << __FUNCTION__ << "() Start SB download check for download = "
             << item->DebugString(false);
+    state = new SafeBrowsingState();
+    state->pending = true;
+    state->verdict = DownloadProtectionService::SAFE;
+    item->SetExternalData(&safe_browsing_id, state);
     service->CheckClientDownload(
         DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
         base::Bind(
             &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
             this,
             item->GetId()));
-    SafeBrowsingState state;
-    state.pending = true;
-    state.verdict = DownloadProtectionService::SAFE;
-    safe_browsing_state_[item->GetId()] = state;
     return false;
   }
 #endif
@@ -331,10 +345,8 @@ void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
     int32 download_id,
     DownloadProtectionService::DownloadCheckResult result) {
   DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id);
-  if (!item) {
-    safe_browsing_state_.erase(download_id);  // Just in case.
+  if (!item)
     return;
-  }
 
   VLOG(2) << __FUNCTION__ << "() download = " << item->DebugString(false)
           << " verdict = " << result;
@@ -344,11 +356,12 @@ void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
       item->GetSafetyState() == DownloadItem::SAFE)
     item->MarkContentDangerous();
 
-  SafeBrowsingStateMap::iterator it = safe_browsing_state_.find(item->GetId());
-  DCHECK(it != safe_browsing_state_.end() && it->second.pending);
-  if (it != safe_browsing_state_.end()) {
-    it->second.pending = false;
-    it->second.verdict = result;
+  SafeBrowsingState* state = static_cast<SafeBrowsingState*>(
+      item->GetExternalData(&safe_browsing_id));
+  DCHECK(state);
+  if (state) {
+    state->pending = false;
+    state->verdict = result;
   }
   item->MaybeCompleteDownload();
 }

chrome/browser/download/chrome_download_manager_delegate.h

@@ -149,16 +149,6 @@ class ChromeDownloadManagerDelegate
   typedef base::hash_map<CrxInstaller*, int> CrxInstallerMap;
   CrxInstallerMap crx_installers_;
 
-  // Maps the SafeBrowsing download check state to a DownloadItem ID.
-  struct SafeBrowsingState {
-    // If true the SafeBrowsing check is not done yet.
-    bool pending;
-    // The verdict that we got from calling CheckClientDownload.
-    safe_browsing::DownloadProtectionService::DownloadCheckResult verdict;
-  };
-  typedef base::hash_map<int, SafeBrowsingState> SafeBrowsingStateMap;
-  SafeBrowsingStateMap safe_browsing_state_;
-
   content::NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeDownloadManagerDelegate);

chrome/browser/download/download_item_unittest.cc

@@ -91,7 +91,8 @@ class DownloadItemTest : public testing::Test {
   }
 
   // This class keeps ownership of the created download item; it will
-  // be torn down at the end of the test.
+  // be torn down at the end of the test unless DestroyDownloadItem is
+  // called.
   DownloadItem* CreateDownloadItem(DownloadItem::DownloadState state) {
     // Normally, the download system takes ownership of info, and is
     // responsible for deleting it.  In these unit tests, however, we
@@ -109,10 +110,16 @@ class DownloadItemTest : public testing::Test {
     DownloadItem* download =
         new DownloadItemImpl(&delegate_, *(info_.get()),
                              request_handle, false);
-    allocated_downloads_.push_back(download);
+    allocated_downloads_.insert(download);
     return download;
   }
 
+  // Destroy a previously created download item.
+  void DestroyDownloadItem(DownloadItem* item) {
+    allocated_downloads_.erase(item);
+    delete item;
+  }
+
  protected:
   DownloadStatusUpdater download_status_updater_;
 
@@ -122,7 +129,7 @@ class DownloadItemTest : public testing::Test {
   // UI thread.
   content::TestBrowserThread ui_thread_;
   testing::NiceMock<MockDelegate> delegate_;
-  std::vector<DownloadItem*> allocated_downloads_;
+  std::set<DownloadItem*> allocated_downloads_;
 };
 
 namespace {
@@ -274,6 +281,64 @@ TEST_F(DownloadItemTest, NotificationAfterTogglePause) {
   ASSERT_TRUE(observer.CheckUpdated());
 }
 
+static char external_data_test_string[] = "External data test";
+static int destructor_called = 0;
+
+class TestExternalData : public DownloadItem::ExternalData {
+ public:
+  int value;
+  virtual ~TestExternalData() {
+    destructor_called++;
+  }
+};
+
+TEST_F(DownloadItemTest, ExternalData) {
+  DownloadItem* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
+
+  // Shouldn't be anything there before set.
+  EXPECT_EQ(NULL, item->GetExternalData(&external_data_test_string));
+
+  TestExternalData* test1(new TestExternalData());
+  test1->value = 2;
+
+  // Should be able to get back what you set.
+  item->SetExternalData(&external_data_test_string, test1);
+  TestExternalData* test_result =
+      static_cast<TestExternalData*>(
+          item->GetExternalData(&external_data_test_string));
+  EXPECT_EQ(test1, test_result);
+
+  // Destructor should be called if value overwritten.  New value
+  // should then be retrievable.
+  TestExternalData* test2(new TestExternalData());
+  test2->value = 3;
+  EXPECT_EQ(0, destructor_called);
+  item->SetExternalData(&external_data_test_string, test2);
+  EXPECT_EQ(1, destructor_called);
+  EXPECT_EQ(static_cast<DownloadItem::ExternalData*>(test2),
+            item->GetExternalData(&external_data_test_string));
+
+  // Overwriting with the same value shouldn't do anything.
+  EXPECT_EQ(1, destructor_called);
+  item->SetExternalData(&external_data_test_string, test2);
+  EXPECT_EQ(1, destructor_called);
+  EXPECT_EQ(static_cast<DownloadItem::ExternalData*>(test2),
+            item->GetExternalData(&external_data_test_string));
+
+  // Overwriting with NULL should result in destruction.
+  item->SetExternalData(&external_data_test_string, NULL);
+  EXPECT_EQ(2, destructor_called);
+
+  // Destroying the download item should destroy the external data.
+
+  TestExternalData* test3(new TestExternalData());
+  item->SetExternalData(&external_data_test_string, test3);
+  EXPECT_EQ(static_cast<DownloadItem::ExternalData*>(test3),
+            item->GetExternalData(&external_data_test_string));
+  DestroyDownloadItem(item);
+  EXPECT_EQ(3, destructor_called);
+}
+
 TEST(MockDownloadItem, Compiles) {
   MockDownloadItem mock_item;
 }

content/browser/download/download_item.h

@@ -18,6 +18,7 @@
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_
 #pragma once
 
+#include <map>
 #include <string>
 
 #include "base/string16.h"
@@ -100,6 +101,13 @@ class CONTENT_EXPORT DownloadItem {
     virtual ~Observer() {}
   };
 
+  // Interface for data that can be stored associated with (and owned
+  // by) an object of this class via GetExternalData/SetExternalData.
+  class ExternalData {
+   public:
+    virtual ~ExternalData() {};
+  };
+
   virtual ~DownloadItem();
 
   virtual void AddObserver(DownloadItem::Observer* observer) = 0;
@@ -308,6 +316,21 @@ class CONTENT_EXPORT DownloadItem {
   // rewrites of the DownloadManager queues.
   virtual void OffThreadCancel(DownloadFileManager* file_manager) = 0;
 
+  // Manage data owned by other subsystems associated with the
+  // DownloadItem.  By custom, key is the address of a
+  // static char subsystem_specific_string[] = ".."; defined
+  // in the subsystem, but the only requirement of this interface
+  // is that the key be unique over all data stored with this
+  // DownloadItem.
+  //
+  // Note that SetExternalData takes ownership of the
+  // passed object; it will be destroyed when the DownloadItem is.
+  // If an object is already held by the DownloadItem associated with
+  // the passed key, it will be destroyed if overwriten by a new pointer
+  // (overwrites by the same pointer are ignored).
+  virtual ExternalData* GetExternalData(const void* key) = 0;
+  virtual void SetExternalData(const void* key, ExternalData* data) = 0;
+
   virtual std::string DebugString(bool verbose) const = 0;
 
   virtual void MockDownloadOpenForTesting() = 0;

content/browser/download/download_item_impl.cc

@@ -14,6 +14,7 @@
 #include "base/i18n/string_search.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/stl_util.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "content/browser/download/download_create_info.h"
@@ -260,6 +261,8 @@ DownloadItemImpl::~DownloadItemImpl() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   TransitionTo(REMOVING);
+  STLDeleteContainerPairSecondPointers(
+      external_data_map_.begin(), external_data_map_.end());
   delegate_->AssertStateConsistent(this);
   delegate_->Detach();
 }
@@ -928,3 +931,23 @@ bool DownloadItemImpl::NeedsRename() const {
   return state_info_.target_name != full_path_.BaseName();
 }
 void DownloadItemImpl::MockDownloadOpenForTesting() { open_enabled_ = false; }
+
+DownloadItem::ExternalData*
+DownloadItemImpl::GetExternalData(const void* key) {
+  if (!ContainsKey(external_data_map_, key))
+    return NULL;
+  return external_data_map_[key];
+}
+
+void DownloadItemImpl::SetExternalData(
+    const void* key, DownloadItem::ExternalData* data) {
+  std::map<const void*, ExternalData*>::iterator it =
+      external_data_map_.find(key);
+
+  if (it == external_data_map_.end()) {
+    external_data_map_[key] = data;
+  } else if (it->second != data) {
+    delete it->second;
+    it->second = data;
+  }
+}

content/browser/download/download_item_impl.h

@@ -202,6 +202,8 @@ class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
   virtual void OffThreadCancel(DownloadFileManager* file_manager) OVERRIDE;
   virtual std::string DebugString(bool verbose) const OVERRIDE;
   virtual void MockDownloadOpenForTesting() OVERRIDE;
+  virtual ExternalData* GetExternalData(const void* key) OVERRIDE;
+  virtual void SetExternalData(const void* key, ExternalData* data) OVERRIDE;
 
  private:
   // Construction common to all constructors. |active| should be true for new
@@ -352,6 +354,10 @@ class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
   // Did the delegate delay calling Complete on this download?
   bool delegate_delayed_complete_;
 
+  // External Data storage.  All objects in the store
+  // are owned by the DownloadItemImpl.
+  std::map<const void*, ExternalData*> external_data_map_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadItemImpl);
 };
 

content/browser/download/mock_download_item.h

@@ -107,5 +107,7 @@ class MockDownloadItem : public DownloadItem {
   MOCK_METHOD1(OffThreadCancel, void(DownloadFileManager* file_manager));
   MOCK_CONST_METHOD1(DebugString, std::string(bool));
   MOCK_METHOD0(MockDownloadOpenForTesting, void());
+  MOCK_METHOD1(GetExternalData, ExternalData*(const void*));
+  MOCK_METHOD2(SetExternalData, void(const void*, ExternalData*));
 };
 #endif  // CONTENT_BROWSER_DOWNLOAD_MOCK_DOWNLOAD_ITEM_H_