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 586 matching lines @@
   // Notify observers if the safety state has changed as a result of the new
   // danger type.
   SafetyState updated_value = IsDangerous() ?
       DownloadItem::DANGEROUS : DownloadItem::SAFE;
   if (updated_value != safety_state_) {
     safety_state_ = updated_value;
     bound_net_log_.AddEvent(
         net::NetLog::TYPE_DOWNLOAD_ITEM_SAFETY_STATE_UPDATED,
         base::Bind(&download_net_logs::ItemCheckedCallback,
                    GetDangerType(), GetSafetyState()));
-    UpdateObservers();
   }
 }
 
 void DownloadItemImpl::SetFullPath(const FilePath& new_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   VLOG(20) << __FUNCTION__ << "()"
            << " new_path = \"" << new_path.value() << "\""
            << " " << DebugString(true);
   DCHECK(!new_path.empty());
   current_path_ = new_path;
@@ skipping 246 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;
+  // 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);
 }
 
 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(
     content::DownloadDangerType danger_type) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.

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

Nit: Remove this comment?

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

Done.

   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(
     DownloadFileManager* file_manager,
     const FilePath& intermediate_path) {
   DownloadFileManager::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
                  weak_ptr_factory_.GetWeakPtr());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
@@ skipping 283 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;
   }
 }