Ensure proper extension when saving an HTML page.

When an HTML page has a title, it is used as the suggested file name in saving
the page. Then the file extension is guessed from the suggested file name.

This results in a wrong extension if the title contains a period ".".
This the source of Bug 10581.

Merely setting ignore_suggested_ext to true in win_util::SaveFileAsWithFilter
is not a good fix, because then we have an issue in saving a page without
a title, where the extension can be correctly derived from the suggested
file name.

This change solves the issue by appending ".htm" to the suggested file name
if the page content type is HTML and the suggested file name doesn't have a
proper extension.

BUG=10581

--------

patch by yuzo@google.com
review here: <http://codereview.chromium.org/115235>


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

4 files changed, 72 insertions, 24 deletions

diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index 9356a68..645346b 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -951,8 +951,8 @@ void SavePackage::SetShouldPromptUser(bool should_prompt) {
 }
 
 // static
-FilePath SavePackage::GetSuggestNameForSaveAs(PrefService* prefs,
-                                              const FilePath& name) {
+FilePath SavePackage::GetSuggestNameForSaveAs(
+    PrefService* prefs, const FilePath& name, bool can_save_as_complete) {
   // Check whether the preference has the preferred directory for saving file.
   // If not, initialize it with default directory.
   if (!prefs->IsPrefRegistered(prefs::kSaveFileDefaultDirectory)) {
@@ -979,7 +979,9 @@ FilePath SavePackage::GetSuggestNameForSaveAs(PrefService* prefs,
   DCHECK(!(*save_file_path).empty());
 
   // Ask user for getting final saving name.
-  std::wstring file_name = name.ToWStringHack();
+  FilePath name_with_proper_ext =
+      can_save_as_complete ? EnsureHtmlExtension(name) : name;
+  std::wstring file_name = name_with_proper_ext.ToWStringHack();
   // TODO(port): we need a version of ReplaceIllegalCharacters() that takes
   // FilePaths.
   file_util::ReplaceIllegalCharacters(&file_name, L' ');
@@ -990,22 +992,39 @@ FilePath SavePackage::GetSuggestNameForSaveAs(PrefService* prefs,
   return suggest_name;
 }
 
+FilePath SavePackage::EnsureHtmlExtension(const FilePath& name) {
+  // If the file name doesn't have an extension suitable for HTML files,
+  // append ".htm".
+  FilePath::StringType ext = file_util::GetFileExtensionFromPath(name);
+  std::string mime_type;
+  if (!net::GetMimeTypeFromExtension(ext, &mime_type) ||
+      !CanSaveAsComplete(mime_type)) {
+    return FilePath(name.value() + FILE_PATH_LITERAL(".htm"));
+  }
+  return name;
+}
+
 void SavePackage::GetSaveInfo() {
   // Use "Web Page, Complete" option as default choice of saving page.
   int file_type_index = 2;
   SelectFileDialog::FileTypeInfo file_type_info;
   FilePath::StringType default_extension;
-  FilePath title =
-      FilePath::FromWStringHack(UTF16ToWideHack(tab_contents_->GetTitle()));
-  FilePath suggested_path =
-      GetSuggestNameForSaveAs(tab_contents_->profile()->GetPrefs(), title);
 
   SavePackageParam* save_params =
       new SavePackageParam(tab_contents_->contents_mime_type());
 
+  bool can_save_as_complete =
+      CanSaveAsComplete(save_params->current_tab_mime_type);
+
+  FilePath title =
+      FilePath::FromWStringHack(UTF16ToWideHack(tab_contents_->GetTitle()));
+  FilePath suggested_path =
+      GetSuggestNameForSaveAs(tab_contents_->profile()->GetPrefs(), title,
+                              can_save_as_complete);
+
   // If the contents can not be saved as complete-HTML, do not show the
   // file filters.
-  if (CanSaveAsComplete(save_params->current_tab_mime_type)) {
+  if (can_save_as_complete) {
     file_type_info.extensions.resize(2);
     file_type_info.extensions[0].push_back(FILE_PATH_LITERAL("htm"));
     file_type_info.extension_description_overrides.push_back(
diff --git a/chrome/browser/download/save_package.h b/chrome/browser/download/save_package.h
index 698fa60..5c75936 100644
--- a/chrome/browser/download/save_package.h
+++ b/chrome/browser/download/save_package.h
@@ -156,12 +156,6 @@ class SavePackage : public base::RefCountedThreadSafe<SavePackage>,
   // web page.  This is available for testing.
   static void SetShouldPromptUser(bool should_prompt);
 
-  // Helper function for preparing suggested name for the SaveAs Dialog. The
-  // suggested name is composed of the default save path and the web document's
-  // title.
-  static FilePath GetSuggestNameForSaveAs(PrefService* prefs,
-                                          const FilePath& name);
-
   // Check whether we can do the saving page operation for the specified URL.
   static bool IsSavableURL(const GURL& url);
 
@@ -244,6 +238,16 @@ class SavePackage : public base::RefCountedThreadSafe<SavePackage>,
                             saved_failed_items_.size());
   }
 
+  // Helper function for preparing suggested name for the SaveAs Dialog. The
+  // suggested name is composed of the default save path and the web document's
+  // title.
+  static FilePath GetSuggestNameForSaveAs(
+      PrefService* prefs, const FilePath& name, bool can_save_as_complete);
+
+  // Ensure that the file name has a proper extension for HTML by adding ".htm"
+  // if necessary.
+  static FilePath EnsureHtmlExtension(const FilePath& name);
+
   typedef std::queue<SaveItem*> SaveItemQueue;
   // A queue for items we are about to start saving.
   SaveItemQueue waiting_item_queue_;
diff --git a/chrome/browser/download/save_package_unittest.cc b/chrome/browser/download/save_package_unittest.cc
index 7e19848..d828dcb 100644
--- a/chrome/browser/download/save_package_unittest.cc
+++ b/chrome/browser/download/save_package_unittest.cc
@@ -80,6 +80,10 @@ class SavePackageTest : public testing::Test {
                                           generated_name);
   }
 
+  FilePath EnsureHtmlExtension(const FilePath& name) {
+    return SavePackage::EnsureHtmlExtension(name);
+  }
+
  private:
   // SavePackage for successfully generating file name.
   scoped_refptr<SavePackage> save_package_success_;
@@ -173,3 +177,30 @@ TEST_F(SavePackageTest, TestLongSavePackageFilename) {
   EXPECT_TRUE(HasOrdinalNumber(filename2));
   EXPECT_NE(filename, filename2);
 }
+
+static const struct {
+  const FilePath::CharType* page_title;
+  const FilePath::CharType* expected_name;
+} kExtensionTestCases[] = {
+  // Extension is preserved if it is already proper for HTML.
+  {FPL("filename.html"), FPL("filename.html")},
+  {FPL("filename.HTML"), FPL("filename.HTML")},
+  {FPL("filename.htm"), FPL("filename.htm")},
+  // ".htm" is added if the extension is improper for HTML.
+  {FPL("hello.world"), FPL("hello.world.htm")},
+  {FPL("hello.txt"), FPL("hello.txt.htm")},
+  {FPL("is.html.good"), FPL("is.html.good.htm")},
+  // ".htm" is added if the name doesn't have an extension.
+  {FPL("helloworld"), FPL("helloworld.htm")},
+  {FPL("helloworld."), FPL("helloworld..htm")},
+};
+
+TEST_F(SavePackageTest, TestEnsureHtmlExtension) {
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kExtensionTestCases); ++i) {
+    FilePath original = FilePath(kExtensionTestCases[i].page_title);
+    FilePath expected = FilePath(kExtensionTestCases[i].expected_name);
+    FilePath actual = EnsureHtmlExtension(original);
+    EXPECT_EQ(expected.value(), actual.value()) << "Failed for page title: " <<
+        kExtensionTestCases[i].page_title;
+  }
+}
diff --git a/chrome/browser/download/save_page_uitest.cc b/chrome/browser/download/save_page_uitest.cc
index bac6b86..9172680 100644
--- a/chrome/browser/download/save_page_uitest.cc
+++ b/chrome/browser/download/save_page_uitest.cc
@@ -17,14 +17,7 @@
 
 const std::string kTestDir = "save_page";
 
-// We don't append an extension on linux.
-#if defined(OS_WIN)
 const std::string kAppendedExtension = ".htm";
-#elif defined(OS_LINUX)
-const std::string kAppendedExtension = "";
-#elif defined(OS_MACOSX)
-const std::string kAppendedExtension = ".html";
-#endif
 
 class SavePageTest : public UITest {
  protected:
@@ -168,14 +161,15 @@ TEST_F(SavePageTest, FilenameFromPageTitle) {
   EXPECT_TRUE(DieFileDie(dir, true));
 }
 
-// This tests that a webpage with the title "test.exe" is saved as "test.htm".
+// This tests that a webpage with the title "test.exe" is saved as
+// "test.exe.htm".
 // We probably don't care to handle this on Linux or Mac.
 #if defined(OS_WIN)
 TEST_F(SavePageTest, CleanFilenameFromPageTitle) {
   std::string file_name = "c.htm";
-  FilePath full_file_name = download_dir_.AppendASCII("test" +
+  FilePath full_file_name = download_dir_.AppendASCII("test.exe" +
                                                       kAppendedExtension);
-  FilePath dir = download_dir_.AppendASCII("test_files");
+  FilePath dir = download_dir_.AppendASCII("test.exe_files");
 
   GURL url = URLRequestMockHTTPJob::GetMockUrl(UTF8ToWide(kTestDir + "/" +
                                                file_name));