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..b1c7655e257c0fe6ad8ee94661f14f6f94bbef05 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -753,6 +753,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
   target_path_ = full_path;
   SetFullPath(full_path);
   delegate_->DownloadRenamedToFinalName(this);
+  UpdateObservers();

[Randy Smith (Not in Mondays), 2012/07/19 17:25:36]

I don't think this is necessary--the notification when we transition to COMPLETE
is going to happen almost immediately, and is almost always what we want
consumers to pay attention to.

[benjhayden, 2012/07/23 15:58:42]

Done.

 
   // Complete the download and release the DownloadFile.
   BrowserThread::PostTask(
@@ -871,10 +872,11 @@ 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;
+  UpdateObservers();
+  // SetDangerType() may call UpdateObservers() again.

[Randy Smith (Not in Mondays), 2012/07/19 17:25:36]

This seems like avoidable redundancy; would it make sense to hoist the
UpdateObservers() call outside of SetDangerType()?  How often is SetDangerType()
called?

[asanka, 2012/07/19 18:14:07]

+1 to avoiding an additional UpdateObservers() call. Also, I think we shouldn't
call UpdateObservers() before seeing the final filename. I.e., avoid calling
UpdateObservers() at all here.

The UpdateObservers() call resulting from SetDangerType() wasn't an issue until
now since no-one outside of downloads/ would have seen DownloadItem yet. But
this assumption is now no longer true.

[Randy Smith (Not in Mondays), 2012/07/19 18:16:51]

I'm thinking this means we should hoist the UpdateObserver() call out of
SetDangerType() (and probably be careful going forward about where we put
UpdateObserver() calls--it's not something I think has received a lot of
attention previously.)

[benjhayden, 2012/07/23 15:58:42]

Pending discussion with Asanka re FND3.

[asanka, 2012/07/23 19:10:21]

My suggestion is to pull the UpdateObservers() call out of SetDangerType() and
only call it in OnContentCheckCompleted().

[benjhayden, 2012/07/25 16:24:38]

Done.

   SetDangerType(danger_type);
 }
 
@@ -882,6 +884,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();

[Randy Smith (Not in Mondays), 2012/07/19 17:25:36]

This also seems redundant, especially since when Asanka's done with FNDR III,
we'll no longer have the option for two UpdateObservers() calls.  Can we just
call this once once we're comfortable we have the final filename?

[asanka, 2012/07/19 18:14:07]

I think the UpdateObservers() call in OnDownloadRenamedToIntermediateName()
captures the state where:
- GetTargetFilePath() is the final target file path.
- GetFullPath() is the intermediate (current) file path.

[benjhayden, 2012/07/23 15:58:42]

Pending discussion with Asanka re FND3.

[asanka, 2012/07/23 19:10:21]

As above, I think we shouldn't call UpdateObservers() here. We can instead rely
on the observers being notified in OnDownloadRenamedToIntermediateName().

[benjhayden, 2012/07/25 16:24:38]

Done.

 }
 
 void DownloadItemImpl::OnContentCheckCompleted(
@@ -917,6 +920,7 @@ FilePath DownloadItemImpl::GetFileNameToReportUser() const {
 
 void DownloadItemImpl::SetDisplayName(const FilePath& name) {
   display_name_ = name;
+  UpdateObservers();

[asanka, 2012/07/19 18:14:07]

Same as above. This is an intermediate state that I think we  don't want to
announce.

[benjhayden, 2012/07/23 15:58:42]

Done.

 }
 
 FilePath DownloadItemImpl::GetUserVerifiedFilePath() const {