Put history insertion for downloads processing inline.

chrome/browser/download/download_item.cc

 // Copyright (c) 2010 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 "chrome/browser/download/download_item.h"
 
 #include "app/l10n_util.h"
 #include "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/stringprintf.h"
 #include "base/timer.h"
 #include "base/utf_string_conversions.h"
 #include "net/base/net_util.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/download/download_extensions.h"
 #include "chrome/browser/download/download_file_manager.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/history/download_create_info.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/pref_names.h"
 
+// A DownloadItem normally goes through the following states:
+//      * Created (when download starts)
+//      * Made visible to consumers (e.g. Javascript) after the
+//        destination file has been determined.
+//      * Entered into the history database.
+//      * Made visible in the download shelf.
+//      * All data is received.  Note that the actual data download occurs
+//        in parallel with the above steps, but until those steps are
+//        complete, completion of the data download will be ignored.
+//      * Download file is renamed to its final name, and possibly
+//        auto-opened.
+// TODO(rdsmith): This progress should be reflected in
+// DownloadItem::DownloadState and a state transition table/state diagram.
+//
+// TODO(rdsmith): This description should be updated to reflect the cancel
+// pathways.
+
 namespace {
 
 // Update frequency (milliseconds).
 const int kUpdateTimeMs = 1000;
 
 void DeleteDownloadedFile(const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   // Make sure we only delete files.
   if (!file_util::DirectoryExists(path))
@@ skipping 54 matching lines @@
       safety_state_(SAFE),
       auto_opened_(false),
       target_name_(info.original_name),
       render_process_id_(-1),
       request_id_(-1),
       save_as_(false),
       is_otr_(false),
       is_extension_install_(info.is_extension_install),
       name_finalized_(false),
       is_temporary_(false),
+      all_data_received_(false),
       opened_(false) {
   if (state_ == IN_PROGRESS)
     state_ = CANCELLED;
+  if (state_ == COMPLETE)
+    all_data_received_ = true;
   Init(false /* don't start progress timer */);
 }
 
 // Constructing for a regular download:
 DownloadItem::DownloadItem(DownloadManager* download_manager,
                            const DownloadCreateInfo& info,
                            bool is_otr)
     : id_(info.download_id),
       full_path_(info.path),
       path_uniquifier_(info.path_uniquifier),
@@ skipping 13 matching lines @@
       safety_state_(info.is_dangerous ? DANGEROUS : SAFE),
       auto_opened_(false),
       target_name_(info.original_name),
       render_process_id_(info.child_id),
       request_id_(info.request_id),
       save_as_(info.prompt_user_for_save_location),
       is_otr_(is_otr),
       is_extension_install_(info.is_extension_install),
       name_finalized_(false),
       is_temporary_(!info.save_info.file_path.empty()),
+      all_data_received_(false),
       opened_(false) {
   Init(true /* start progress timer */);
 }
 
 // Constructing for the "Save Page As..." feature:
 DownloadItem::DownloadItem(DownloadManager* download_manager,
                            const FilePath& path,
                            const GURL& url,
                            bool is_otr)
     : id_(1),
@@ skipping 14 matching lines @@
       open_when_complete_(false),
       safety_state_(SAFE),
       auto_opened_(false),
       render_process_id_(-1),
       request_id_(-1),
       save_as_(false),
       is_otr_(is_otr),
       is_extension_install_(false),
       name_finalized_(false),
       is_temporary_(false),
+      all_data_received_(false),
       opened_(false) {
   Init(true /* start progress timer */);
 }
 
 DownloadItem::~DownloadItem() {
   state_ = REMOVING;
   UpdateObservers();
 }
 
 void DownloadItem::AddObserver(Observer* observer) {
@@ skipping 105 matching lines @@
     // Small downloads might be complete before this method has a chance to run.
     return;
   }
   state_ = CANCELLED;
   UpdateObservers();
   StopProgressTimer();
   if (update_history)
     download_manager_->DownloadCancelled(id_);
 }
 
+void DownloadItem::OnDataReceptionAccepted() {

[Paweł Hajdan Jr., 2011/01/19 17:16:15]

This is another ambiguous name that I'd like to avoid. How about MarkAsComplete?

[Randy Smith (Not in Mondays), 2011/01/19 19:53:03]

Done.

+  DCHECK(all_data_received_);
+  state_ = COMPLETE;
+}
+
 void DownloadItem::OnAllDataSaved(int64 size) {
-  state_ = COMPLETE;
+  DCHECK(!all_data_received_);
+  all_data_received_ = true;

[Paweł Hajdan Jr., 2011/01/19 17:16:15]

nit: Now we have two names for the same thing: "saved" and "received". I'd
prefer to stay with Saved.

[Randy Smith (Not in Mondays), 2011/01/19 19:53:03]

Done.

   UpdateSize(size);
   StopProgressTimer();
 }
 
 void DownloadItem::Finished() {
   // Handle chrome extensions explicitly and skip the shell execute.
   if (is_extension_install()) {
     download_util::OpenChromeExtension(download_manager_->profile(),
                                        download_manager_,
                                        *this);
     auto_opened_ = true;
   } else if (open_when_complete() ||
              download_manager_->ShouldOpenFileBasedOnExtension(
                  GetUserVerifiedFilePath()) ||
              is_temporary()) {
     // If the download is temporary, like in drag-and-drop, do not open it but
     // we still need to set it auto-opened so that it can be removed from the
     // download shelf.
     if (!is_temporary())
       OpenDownload();
     auto_opened_ = true;
   }
 
-  // Notify our observers that we are complete (the call to OnAllDataSaved()
+  // Notify our observers that we are complete (the call to OnReadyToFinish()

[Paweł Hajdan Jr., 2011/01/19 17:16:15]

nit: This method name no longer exists.

[Randy Smith (Not in Mondays), 2011/01/19 19:53:03]

Fixed, though now that you point it out, I don't completely like that--I'd like
to have an invariant of modifying the state and updating observers.  In a
different CL.

   // set the state to complete but did not notify).
   UpdateObservers();
 
   // The download file is meant to be completed if both the filename is
   // finalized and the file data is downloaded. The ordering of these two
   // actions is indeterministic. Thus, if the filename is not finalized yet,
   // delay the notification.
   if (name_finalized()) {
     NotifyObserversDownloadFileCompleted();
     download_manager_->RemoveFromActiveList(id());
@@ skipping 33 matching lines @@
   return diff_ms == 0 ? 0 : received_bytes_ * 1000 / diff_ms;
 }
 
 int DownloadItem::PercentComplete() const {
   int percent = -1;
   if (total_bytes_ > 0)
     percent = static_cast<int>(received_bytes_ * 100.0 / total_bytes_);
   return percent;
 }
 
+bool DownloadItem::AllDataReceived() const {

[Paweł Hajdan Jr., 2011/01/19 17:16:15]

nit: Can we add prefix like "Is" or "Has" to unambiguously indicate it's a
getter?

By the way, I think it should just use hacker_style instead of CamelCase. Then
no prefix should be needed.

[Randy Smith (Not in Mondays), 2011/01/19 19:53:03]

The reason I hadn't done this is that I'm not providing a setter (and have no
intention of doing so), so I don't think of it is "just exporting an accessor." 
But when I read the style guide more carefully, it's pretty solidly on your
side.  Done.

+  return all_data_received_;
+}
+
 void DownloadItem::Rename(const FilePath& full_path) {
   VLOG(20) << " " << __FUNCTION__ << "()"
            << " full_path = " << full_path.value()
            << DebugString(true);
   DCHECK(!full_path.empty());
   full_path_ = full_path;
 }
 
 void DownloadItem::TogglePause() {
   DCHECK(state_ == IN_PROGRESS);
@@ skipping 159 matching lines @@
         target_name_.value().c_str(),
         full_path().value().c_str());
   } else {
     description += base::StringPrintf(" url = \"%s\"", url().spec().c_str());
   }
 
   description += " }";
 
   return description;
 }