Misc. tiny cleanups to downloads. Reduce API surface, shorten code. Only functional change should be that DownloadItem::Completed() now effectively DCHECK()s that |all_data_saved_| is true before returning, which I think is a correct assertion.

BUG=none
TEST=none
Review URL: http://codereview.chromium.org/6935031

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

4 files changed, 39 insertions, 72 deletions

diff --git a/chrome/browser/download/download_extensions.cc b/chrome/browser/download/download_extensions.cc
index b9cac1b..7bb71d8 100644
--- a/chrome/browser/download/download_extensions.cc
+++ b/chrome/browser/download/download_extensions.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -187,11 +187,7 @@ static const struct Executables {
 };
 
 DownloadDangerLevel GetFileDangerLevel(const FilePath& path) {
-  return GetFileExtensionDangerLevel(path.Extension());
-}
-
-DownloadDangerLevel GetFileExtensionDangerLevel(
-    const FilePath::StringType& extension) {
+  FilePath::StringType extension(path.Extension());
   if (extension.empty())
     return NotDangerous;
   if (!IsStringASCII(extension))
@@ -213,10 +209,6 @@ DownloadDangerLevel GetFileExtensionDangerLevel(
   return NotDangerous;
 }
 
-bool IsFileSafe(const FilePath& path) {
-  return GetFileDangerLevel(path) == NotDangerous;
-}
-
 static const char* kExecutableWhiteList[] = {
   // JavaScript is just as powerful as EXE.
   "text/javascript",
diff --git a/chrome/browser/download/download_extensions.h b/chrome/browser/download/download_extensions.h
index 022f2cd..a8ddae7 100644
--- a/chrome/browser/download/download_extensions.h
+++ b/chrome/browser/download/download_extensions.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -21,13 +21,6 @@ enum DownloadDangerLevel {
 // Determine the download danger level of a file.
 DownloadDangerLevel GetFileDangerLevel(const FilePath& path);
 
-// Determine the download danger level using a file extension.
-DownloadDangerLevel GetFileExtensionDangerLevel(
-    const FilePath::StringType& extension);
-
-// True if the download danger level of the file is NotDangerous.
-bool IsFileSafe(const FilePath& path);
-
 // Tests if we think the server means for this mime_type to be executable.
 bool IsExecutableMimeType(const std::string& mime_type);
 
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index 58b70a3..c94d3b8 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -105,10 +105,9 @@ DownloadItem::DangerType GetDangerType(bool dangerous_file,
   if (dangerous_url) {
     // dangerous URL overweights dangerous file. We check dangerous URL first.
     return DownloadItem::DANGEROUS_URL;
-  } else if (dangerous_file) {
-    return DownloadItem::DANGEROUS_FILE;
   }
-  return DownloadItem::NOT_DANGEROUS;
+  return dangerous_file ?
+      DownloadItem::DANGEROUS_FILE : DownloadItem::NOT_DANGEROUS;
 }
 
 }  // namespace
@@ -333,7 +332,7 @@ void DownloadItem::Update(int64 bytes_so_far) {
 
 // Triggered by a user action.
 void DownloadItem::Cancel(bool update_history) {
-  VLOG(20) << __FUNCTION__ << "()" << " download = " << DebugString(true);
+  VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   if (!IsPartialDownload()) {
     // Small downloads might be complete before this method has
     // a chance to run.
@@ -363,8 +362,7 @@ void DownloadItem::OnAllDataSaved(int64 size) {
 }
 
 void DownloadItem::Completed() {
-  VLOG(20) << " " << __FUNCTION__ << "() "
-           << DebugString(false);
+  VLOG(20) << __FUNCTION__ << "() " << DebugString(false);
 
   download_util::RecordDownloadCount(download_util::COMPLETED_COUNT);
 
@@ -405,21 +403,18 @@ void DownloadItem::Interrupted(int64 size, int os_error) {
 void DownloadItem::Delete(DeleteReason reason) {
   switch (reason) {
     case DELETE_DUE_TO_USER_DISCARD:
-      UMA_HISTOGRAM_ENUMERATION("Download.UserDiscard",
-                                danger_type_,
+      UMA_HISTOGRAM_ENUMERATION("Download.UserDiscard", danger_type_,
                                 DANGEROUS_TYPE_MAX);
       break;
     case DELETE_DUE_TO_BROWSER_SHUTDOWN:
-      UMA_HISTOGRAM_ENUMERATION("Download.Discard",
-                                danger_type_,
+      UMA_HISTOGRAM_ENUMERATION("Download.Discard", danger_type_,
                                 DANGEROUS_TYPE_MAX);
       break;
     default:
       NOTREACHED();
   }
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
       NewRunnableFunction(&DeleteDownloadedFile, full_path_));
   Remove();
   // We have now been deleted.
@@ -454,16 +449,14 @@ int64 DownloadItem::CurrentSpeed() const {
 }
 
 int DownloadItem::PercentComplete() const {
-  int percent = -1;
-  if (total_bytes_ > 0)
-    percent = static_cast<int>(received_bytes_ * 100.0 / total_bytes_);
-  return percent;
+  return (total_bytes_ > 0) ?
+      static_cast<int>(received_bytes_ * 100.0 / total_bytes_) : -1;
 }
 
 void DownloadItem::Rename(const FilePath& full_path) {
-  VLOG(20) << " " << __FUNCTION__ << "()"
+  VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
-           << DebugString(true);
+           << " " << DebugString(true);
   DCHECK(!full_path.empty());
   full_path_ = full_path;
 }
@@ -476,31 +469,29 @@ void DownloadItem::TogglePause() {
 }
 
 void DownloadItem::OnDownloadCompleting(DownloadFileManager* file_manager) {
-  VLOG(20) << " " << __FUNCTION__ << "() "
+  VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
   DCHECK_NE(DANGEROUS, safety_state());
   DCHECK(file_manager);
 
   if (NeedsRename()) {
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        NewRunnableMethod(
-            file_manager, &DownloadFileManager::RenameCompletingDownloadFile,
-            id(), GetTargetFilePath(), safety_state() == SAFE));
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+        NewRunnableMethod(file_manager,
+            &DownloadFileManager::RenameCompletingDownloadFile, id(),
+            GetTargetFilePath(), safety_state() == SAFE));
     return;
   }
 
   Completed();
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager, &DownloadFileManager::CompleteDownload, id()));
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+      NewRunnableMethod(file_manager, &DownloadFileManager::CompleteDownload,
+                        id()));
 }
 
 void DownloadItem::OnDownloadRenamedToFinalName(const FilePath& full_path) {
-  VLOG(20) << " " << __FUNCTION__ << "()"
+  VLOG(20) << __FUNCTION__ << "()"
            << " full_path = " << full_path.value()
            << " needed rename = " << NeedsRename()
            << " " << DebugString(false);
@@ -537,10 +528,7 @@ bool DownloadItem::MatchesQuery(const string16& query) const {
   // due to normalization and we have a fancier search-query system
   // used elsewhere.
   // http://code.google.com/p/chromium/issues/detail?id=71982
-  if (path.find(query) != string16::npos)
-    return true;
-
-  return false;
+  return (path.find(query) != string16::npos);
 }
 
 void DownloadItem::SetFileCheckResults(const FilePath& path,
@@ -590,9 +578,8 @@ FilePath DownloadItem::GetFileNameToReportUser() const {
 }
 
 FilePath DownloadItem::GetUserVerifiedFilePath() const {
-  if (safety_state_ == DownloadItem::SAFE)
-    return GetTargetFilePath();
-  return full_path_;
+  return (safety_state_ == DownloadItem::SAFE) ?
+      GetTargetFilePath() : full_path_;
 }
 
 void DownloadItem::Init(bool start_timer) {
@@ -600,7 +587,7 @@ void DownloadItem::Init(bool start_timer) {
     target_name_ = full_path_.BaseName();
   if (start_timer)
     StartProgressTimer();
-  VLOG(20) << " " << __FUNCTION__ << "() " << DebugString(true);
+  VLOG(20) << __FUNCTION__ << "() " << DebugString(true);
 }
 
 // TODO(ahendrickson) -- Move |INTERRUPTED| from |IsCancelled()| to
@@ -622,27 +609,24 @@ bool DownloadItem::IsInterrupted() const {
 }
 
 bool DownloadItem::IsComplete() const {
-  return state() == COMPLETE;
+  return (state_ == COMPLETE);
 }
 
 std::string DownloadItem::DebugString(bool verbose) const {
-  std::string description =
-      base::StringPrintf("{ id_ = %d"
-                         " state = %s",
-                         id_,
-                         DebugDownloadStateString(state()));
+  std::string description = base::StringPrintf(
+      "{ id_ = %d state = %s", id_, DebugDownloadStateString(state()));
 
   if (verbose) {
     description += base::StringPrintf(
         " db_handle = %" PRId64
         " total_bytes = %" PRId64
-        " is_paused = " "%c"
-        " is_extension_install = " "%c"
-        " is_otr = " "%c"
-        " safety_state = " "%s"
-        " url = " "\"%s\""
+        " is_paused = %c"
+        " is_extension_install = %c"
+        " is_otr = %c"
+        " safety_state = %s"
+        " url = \"%s\""
         " target_name_ = \"%" PRFilePath "\""
-        " full_path = \"%" PRFilePath "\"",
+        " full_path = \"%" PRFilePath "\" }",
         db_handle(),
         total_bytes(),
         is_paused() ? 'T' : 'F',
@@ -653,10 +637,7 @@ std::string DownloadItem::DebugString(bool verbose) const {
         target_name_.value().c_str(),
         full_path().value().c_str());
   } else {
-    description += base::StringPrintf(" url = \"%s\"", url().spec().c_str());
+    description += " url = \"" + url().spec() + "\" }";
   }
-
-  description += " }";
-
   return description;
 }
diff --git a/chrome/browser/download/download_prefs.cc b/chrome/browser/download/download_prefs.cc
index d55db9a..e9e0b76 100644
--- a/chrome/browser/download/download_prefs.cc
+++ b/chrome/browser/download/download_prefs.cc
@@ -35,7 +35,8 @@ DownloadPrefs::DownloadPrefs(PrefService* prefs) : prefs_(prefs) {
 #elif defined(OS_WIN)
     FilePath path(UTF8ToWide(extensions[i]));
 #endif
-    if (!extensions[i].empty() && download_util::IsFileSafe(path))
+    if (!extensions[i].empty() &&
+        download_util::GetFileDangerLevel(path) == download_util::NotDangerous)
       auto_open_.insert(path.value());
   }
 }