DownloadManager::Observer::OnDownloadCreated

Currently, it is fired when the DownloadItem is created, not when it has its filename, not when it's persisted. Listeners should observe the item for those kinds of state changes.
Those Observers that care about the filename (any of the filenames) should check whether it's empty() before using it. The DownloadItem[Impl] will UpdateObservers when the filename changes.
OnDownloadCreated is not fired for SavePackage downloads. It will be fired when we are comfortable with the user interacting with them, which might not happen until after the SavePackage integration.
ExtensionDownloadsEventRouter filters out temporary downloads, which are downloads whose DownloadCreateInfo.save_info.file_path is not empty(). GData also calls SetIsTemporary after OnDownloadCreated is fired, but only for SavePackage downloads, so EDER is safe there.

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

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

8 files changed, 117 insertions, 121 deletions

diff --git a/chrome/browser/extensions/api/downloads/downloads_api.cc b/chrome/browser/extensions/api/downloads/downloads_api.cc
index 20b7e8d..bbfe75d 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api.cc
@@ -330,10 +330,10 @@ DownloadItem* GetActiveItem(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);
+  DownloadItem* download_item = manager->GetDownload(id);
   if (!download_item && incognito_manager)
-    download_item = incognito_manager->GetActiveDownloadItem(id);
-  return download_item;
+    download_item = incognito_manager->GetDownload(id);
+  return download_item && download_item->IsInProgress() ? download_item : NULL;
 }
 
 enum DownloadsFunctionName {
@@ -454,9 +454,9 @@ void RunDownloadQuery(
   GetManagers(profile, include_incognito, &manager, &incognito_manager);
   DownloadQuery::DownloadVector all_items;
   if (query_in.id.get()) {
-    DownloadItem* item = manager->GetDownloadItem(*query_in.id.get());
+    DownloadItem* item = manager->GetDownload(*query_in.id.get());
     if (!item && incognito_manager)
-      item = incognito_manager->GetDownloadItem(*query_in.id.get());
+      item = incognito_manager->GetDownload(*query_in.id.get());
     if (item)
       all_items.push_back(item);
   } else {
@@ -595,7 +595,7 @@ bool DownloadsPauseFunction::RunImpl() {
   EXTENSION_FUNCTION_VALIDATE(params.get());
   DownloadItem* download_item = GetActiveItem(
       profile(), include_incognito(), params->download_id);
-  if ((download_item == NULL) || !download_item->IsInProgress()) {
+  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;
@@ -752,9 +752,9 @@ bool DownloadsGetFileIconFunction::RunImpl() {
   DownloadManager* manager = NULL;
   DownloadManager* incognito_manager = NULL;
   GetManagers(profile(), include_incognito(), &manager, &incognito_manager);
-  DownloadItem* download_item = manager->GetDownloadItem(params->download_id);
+  DownloadItem* download_item = manager->GetDownload(params->download_id);
   if (!download_item && incognito_manager)
-    download_item = incognito_manager->GetDownloadItem(params->download_id);
+    download_item = incognito_manager->GetDownload(params->download_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
@@ -890,58 +890,22 @@ void ExtensionDownloadsEventRouter::OnDownloadOpened(DownloadItem* item) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
-void ExtensionDownloadsEventRouter::ModelChanged(DownloadManager* manager) {
+void ExtensionDownloadsEventRouter::OnDownloadCreated(
+    DownloadManager* manager, DownloadItem* download_item) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(manager_ == manager);
-  typedef std::set<int> DownloadIdSet;
-
-  // Get all the download items.
-  DownloadManager::DownloadVector current_vec;
-  manager_->SearchDownloads(string16(), &current_vec);
-
-  // Populate set<>s of download item identifiers so that we can find
-  // differences between the old and the new set of download items.
-  DownloadIdSet current_set, prev_set;
-  for (ItemMap::const_iterator iter = downloads_.begin();
-       iter != downloads_.end(); ++iter) {
-    prev_set.insert(iter->first);
-  }
-  ItemMap current_map;
-  for (DownloadManager::DownloadVector::const_iterator iter =
-       current_vec.begin();
-       iter != current_vec.end(); ++iter) {
-    DownloadItem* item = *iter;
-    int item_id = item->GetId();
-    CHECK(item_id >= 0);
-    DCHECK(current_map.find(item_id) == current_map.end());
-    current_set.insert(item_id);
-    current_map[item_id] = item;
-  }
-  DownloadIdSet new_set;  // current_set - prev_set;
-  std::set_difference(current_set.begin(), current_set.end(),
-                      prev_set.begin(), prev_set.end(),
-                      std::insert_iterator<DownloadIdSet>(
-                        new_set, new_set.begin()));
-
-  // For each download that was not in the old set of downloads but is in the
-  // new set of downloads, fire an onCreated event, register as an Observer of
-  // the item, store a json representation of the item so that we can easily
-  // find changes in that json representation, and make an OnChangedStat.
-  for (DownloadIdSet::const_iterator iter = new_set.begin();
-       iter != new_set.end(); ++iter) {
-    scoped_ptr<base::DictionaryValue> item(
-        DownloadItemToJSON(current_map[*iter]));
-    DispatchEvent(extensions::event_names::kOnDownloadCreated,
-                  item->DeepCopy());
-    DCHECK(item_jsons_.find(*iter) == item_jsons_.end());
-    on_changed_stats_[*iter] = new OnChangedStat();
-    current_map[*iter]->AddObserver(this);
-    item_jsons_[*iter] = item.release();
-  }
-  downloads_.swap(current_map);
-
-  // Dispatching onErased is handled in OnDownloadUpdated when an item
-  // transitions to the REMOVING state.
+  if (download_item->IsTemporary()) return;
+
+  download_item->AddObserver(this);
+  scoped_ptr<base::DictionaryValue> json_item(
+      DownloadItemToJSON(download_item));
+  DispatchEvent(extensions::event_names::kOnDownloadCreated,
+                json_item->DeepCopy());
+  int32 download_id = download_item->GetId();
+  DCHECK(item_jsons_.find(download_id) == item_jsons_.end());
+  on_changed_stats_[download_id] = new OnChangedStat();
+  item_jsons_[download_id] = json_item.release();
+  downloads_[download_id] = download_item;
 }
 
 void ExtensionDownloadsEventRouter::ManagerGoingDown(
diff --git a/chrome/browser/extensions/api/downloads/downloads_api.h b/chrome/browser/extensions/api/downloads/downloads_api.h
index c4c35d0..9461dd4 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.h
+++ b/chrome/browser/extensions/api/downloads/downloads_api.h
@@ -219,8 +219,12 @@ class ExtensionDownloadsEventRouter : public content::DownloadManager::Observer,
       Profile* profile, content::DownloadManager* manager);
   virtual ~ExtensionDownloadsEventRouter();
 
-  virtual void ModelChanged(content::DownloadManager* manager) OVERRIDE;
+  // content::DownloadManager::Observer
+  virtual void OnDownloadCreated(content::DownloadManager* manager,
+                                 content::DownloadItem* download_item) OVERRIDE;
   virtual void ManagerGoingDown(content::DownloadManager* manager) OVERRIDE;
+
+  // content::DownloadItem::Observer
   virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
   virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE;
 
diff --git a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
index 0765f85..b0e5373 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
@@ -298,34 +298,15 @@ class DownloadExtensionTest : public ExtensionApiTest {
                            const std::string& on_created_event) {
     if (!WaitFor(events::kOnDownloadCreated, on_created_event))
       return false;
-    // The item may or may not be interrupted before the onCreated event fires.
-    if (item->IsInterrupted()) {
-      scoped_ptr<base::Value> args(base::JSONReader::Read(on_created_event));
-      base::ListValue* args_list = NULL;
-      base::DictionaryValue* args_dict = NULL;
-      if (!args->GetAsList(&args_list) ||
-          !args_list->GetDictionary(0, &args_dict))
-        return false;
-      args_dict->SetString("state", "interrupted");
-      args_dict->SetInteger("error", expected_error);
-      std::string created_error;
-      base::JSONWriter::Write(args_list, &created_error);
-      // This is not waiting for a different event, it's refining the
-      // expectations on the onCreated event that was just caught. Specifically,
-      // if a DownloadItem is already interrupted by the time the onCreated
-      // event fires, then the onCreated event should already describe the
-      // error.
-      return WaitFor(events::kOnDownloadCreated, created_error);
-    } else {
-      return WaitFor(events::kOnDownloadChanged,
-          base::StringPrintf("[{\"id\": %d,"
-                              "  \"error\": {\"current\": %d},"
-                              "  \"state\": {"
-                              "    \"previous\": \"in_progress\","
-                              "    \"current\": \"interrupted\"}}]",
-                              item->GetId(),
-                              expected_error));
-    }
+    // Now, onCreated is always fired before interruption.
+    return WaitFor(events::kOnDownloadChanged,
+        base::StringPrintf("[{\"id\": %d,"
+                            "  \"error\": {\"current\": %d},"
+                            "  \"state\": {"
+                            "    \"previous\": \"in_progress\","
+                            "    \"current\": \"interrupted\"}}]",
+                            item->GetId(),
+                            expected_error));
   }
 
   std::string GetExtensionURL() {
@@ -1504,12 +1485,10 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
 
   ASSERT_TRUE(WaitFor(events::kOnDownloadCreated,
       base::StringPrintf("[{\"danger\": \"safe\","
-                          "  \"filename\": \"%s\","
                           "  \"incognito\": false,"
                           "  \"mime\": \"text/plain\","
                           "  \"paused\": false,"
                           "  \"url\": \"%s\"}]",
-                          GetFilename("slow.txt.crdownload").c_str(),
                           download_url.c_str())));
   ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
       base::StringPrintf("[{\"id\": %d,"
@@ -1547,12 +1526,10 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
 
   ASSERT_TRUE(WaitFor(events::kOnDownloadCreated,
       base::StringPrintf("[{\"danger\": \"safe\","
-                          "  \"filename\": \"%s\","
                           "  \"incognito\": true,"
                           "  \"mime\": \"text/plain\","
                           "  \"paused\": false,"
                           "  \"url\": \"%s\"}]",
-                          GetFilename("slow.txt.crdownload").c_str(),
                           download_url.c_str())));
   ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
       base::StringPrintf("[{\"id\":%d,"
@@ -1703,12 +1680,10 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
 
   ASSERT_TRUE(WaitFor(events::kOnDownloadCreated,
       base::StringPrintf("[{\"danger\": \"safe\","
-                          "  \"filename\": \"%s\","
                           "  \"incognito\": false,"
                           "  \"mime\": \"text/plain\","
                           "  \"paused\": false,"
                           "  \"url\": \"%s\"}]",
-                          GetFilename("slow.txt.crdownload").c_str(),
                           download_url.c_str())));
   ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
       base::StringPrintf("[{\"id\": %d,"
@@ -1746,12 +1721,10 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
 
   ASSERT_TRUE(WaitFor(events::kOnDownloadCreated,
       base::StringPrintf("[{\"danger\": \"safe\","
-                          "  \"filename\": \"%s\","
                           "  \"incognito\": false,"
                           "  \"mime\": \"text/plain\","
                           "  \"paused\": false,"
                           "  \"url\": \"%s\"}]",
-                          GetFilename("data.txt.crdownload").c_str(),
                           download_url.c_str())));
   ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
       base::StringPrintf("[{\"id\": %d,"
@@ -1792,12 +1765,10 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
 
   ASSERT_TRUE(WaitFor(events::kOnDownloadCreated,
       base::StringPrintf("[{\"danger\": \"safe\","
-                          "  \"filename\": \"%s\","
                           "  \"incognito\": false,"
                           "  \"mime\": \"text/html\","
                           "  \"paused\": false,"
                           "  \"url\": \"%s\"}]",
-                          GetFilename("file.txt.crdownload").c_str(),
                           download_url.c_str())));
   ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
       base::StringPrintf("[{\"id\": %d,"
@@ -1999,16 +1970,21 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
 
   ASSERT_TRUE(WaitFor(events::kOnDownloadCreated,
       base::StringPrintf("[{\"danger\": \"safe\","
-                          "  \"incognito\": false,"
-                          "  \"mime\": \"application/octet-stream\","
-                          "  \"paused\": false,"
-                          "  \"bytesReceived\": 164,"
-                          "  \"url\": \"%s\"}]", download_url.c_str())));
+                         "  \"incognito\": false,"
+                         "  \"mime\": \"application/octet-stream\","
+                         "  \"paused\": false,"
+                         "  \"url\": \"%s\"}]", download_url.c_str())));
   ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
       base::StringPrintf("[{\"id\": %d,"
-                          "  \"state\": {"
-                          "    \"previous\": \"in_progress\","
-                          "    \"current\": \"complete\"}}]", result_id)));
+                         "  \"state\": {"
+                         "    \"previous\": \"in_progress\","
+                         "    \"current\": \"complete\"},"
+                         "  \"filename\": {"
+                         "    \"previous\": \"%s\","
+                         "    \"current\": \"%s\"}}]",
+                         result_id,
+                         GetFilename("post-succeed.txt.crdownload").c_str(),
+                         GetFilename("post-succeed.txt").c_str())));
 }
 
 // Test that downloadPostSuccess would fail if the resource requires the POST
@@ -2164,12 +2140,10 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
 
   ASSERT_TRUE(WaitFor(events::kOnDownloadCreated,
       base::StringPrintf("[{\"danger\": \"safe\","
-                          "  \"filename\": \"%s\","
                           "  \"incognito\": false,"
                           "  \"mime\": \"text/plain\","
                           "  \"paused\": false,"
                           "  \"url\": \"%s\"}]",
-                          GetFilename("on_record.txt.crdownload").c_str(),
                           download_url.c_str())));
   ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
       base::StringPrintf("[{\"id\": %d,"
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index a3e72ff..6f168fb 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -604,7 +604,6 @@ void DownloadItemImpl::SetDangerType(content::DownloadDangerType danger_type) {
         net::NetLog::TYPE_DOWNLOAD_ITEM_SAFETY_STATE_UPDATED,
         base::Bind(&download_net_logs::ItemCheckedCallback,
                    GetDangerType(), GetSafetyState()));
-    UpdateObservers();
   }
 }
 
@@ -871,7 +870,6 @@ void DownloadItemImpl::OnTargetPathDetermined(
     const FilePath& target_path,
     TargetDisposition disposition,
     content::DownloadDangerType danger_type) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   target_path_ = target_path;
   target_disposition_ = disposition;
@@ -886,10 +884,10 @@ void DownloadItemImpl::OnTargetPathSelected(const FilePath& target_path) {
 
 void DownloadItemImpl::OnContentCheckCompleted(
     content::DownloadDangerType danger_type) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(AllDataSaved());
   SetDangerType(danger_type);
+  UpdateObservers();
 }
 
 void DownloadItemImpl::OnIntermediatePathDetermined(
diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc
index cbc5838..8bdd159 100644
--- a/content/browser/download/download_item_impl_unittest.cc
+++ b/content/browser/download/download_item_impl_unittest.cc
@@ -299,8 +299,8 @@ TEST_F(DownloadItemTest, NotificationAfterOnTargetPathDetermined) {
   DownloadItemImpl* safe_item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
   MockObserver safe_observer(safe_item);
 
-  // Calling OnTargetPathDetermined does not trigger notification if danger type
-  // is NOT_DANGEROUS.
+  // Calling OnTargetPathDetermined triggers notification regardless of danger
+  // type.
   safe_item->OnTargetPathDetermined(
       FilePath(kDummyPath), DownloadItem::TARGET_DISPOSITION_OVERWRITE,
       content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
@@ -315,7 +315,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnTargetPathDetermined) {
   dangerous_item->OnTargetPathDetermined(
       FilePath(kDummyPath), DownloadItem::TARGET_DISPOSITION_OVERWRITE,
       content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE);
-  EXPECT_TRUE(dangerous_observer.CheckUpdated());
+  EXPECT_FALSE(dangerous_observer.CheckUpdated());
 }
 
 TEST_F(DownloadItemTest, NotificationAfterOnTargetPathSelected) {
@@ -342,7 +342,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
   EXPECT_TRUE(safe_observer.CheckUpdated());
   safe_item->OnContentCheckCompleted(
       content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
-  EXPECT_FALSE(safe_observer.CheckUpdated());
+  EXPECT_TRUE(safe_observer.CheckUpdated());
 
   // Setting to unsafe url or unsafe file should trigger a notification.
   DownloadItemImpl* unsafeurl_item =
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 7570379..08f7369 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -534,6 +534,7 @@ net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
   downloads_[download->GetId()] = download;
   DCHECK(!ContainsKey(active_downloads_, download->GetId()));
   active_downloads_[download->GetId()] = download;
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
 
   return bound_net_log;
 }
@@ -563,6 +564,10 @@ DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem(
   new SavePageExternalData(download);
   DCHECK(SavePageExternalData::Get(download));
 
+  // TODO(benjhayden): Fire OnDownloadCreated for SavePackage downloads when
+  // we're comfortable with the user interacting with them.
+  // FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
+
   // Will notify the observer in the callback.
   if (delegate_)
     delegate_->AddItemToPersistentStore(download);
@@ -947,6 +952,7 @@ void DownloadManagerImpl::OnPersistentStoreQueryComplete(
         this, GetNextId(), entries->at(i), bound_net_log);
     DCHECK(!ContainsKey(downloads_, download->GetId()));
     downloads_[download->GetId()] = download;
+    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
     VLOG(20) << __FUNCTION__ << "()" << i << ">"
              << " download = " << download->DebugString(true);
   }
diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc
index f59f292..6738a5c 100644
--- a/content/browser/download/download_manager_impl_unittest.cc
+++ b/content/browser/download/download_manager_impl_unittest.cc
@@ -194,9 +194,9 @@ class MockDownloadManagerDelegate : public content::DownloadManagerDelegate {
       bool, const content::SavePackagePathPickedCallback&));
 };
 
-MockDownloadManagerDelegate::MockDownloadManagerDelegate() { }
+MockDownloadManagerDelegate::MockDownloadManagerDelegate() {}
 
-MockDownloadManagerDelegate::~MockDownloadManagerDelegate() { }
+MockDownloadManagerDelegate::~MockDownloadManagerDelegate() {}
 
 class MockDownloadFileManager : public DownloadFileManager {
  public:
@@ -240,9 +240,9 @@ class MockDownloadFileManager : public DownloadFileManager {
 };
 
 MockDownloadFileManager::MockDownloadFileManager()
-    : DownloadFileManager(NULL) { }
+    : DownloadFileManager(NULL) {}
 
-MockDownloadFileManager::~MockDownloadFileManager() { }
+MockDownloadFileManager::~MockDownloadFileManager() {}
 
 class MockDownloadItemFactory
     : public content::DownloadItemFactory,
@@ -377,8 +377,8 @@ DownloadItemImpl* MockDownloadItemFactory::CreateSavePageItem(
 
 class MockBrowserContext : public content::BrowserContext {
  public:
-  MockBrowserContext() { }
-  ~MockBrowserContext() { }
+  MockBrowserContext() {}
+  ~MockBrowserContext() {}
 
   MOCK_METHOD0(GetPath, FilePath());
   MOCK_CONST_METHOD0(IsOffTheRecord, bool());
@@ -396,6 +396,18 @@ class MockBrowserContext : public content::BrowserContext {
   MOCK_METHOD0(GetSpecialStoragePolicy, quota::SpecialStoragePolicy*());
 };
 
+class MockDownloadManagerObserver : public content::DownloadManager::Observer {
+ public:
+  MockDownloadManagerObserver() {}
+  ~MockDownloadManagerObserver() {}
+  MOCK_METHOD2(OnDownloadCreated, void(
+        content::DownloadManager*, content::DownloadItem*));
+  MOCK_METHOD1(ModelChanged, void(content::DownloadManager*));
+  MOCK_METHOD1(ManagerGoingDown, void(content::DownloadManager*));
+  MOCK_METHOD2(SelectFileDialogDisplayed, void(
+        content::DownloadManager*, int32));
+};
+
 } // namespace
 
 class DownloadManagerTest : public testing::Test {
@@ -434,6 +446,10 @@ class DownloadManagerTest : public testing::Test {
         mock_download_file_manager_.get(),
         scoped_ptr<content::DownloadItemFactory>(
             mock_download_item_factory_.get()).Pass(), NULL);
+    observer_.reset(new MockDownloadManagerObserver());
+    EXPECT_CALL(GetMockObserver(), ModelChanged(download_manager_.get()))
+        .WillOnce(Return());
+    download_manager_->AddObserver(observer_.get());
     download_manager_->SetDelegate(mock_download_manager_delegate_.get());
     download_manager_->Init(mock_browser_context_.get());
   }
@@ -446,6 +462,8 @@ class DownloadManagerTest : public testing::Test {
       EXPECT_CALL(*item, IsPartialDownload())
           .WillOnce(Return(false));
     }
+    EXPECT_CALL(GetMockObserver(), ManagerGoingDown(download_manager_.get()))
+        .WillOnce(Return());
 
     download_manager_->Shutdown();
     download_manager_ = NULL;
@@ -499,6 +517,10 @@ class DownloadManagerTest : public testing::Test {
     return *mock_download_file_manager_;
   }
 
+  MockDownloadManagerObserver& GetMockObserver() {
+    return *observer_;
+  }
+
   // Probe at private internals.
   void DownloadStopped(DownloadItemImpl* item) {
     download_manager_->DownloadStopped(item);
@@ -545,6 +567,7 @@ class DownloadManagerTest : public testing::Test {
   scoped_ptr<MockDownloadManagerDelegate> mock_download_manager_delegate_;
   scoped_refptr<MockDownloadFileManager> mock_download_file_manager_;
   scoped_ptr<MockBrowserContext> mock_browser_context_;
+  scoped_ptr<MockDownloadManagerObserver> observer_;
   int next_download_id_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerTest);
@@ -558,6 +581,8 @@ TEST_F(DownloadManagerTest, StartDownload) {
 
   EXPECT_FALSE(download_manager_->GetActiveDownloadItem(local_id));
 
+  EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
+      .WillOnce(Return());
   EXPECT_CALL(GetMockDownloadManagerDelegate(), GetNextId())
       .WillOnce(Return(content::DownloadId(this, local_id)));
   EXPECT_CALL(GetMockDownloadManagerDelegate(), GenerateFileHash())
@@ -572,6 +597,10 @@ TEST_F(DownloadManagerTest, StartDownload) {
 
 // Does the DownloadManager prompt when requested?
 TEST_F(DownloadManagerTest, RestartDownload) {
+  EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
+      .WillOnce(Return());
+  EXPECT_CALL(GetMockObserver(), SelectFileDialogDisplayed(
+        download_manager_.get(), 0)).WillOnce(Return());
   // Put a mock we have a handle to on the download manager.
   MockDownloadItemImpl& item(AddItemToManager());
   int download_id = item.GetId();
@@ -597,6 +626,8 @@ TEST_F(DownloadManagerTest, RestartDownload) {
 // download?  Note that this path is tested from RestartDownload
 // to test the non-prompting path in RestartDownload as well.
 TEST_F(DownloadManagerTest, OnTargetPathAvailable) {
+  EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
+      .WillOnce(Return());
   // Put a mock we have a handle to on the download manager.
   MockDownloadItemImpl& item(AddItemToManager());
 
@@ -623,6 +654,8 @@ TEST_F(DownloadManagerTest, OnTargetPathAvailable) {
 // Do the results of an OnDownloadInterrupted get passed through properly
 // to the DownloadItem?
 TEST_F(DownloadManagerTest, OnDownloadInterrupted) {
+  EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
+      .WillOnce(Return());
   // Put a mock we have a handle to on the download manager.
   MockDownloadItemImpl& item(AddItemToManager());
   int download_id = item.GetId();
@@ -638,6 +671,8 @@ TEST_F(DownloadManagerTest, OnDownloadInterrupted) {
 // Does DownloadStopped remove Download from appropriate queues?
 // This test tests non-persisted downloads.
 TEST_F(DownloadManagerTest, OnDownloadStopped_NonPersisted) {
+  EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
+      .WillOnce(Return());
   // Put a mock we have a handle to on the download manager.
   MockDownloadItemImpl& item(AddItemToManager());
 
@@ -659,12 +694,16 @@ TEST_F(DownloadManagerTest, OnDownloadStopped_NonPersisted) {
 // Does DownloadStopped remove Download from appropriate queues?
 // This test tests persisted downloads.
 TEST_F(DownloadManagerTest, OnDownloadStopped_Persisted) {
+  EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
+      .WillOnce(Return());
   // Put a mock we have a handle to on the download manager.
   MockDownloadItemImpl& item(AddItemToManager());
   int download_id = item.GetId();
   int64 db_handle = 0x7;
   EXPECT_CALL(item, GetExternalData(_))
       .WillOnce(Return(static_cast<DownloadItem::ExternalData*>(NULL)));
+  EXPECT_CALL(GetMockObserver(), ModelChanged(download_manager_.get()))
+      .WillOnce(Return());
   AddItemToHistory(item, db_handle);
 
   EXPECT_CALL(item, IsPersisted())
diff --git a/content/public/browser/download_manager.h b/content/public/browser/download_manager.h
index 09628b0..eea06be 100644
--- a/content/public/browser/download_manager.h
+++ b/content/public/browser/download_manager.h
@@ -79,9 +79,20 @@ class CONTENT_EXPORT DownloadManager
   // to the DownloadManager's collection of downloads.
   class CONTENT_EXPORT Observer {
    public:
+    // A DownloadItem was created.  Unlike ModelChanged, this item may be
+    // visible before the filename is determined; in this case the return value
+    // of GetTargetFileName() will be null.  This method may be called an
+    // arbitrary number of times, e.g. when loading history on startup.  As a
+    // result, consumers should avoid doing large amounts of work in
+    // OnDownloadCreated().  TODO(<whoever>): When we've fully specified the
+    // possible states of the DownloadItem in download_item.h and removed
+    // ModelChanged, we should remove the caveat above.
+    virtual void OnDownloadCreated(
+        DownloadManager* manager, DownloadItem* item) {}
+
     // New or deleted download, observers should query us for the current set
     // of downloads.
-    virtual void ModelChanged(DownloadManager* manager) = 0;
+    virtual void ModelChanged(DownloadManager* manager) {}
 
     // Called when the DownloadManager is being destroyed to prevent Observers
     // from calling back to a stale pointer.