Fix for bug 10876 that resulted in some refactoring:

The bug originates from extensions being treated case sensitive on Windows and Mac OSX, where they shouldn't be. 

Therefore I added generic static methods to FilePath to compare strings in the same way the file system does, and changed the relevant parts of the code to make use of them.
I tested the methods under Windows and Mac OS X. I also wrote a basic version for Linux/Posix that behaves the same way as the original code, so there should at least be no regression.

Also, while fixing this I found some confusion in the code about whether extensions are used with or without leading dot. For this reason I changed some functions that were taking an extension as parameter to instead take the whole file path. This makes calling these functions easier and the caller doesn't need to know whether the extension is supposed to be with or without dot.

In the same vein, I split DownloadManager::IsExecutable into IsExecutableFile, where one again passes in the whole file and doesn't have to worry about getting the extension right, and IsExecutableExtension, which corresponds to the original functionality. Ideally only the former method should be public, but that again would have required further code scrubbing that was (even more) outside of the original bug fix.

Finally, fixed a wrong comment in the file path tests.


BUG=10876
TEST=FilePathTest.MatchesExtension, .CompareIgnoreCase

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@30323 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) {