Commit patch set from http://codereview.chromium.org/149796

(see discussion and history there)

BUG=10876
TEST=FilePathTest.MatchesExtension.CompareIgnoreCase


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

4 files changed, 58 insertions, 40 deletions

diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index e2f998a..effcf05 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -526,8 +526,8 @@ bool DownloadManager::Init(Profile* profile) {
   std::vector<std::wstring> extensions;
   SplitString(extensions_to_open, L':', &extensions);
   for (size_t i = 0; i < extensions.size(); ++i) {
-    if (!extensions[i].empty() && !IsExecutable(
-        FilePath::FromWStringHack(extensions[i]).value()))
+    if (!extensions[i].empty() && !IsExecutableFile(
+        FilePath::FromWStringHack(extensions[i])))
       auto_open_.insert(FilePath::FromWStringHack(extensions[i]).value());
   }
 
@@ -857,19 +857,13 @@ void DownloadManager::ContinueDownloadFinished(DownloadItem* download) {
   if (it != dangerous_finished_.end())
     dangerous_finished_.erase(it);
 
-  // Open the download if the user or user prefs indicate it should be.
-  FilePath::StringType extension = download->full_path().Extension();
-  // Drop the leading period. (The auto-open list is period-less.)
-  if (extension.size() > 0)
-    extension = extension.substr(1);
-
   // Handle chrome extensions explicitly and skip the shell execute.
   if (IsExtensionInstall(download)) {
     OpenChromeExtension(download->full_path(), download->url(),
                         download->referrer_url());
     download->set_auto_opened(true);
   } else if (download->open_when_complete() ||
-             ShouldOpenFileExtension(extension)) {
+             ShouldOpenFileBasedOnExtension(download->full_path())) {
     OpenDownloadInShell(download, NULL);
     download->set_auto_opened(true);
   }
@@ -878,6 +872,7 @@ void DownloadManager::ContinueDownloadFinished(DownloadItem* download) {
   // state to complete but did not notify).
   download->UpdateObservers();
 }
+
 // Called on the file thread.  Renames the downloaded file to its original name.
 void DownloadManager::ProceedWithFinishedDangerousDownload(
     int64 download_handle,
@@ -1004,11 +999,7 @@ void DownloadManager::OnPauseDownloadRequest(ResourceDispatcherHost* rdh,
 
 bool DownloadManager::IsDangerous(const FilePath& file_name) {
   // TODO(jcampan): Improve me.
-  FilePath::StringType extension = file_name.Extension();
-  // Drop the leading period.
-  if (extension.size() > 0)
-    extension = extension.substr(1);
-  return IsExecutable(extension);
+  return IsExecutableFile(file_name);
 }
 
 void DownloadManager::RenameDownload(DownloadItem* download,
@@ -1155,7 +1146,7 @@ void DownloadManager::GenerateExtension(
     return;
   }
 
-  if (IsExecutable(extension) && !IsExecutableMimeType(mime_type)) {
+  if (IsExecutableExtension(extension) && !IsExecutableMimeType(mime_type)) {
     // We want to be careful about executable extensions.  The worry here is
     // that a trusted web site could be tricked into dropping an executable file
     // on the user's filesystem.
@@ -1183,7 +1174,7 @@ void DownloadManager::GenerateExtension(
     if (net::GetPreferredExtensionForMimeType(mime_type, &append_extension)) {
       if (append_extension != FILE_PATH_LITERAL("txt") &&
           append_extension != extension &&
-          !IsExecutable(append_extension) &&
+          !IsExecutableExtension(append_extension) &&
           !(append_extension == FILE_PATH_LITERAL("gz") &&
             extension == FILE_PATH_LITERAL("tgz")) &&
           (append_extension != FILE_PATH_LITERAL("tar") ||
@@ -1282,21 +1273,32 @@ void DownloadManager::OpenDownloadInShell(const DownloadItem* download,
 #endif
 }
 
-void DownloadManager::OpenFilesOfExtension(
-    const FilePath::StringType& extension, bool open) {
-  if (open && !IsExecutable(extension))
+void DownloadManager::OpenFilesBasedOnExtension(
+    const FilePath& path, bool open) {
+  FilePath::StringType extension = path.Extension();
+  if (extension.empty())
+    return;
+  DCHECK(extension[0] == FilePath::kExtensionSeparator);
+  extension.erase(0, 1);
+  if (open && !IsExecutableExtension(extension))
     auto_open_.insert(extension);
   else
     auto_open_.erase(extension);
   SaveAutoOpens();
 }
 
-bool DownloadManager::ShouldOpenFileExtension(
-    const FilePath::StringType& extension) {
+bool DownloadManager::ShouldOpenFileBasedOnExtension(
+    const FilePath& path) const {
   // Special-case Chrome extensions as always-open.
-  if (!IsExecutable(extension) &&
-      (auto_open_.find(extension) != auto_open_.end() ||
-       Extension::IsExtension(FilePath(extension))))
+  FilePath::StringType extension = path.Extension();
+  if (extension.empty())
+    return false;
+  if (IsExecutableExtension(extension))
+    return false;
+  DCHECK(extension[0] == FilePath::kExtensionSeparator);
+  extension.erase(0, 1);
+  if (auto_open_.find(extension) != auto_open_.end() ||
+      Extension::IsExtension(path))
     return true;
   return false;
 }
@@ -1333,7 +1335,14 @@ bool DownloadManager::IsExecutableMimeType(const std::string& mime_type) {
   return net::MatchesMimeType("application/*", mime_type);
 }
 
-bool DownloadManager::IsExecutable(const FilePath::StringType& extension) {
+bool DownloadManager::IsExecutableFile(const FilePath& path) const {
+  return IsExecutableExtension(path.Extension());
+}
+
+bool DownloadManager::IsExecutableExtension(
+    const FilePath::StringType& extension) const {
+  if (extension.empty())
+    return false;
   if (!IsStringASCII(extension))
     return false;
 #if defined(OS_WIN)
@@ -1343,6 +1352,10 @@ bool DownloadManager::IsExecutable(const FilePath::StringType& extension) {
 #endif
   StringToLowerASCII(&ascii_extension);
 
+  // Strip out leading dot if it's still there
+  if (ascii_extension[0] == FilePath::kExtensionSeparator)
+    ascii_extension.erase(0, 1);
+
   return exe_types_.find(ascii_extension) != exe_types_.end();
 }
 
@@ -1359,7 +1372,7 @@ void DownloadManager::SaveAutoOpens() {
   PrefService* prefs = profile_->GetPrefs();
   if (prefs) {
     FilePath::StringType extensions;
-    for (std::set<FilePath::StringType>::iterator it = auto_open_.begin();
+    for (AutoOpenSet::iterator it = auto_open_.begin();
          it != auto_open_.end(); ++it) {
       extensions += *it + FILE_PATH_LITERAL(":");
     }
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 34a2e42..5f613a8 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -419,16 +419,19 @@ class DownloadManager : public base::RefCountedThreadSafe<DownloadManager>,
   // Registers this file extension for automatic opening upon download
   // completion if 'open' is true, or prevents the extension from automatic
   // opening if 'open' is false.
-  void OpenFilesOfExtension(const FilePath::StringType& extension, bool open);
+  void OpenFilesBasedOnExtension(const FilePath& path, bool open);
 
   // Tests if a file type should be opened automatically.
-  bool ShouldOpenFileExtension(const FilePath::StringType& extension);
+  bool ShouldOpenFileBasedOnExtension(const FilePath& path) const;
 
   // Tests if we think the server means for this mime_type to be executable.
   static bool IsExecutableMimeType(const std::string& mime_type);
 
+  // Tests if a file is considered executable, based on its type.
+  bool IsExecutableFile(const FilePath& path) const;
+
   // Tests if a file type is considered executable.
-  bool IsExecutable(const FilePath::StringType& extension);
+  bool IsExecutableExtension(const FilePath::StringType& extension) const;
 
   // Resets the automatic open preference.
   void ResetAutoOpenFiles();
@@ -605,7 +608,14 @@ class DownloadManager : public base::RefCountedThreadSafe<DownloadManager>,
   FilePath last_download_path_;
 
   // Set of file extensions to open at download completion.
-  std::set<FilePath::StringType> auto_open_;
+  struct AutoOpenCompareFunctor {
+    inline bool operator()(const FilePath::StringType& a,
+                           const FilePath::StringType& b) const {
+      return FilePath::CompareLessIgnoreCase(a, b);
+    }
+  };
+  typedef std::set<FilePath::StringType, AutoOpenCompareFunctor> AutoOpenSet;
+  AutoOpenSet auto_open_;
 
   // Set of file extensions that are executables and shouldn't be auto opened.
   std::set<std::string> exe_types_;
diff --git a/chrome/browser/download/download_shelf.cc b/chrome/browser/download/download_shelf.cc
index b3ccd28..1ab4e77 100644
--- a/chrome/browser/download/download_shelf.cc
+++ b/chrome/browser/download/download_shelf.cc
@@ -32,9 +32,8 @@ bool DownloadShelfContextMenu::ItemIsChecked(int id) const {
       return download_->open_when_complete();
     }
     case ALWAYS_OPEN_TYPE: {
-      const FilePath::StringType extension =
-          file_util::GetFileExtensionFromPath(download_->full_path());
-      return download_->manager()->ShouldOpenFileExtension(extension);
+      return download_->manager()->ShouldOpenFileBasedOnExtension(
+          download_->full_path());
     }
     case TOGGLE_PAUSE: {
       return download_->is_paused();
@@ -96,10 +95,8 @@ void DownloadShelfContextMenu::ExecuteItemCommand(int id) {
       download_util::OpenDownload(download_);
       break;
     case ALWAYS_OPEN_TYPE: {
-      const FilePath::StringType extension =
-          file_util::GetFileExtensionFromPath(download_->full_path());
-      download_->manager()->OpenFilesOfExtension(
-          extension, !ItemIsChecked(ALWAYS_OPEN_TYPE));
+      download_->manager()->OpenFilesBasedOnExtension(
+          download_->full_path(), !ItemIsChecked(ALWAYS_OPEN_TYPE));
       break;
     }
     case CANCEL:
diff --git a/chrome/browser/download/download_util.cc b/chrome/browser/download/download_util.cc
index e7650d5..786271d 100644
--- a/chrome/browser/download/download_util.cc
+++ b/chrome/browser/download/download_util.cc
@@ -53,9 +53,7 @@ bool CanOpenDownload(DownloadItem* download) {
   if (!download->original_name().value().empty())
     file_to_use = download->original_name();
 
-  const FilePath::StringType extension =
-      file_util::GetFileExtensionFromPath(file_to_use);
-  return !download->manager()->IsExecutable(extension);
+  return !download->manager()->IsExecutableFile(file_to_use);
 }
 
 void OpenDownload(DownloadItem* download) {