clean 96627/85408 debugging mechanisms out of DownloadManager/DownloadDatabase

OWNERS:
brettw: chrome/browser/history/download_database.cc
jhawkins: chrome/browser/ui/webui/downloads_dom_handler.cc

James, feel free to redirect to arv/estade/csilv.

Review URL: http://codereview.chromium.org/10243004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@135185 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 40 insertions, 113 deletions

diff --git a/chrome/browser/history/download_database.cc b/chrome/browser/history/download_database.cc
index 1766d48..5039e18 100644
--- a/chrome/browser/history/download_database.cc
+++ b/chrome/browser/history/download_database.cc
@@ -19,16 +19,6 @@
 #include "content/public/browser/download_persistent_store_info.h"
 #include "sql/statement.h"
 
-// TODO(benjhayden): Change this to DCHECK when we have more debugging
-// information from the next dev cycle, before the next stable/beta branch is
-// cut, in order to prevent unnecessary crashes on those channels. If we still
-// don't have root cause before the dev cycle after the next stable/beta
-// releases, uncomment it out to re-enable debugging checks. Whenever this macro
-// is toggled, the corresponding macro in download_manager_impl.cc should also
-// be toggled. When 96627 is fixed, this macro and all its usages and
-// returned_ids_ can be deleted or permanently changed to DCHECK as appropriate.
-#define CHECK_96627 CHECK
-
 using content::DownloadItem;
 using content::DownloadPersistentStoreInfo;
 
@@ -88,7 +78,7 @@ DownloadDatabase::~DownloadDatabase() {
 
 void DownloadDatabase::CheckThread() {
   if (owning_thread_set_) {
-    CHECK_96627(owning_thread_ == base::PlatformThread::CurrentId());
+    DCHECK(owning_thread_ == base::PlatformThread::CurrentId());
   } else {
     owning_thread_ = base::PlatformThread::CurrentId();
     owning_thread_set_ = true;
@@ -149,7 +139,7 @@ void DownloadDatabase::QueryDownloads(
     if (!db_handles.insert(info.db_handle).second) {
       // info.db_handle was already in db_handles. The database is corrupt.
       base::debug::Alias(&info.db_handle);
-      CHECK_96627(false);
+      DCHECK(false);
     }
   }
 }
diff --git a/chrome/browser/ui/webui/downloads_dom_handler.cc b/chrome/browser/ui/webui/downloads_dom_handler.cc
index 0d10854..1958a78 100644
--- a/chrome/browser/ui/webui/downloads_dom_handler.cc
+++ b/chrome/browser/ui/webui/downloads_dom_handler.cc
@@ -225,11 +225,9 @@ void DownloadsDOMHandler::ModelChanged(content::DownloadManager* manager) {
     if (static_cast<int>(it - download_items_.begin()) > kMaxDownloads)
       break;
 
-    // TODO(rdsmith): Convert to DCHECK()s when http://crbug.com/85408 is
-    // fixed.
     // We should never see anything that isn't already in the history.
-    CHECK(*it);
-    CHECK((*it)->IsPersisted());
+    DCHECK(*it);
+    DCHECK((*it)->IsPersisted());
 
     (*it)->AddObserver(this);
   }
@@ -317,8 +315,7 @@ void DownloadsDOMHandler::HandleRemove(const ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_REMOVE);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file) {
-    // TODO(rdsmith): Change to DCHECK when http://crbug.com/85408 is fixed.
-    CHECK(file->IsPersisted());
+    DCHECK(file->IsPersisted());
     file->Remove();
   }
 }
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 1281e7a..2f6b380 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -299,8 +299,7 @@ DownloadItemImpl::DownloadItemImpl(Delegate* delegate,
 }
 
 DownloadItemImpl::~DownloadItemImpl() {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   TransitionTo(REMOVING);
   STLDeleteContainerPairSecondPointers(
@@ -310,22 +309,19 @@ DownloadItemImpl::~DownloadItemImpl() {
 }
 
 void DownloadItemImpl::AddObserver(Observer* observer) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   observers_.AddObserver(observer);
 }
 
 void DownloadItemImpl::RemoveObserver(Observer* observer) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   observers_.RemoveObserver(observer);
 }
 
 void DownloadItemImpl::UpdateObservers() {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadUpdated(this));
 }
@@ -343,8 +339,7 @@ bool DownloadItemImpl::ShouldOpenFileBasedOnExtension() {
 }
 
 void DownloadItemImpl::OpenDownload() {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (IsPartialDownload()) {
     // We don't honor the open_when_complete_ flag for temporary
@@ -376,8 +371,7 @@ void DownloadItemImpl::OpenDownload() {
 }
 
 void DownloadItemImpl::ShowDownloadInShell() {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   content::GetContentClient()->browser()->ShowItemInFolder(GetFullPath());
 }
@@ -404,8 +398,7 @@ void DownloadItemImpl::DangerousDownloadValidated() {
 
 void DownloadItemImpl::ProgressComplete(int64 bytes_so_far,
                                         const std::string& final_hash) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   hash_ = final_hash;
   hash_state_ = "";
@@ -443,8 +436,7 @@ void DownloadItemImpl::UpdateProgress(int64 bytes_so_far,
 void DownloadItemImpl::UpdateProgress(int64 bytes_so_far,
                                       int64 bytes_per_sec,
                                       const std::string& hash_state) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!IsInProgress()) {
     NOTREACHED();
@@ -457,8 +449,7 @@ void DownloadItemImpl::UpdateProgress(int64 bytes_so_far,
 
 // Triggered by a user action.
 void DownloadItemImpl::Cancel(bool user_cancel) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   last_reason_ = user_cancel ?
       content::DOWNLOAD_INTERRUPT_REASON_USER_CANCELED :
@@ -479,8 +470,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
 }
 
 void DownloadItemImpl::MarkAsComplete() {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
@@ -494,8 +484,7 @@ void DownloadItemImpl::DelayedDownloadOpened() {
 
 void DownloadItemImpl::OnAllDataSaved(
     int64 size, const std::string& final_hash) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(!all_data_saved_);
   all_data_saved_ = true;
@@ -514,8 +503,7 @@ void DownloadItemImpl::MaybeCompleteDownload() {
 }
 
 void DownloadItemImpl::Completed() {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "() " << DebugString(false);
 
@@ -601,8 +589,7 @@ void DownloadItemImpl::UpdateSafetyState() {
 }
 
 void DownloadItemImpl::UpdateTarget() {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (state_info_.target_name.value().empty())
     state_info_.target_name = full_path_.BaseName();
@@ -611,8 +598,7 @@ void DownloadItemImpl::UpdateTarget() {
 void DownloadItemImpl::Interrupted(int64 size,
                                    const std::string& hash_state,
                                    content::DownloadInterruptReason reason) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!IsInProgress())
     return;
@@ -626,8 +612,7 @@ void DownloadItemImpl::Interrupted(int64 size,
 }
 
 void DownloadItemImpl::Delete(DeleteReason reason) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   switch (reason) {
     case DELETE_DUE_TO_USER_DISCARD:
@@ -651,8 +636,7 @@ void DownloadItemImpl::Delete(DeleteReason reason) {
 }
 
 void DownloadItemImpl::Remove() {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   delegate_->AssertStateConsistent(this);
   Cancel(true);
@@ -701,8 +685,7 @@ void DownloadItemImpl::OnPathDetermined(const FilePath& path) {
 }
 
 void DownloadItemImpl::Rename(const FilePath& full_path) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
@@ -719,8 +702,7 @@ void DownloadItemImpl::Rename(const FilePath& full_path) {
 }
 
 void DownloadItemImpl::TogglePause() {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(IsInProgress());
   if (is_paused_)
@@ -732,8 +714,7 @@ void DownloadItemImpl::TogglePause() {
 }
 
 void DownloadItemImpl::OnDownloadCompleting(DownloadFileManager* file_manager) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
@@ -764,8 +745,7 @@ void DownloadItemImpl::OnDownloadCompleting(DownloadFileManager* file_manager) {
 }
 
 void DownloadItemImpl::OnDownloadRenamedToFinalName(const FilePath& full_path) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
@@ -809,8 +789,7 @@ bool DownloadItemImpl::MatchesQuery(const string16& query) const {
 }
 
 void DownloadItemImpl::SetFileCheckResults(const DownloadStateInfo& state) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true);
   state_info_ = state;
@@ -824,8 +803,7 @@ content::DownloadDangerType DownloadItemImpl::GetDangerType() const {
 }
 
 void DownloadItemImpl::SetDangerType(content::DownloadDangerType danger_type) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   state_info_.danger = danger_type;
   UpdateSafetyState();
 }
@@ -896,8 +874,7 @@ void DownloadItemImpl::OffThreadCancel(DownloadFileManager* file_manager) {
 
 void DownloadItemImpl::Init(bool active,
                             download_net_logs::DownloadType download_type) {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   UpdateTarget();
   if (active)
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 9bdb48a..a8f79e2 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -36,16 +36,6 @@
 #include "content/public/browser/web_contents_delegate.h"
 #include "net/base/upload_data.h"
 
-// TODO(benjhayden): Change this to DCHECK when we have more debugging
-// information from the next dev cycle, before the next stable/beta branch is
-// cut, in order to prevent unnecessary crashes on those channels. If we still
-// don't have root cause before the dev cycle after the next stable/beta
-// releases, uncomment it out to re-enable debugging checks. Whenever this macro
-// is toggled, the corresponding macro in download_database.cc should also
-// be toggled. When 96627 is fixed, this macro and all its usages can be
-// deleted or permanently changed to DCHECK as appropriate.
-#define CHECK_96627 CHECK
-
 using content::BrowserThread;
 using content::DownloadId;
 using content::DownloadItem;
@@ -174,7 +164,6 @@ DownloadManagerImpl::DownloadManagerImpl(
           browser_context_(NULL),
           file_manager_(NULL),
           delegate_(delegate),
-          largest_db_handle_in_history_(DownloadItem::kUninitializedHandle),
           net_log_(net_log) {
 }
 
@@ -433,7 +422,7 @@ net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
   int32 download_id = info->download_id.local();
   DCHECK(!ContainsKey(in_progress_, download_id));
 
-  CHECK_96627(!ContainsKey(active_downloads_, download_id));
+  DCHECK(!ContainsKey(active_downloads_, download_id));
   downloads_.insert(download);
   active_downloads_[download_id] = download;
 
@@ -559,12 +548,11 @@ void DownloadManagerImpl::OnResponseCompleted(int32 download_id,
 }
 
 void DownloadManagerImpl::AssertStateConsistent(DownloadItem* download) const {
-  // TODO(rdsmith): Change to DCHECK after http://crbug.com/96627 resolved.
   if (download->GetState() == DownloadItem::REMOVING) {
-    CHECK(!ContainsKey(downloads_, download));
-    CHECK(!ContainsKey(active_downloads_, download->GetId()));
-    CHECK(!ContainsKey(in_progress_, download->GetId()));
-    CHECK(!ContainsKey(history_downloads_, download->GetDbHandle()));
+    DCHECK(!ContainsKey(downloads_, download));
+    DCHECK(!ContainsKey(active_downloads_, download->GetId()));
+    DCHECK(!ContainsKey(in_progress_, download->GetId()));
+    DCHECK(!ContainsKey(history_downloads_, download->GetDbHandle()));
     return;
   }
 
@@ -577,7 +565,7 @@ void DownloadManagerImpl::AssertStateConsistent(DownloadItem* download) const {
   } else {
     for (DownloadMap::const_iterator it = history_downloads_.begin();
          it != history_downloads_.end(); ++it) {
-      CHECK_96627(it->second != download);
+      DCHECK(it->second != download);
     }
   }
 
@@ -934,14 +922,10 @@ void DownloadManagerImpl::FileSelectionCanceled(int32 download_id) {
 // 'DownloadPersistentStoreInfo's in sorted order (by ascending start_time).
 void DownloadManagerImpl::OnPersistentStoreQueryComplete(
     std::vector<DownloadPersistentStoreInfo>* entries) {
-  // TODO(rdsmith): Remove this and related logic when
-  // http://crbug.com/96627 is fixed.
-  largest_db_handle_in_history_ = 0;
-
   for (size_t i = 0; i < entries->size(); ++i) {
     int64 db_handle = entries->at(i).db_handle;
     base::debug::Alias(&db_handle);
-    CHECK_96627(!ContainsKey(history_downloads_, db_handle));
+    DCHECK(!ContainsKey(history_downloads_, db_handle));
 
     net::BoundNetLog bound_net_log =
         net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
@@ -951,9 +935,6 @@ void DownloadManagerImpl::OnPersistentStoreQueryComplete(
     history_downloads_[download->GetDbHandle()] = download;
     VLOG(20) << __FUNCTION__ << "()" << i << ">"
              << " download = " << download->DebugString(true);
-
-    if (download->GetDbHandle() > largest_db_handle_in_history_)
-      largest_db_handle_in_history_ = download->GetDbHandle();
   }
   NotifyModelChanged();
   CheckForHistoryFilesRemoval();
@@ -962,10 +943,7 @@ void DownloadManagerImpl::OnPersistentStoreQueryComplete(
 void DownloadManagerImpl::AddDownloadItemToHistory(DownloadItem* download,
                                                    int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/96627
-  // is fixed.
-  CHECK_NE(DownloadItem::kUninitializedHandle, db_handle);
+  DCHECK_NE(DownloadItem::kUninitializedHandle, db_handle);
 
   download_stats::RecordHistorySize(history_downloads_.size());
 
@@ -973,9 +951,7 @@ void DownloadManagerImpl::AddDownloadItemToHistory(DownloadItem* download,
   download->SetDbHandle(db_handle);
   download->SetIsPersisted();
 
-  // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/96627
-  // is fixed.
-  CHECK_96627(!ContainsKey(history_downloads_, download->GetDbHandle()));
+  DCHECK(!ContainsKey(history_downloads_, download->GetDbHandle()));
   history_downloads_[download->GetDbHandle()] = download;
 
   // Show in the appropriate browser UI.
@@ -1011,15 +987,12 @@ void DownloadManagerImpl::OnDownloadItemAddedToPersistentStore(
            << " download_id = " << download_id
            << " download = " << download->DebugString(true);
 
-  // TODO(rdsmith): Remove after http://crbug.com/96627 resolved.
-  int64 largest_handle = largest_db_handle_in_history_;
-  base::debug::Alias(&largest_handle);
   int32 matching_item_download_id
       = (ContainsKey(history_downloads_, db_handle) ?
          history_downloads_[db_handle]->GetId() : 0);
   base::debug::Alias(&matching_item_download_id);
 
-  CHECK_96627(!ContainsKey(history_downloads_, db_handle));
+  DCHECK(!ContainsKey(history_downloads_, db_handle));
 
   AddDownloadItemToHistory(download, db_handle);
 
@@ -1033,10 +1006,7 @@ void DownloadManagerImpl::OnDownloadItemAddedToPersistentStore(
   if (download->IsInProgress()) {
     MaybeCompleteDownload(download);
   } else {
-    // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/96627
-    // is fixed.
-    CHECK(download->IsCancelled())
-        << " download = " << download->DebugString(true);
+    DCHECK(download->IsCancelled());
     in_progress_.erase(download_id);
     active_downloads_.erase(download_id);
     delegate_->UpdateItemInPersistentStore(download);
@@ -1174,10 +1144,7 @@ void DownloadManagerImpl::OnSavePageItemAddedToPersistentStore(
     return;
   }
 
-  // TODO(rdsmith): Remove after http://crbug.com/96627 resolved.
-  int64 largest_handle = largest_db_handle_in_history_;
-  base::debug::Alias(&largest_handle);
-  CHECK_96627(!ContainsKey(history_downloads_, db_handle));
+  DCHECK(!ContainsKey(history_downloads_, db_handle));
 
   AddDownloadItemToHistory(download, db_handle);
 
diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
index 90dda08..36a3716 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -246,10 +246,6 @@ class CONTENT_EXPORT DownloadManagerImpl
   // Allows an embedder to control behavior. Guaranteed to outlive this object.
   content::DownloadManagerDelegate* delegate_;
 
-  // TODO(rdsmith): Remove when http://crbug.com/85408 is fixed.
-  // For debugging only.
-  int64 largest_db_handle_in_history_;
-
   net::NetLog* net_log_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerImpl);