DownloadManager::Observer::OnDownloadCreated

content/browser/download/download_item_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/download_item_impl.h"
 
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ skipping 735 matching lines @@
   if (content::DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
     Interrupt(reason);
     return;
   }
 
   // full_path is now the current and target file path.
   DCHECK(!full_path.empty());
   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(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&DownloadFileManager::CompleteDownload,
                  file_manager, GetGlobalId(),
                  base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
                             weak_ptr_factory_.GetWeakPtr())));
 }
 
@@ skipping 98 matching lines @@
 }
 
 DownloadItem::TargetDisposition DownloadItemImpl::GetTargetDisposition() const {
   return target_disposition_;
 }
 
 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);
 }
 
 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(
     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);
 }
 
@@ skipping 15 matching lines @@
 }
 
 FilePath DownloadItemImpl::GetFileNameToReportUser() const {
   if (!display_name_.empty())
     return display_name_;
   return target_path_.BaseName();
 }
 
 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 {
   return (safety_state_ == DownloadItem::SAFE) ?
       GetTargetFilePath() : GetFullPath();
 }
 
 void DownloadItemImpl::OffThreadCancel(DownloadFileManager* file_manager) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   request_handle_->CancelRequest();
@@ skipping 256 matching lines @@
   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;
   }
 }