DownloadManager::Observer::OnDownloadCreated

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index a3e72ffa469e50f0efd18f92c63a50c2a99e0e16..5fba3ede98ff84b9cb6ef2e7bfe91cbf7c0bf74c 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,10 +870,10 @@ 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;
+  // SetDangerType() may call UpdateObservers() again.

[asanka, 2012/07/25 19:36:35]

Stale comment?

[benjhayden, 2012/07/25 21:26:26]

Done.

   SetDangerType(danger_type);
 }
 
@@ -882,6 +881,7 @@ void DownloadItemImpl::OnTargetPathSelected(const FilePath& target_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(TARGET_DISPOSITION_PROMPT, target_disposition_);
   target_path_ = target_path;
+  UpdateObservers();
 }
 
 void DownloadItemImpl::OnContentCheckCompleted(
@@ -890,6 +890,7 @@ void DownloadItemImpl::OnContentCheckCompleted(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(AllDataSaved());
   SetDangerType(danger_type);
+  UpdateObservers();

[asanka, 2012/07/25 19:36:35]

I was under the impression that we weren't going to do an UpdateObservers() here
since we do that immediately after the intermediate file is renamed. Is that not
the case?

[benjhayden, 2012/07/25 21:26:26]

I am so far down the rabbit hole, I don't know anything anymore. I was just
following this:
http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
asanka 2012/07/23 19:10:21
My suggestion is to pull the UpdateObservers() call out of SetDangerType() and
only call it in OnContentCheckCompleted().

Just let me know exactly where to move this call if you want it moved. I'll fix
the tests when we figure this out.

[asanka, 2012/07/25 21:56:04]

Whoa. Apparently I clicked on the wrong UpdateObservers() call when entering the
comment. Sorry about that. I blame this on the broken coffee machine.

To recap:

- UpdateObservers() in OnContentCheckCompleted() is necessary since
SetDangerType() doesn't update observers anymore.

- UpdateObservers() in OnTargetPathSelected() is unnecessary since we end up
calling UpdateObservers() once the download has been renamed. The call in
OnDownloadRenamedToIntermediateName() is better because we have both
GetTargetFilePath() and GetFullPath() by then.

[benjhayden, 2012/07/26 15:03:42]

Thank you! That was very helpful. :-)
PTAL

 }
 
 void DownloadItemImpl::OnIntermediatePathDetermined(