Ensure proper paths when saving pages with no title.

When saving a page with no title, such as a text file, attempt to
use a sane value for the save name by retrieving the last component
of the URL (if one exists).

This prevents the case where a page http://www.foo.com/a/path/name.txt
is saved as file "http www" with extension type "foo.com a path name.txt".

This change also makes the SavePackage code a little more flexible
and simpler to test.

BUG=none
TEST=Covered by unittest.

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

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

3 files changed, 100 insertions, 35 deletions

diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index 8d8873b..f36b388 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -953,46 +953,40 @@ void SavePackage::SetShouldPromptUser(bool should_prompt) {
   g_should_prompt_for_filename = should_prompt;
 }
 
-// static
-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)) {
-    FilePath default_save_path;
-    if (!prefs->IsPrefRegistered(prefs::kDownloadDefaultDirectory)) {
-      if (!PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS,
-                            &default_save_path)) {
-        NOTREACHED();
+// static.
+FilePath SavePackage::GetSuggestedNameForSaveAs(const FilePath& name,
+                                                bool can_save_as_complete) {
+  // If the name is a URL, try to use the last path component or if there is
+  // none, the domain as the file name.
+  FilePath name_with_proper_ext = name;
+  GURL url(WideToUTF8(name_with_proper_ext.ToWStringHack()));
+  if (url.is_valid()) {
+    std::string url_path;
+    std::vector<std::string> url_parts;
+    SplitString(url.path(), '/', &url_parts);
+    if (!url_parts.empty()) {
+      for (int i = static_cast<int>(url_parts.size()) - 1; i >= 0; --i) {
+        url_path = url_parts[i];
+        if (!url_path.empty())
+          break;
       }
-    } else {
-      StringPrefMember default_download_path;
-      default_download_path.Init(prefs::kDownloadDefaultDirectory,
-                                 prefs, NULL);
-      default_save_path = FilePath::FromWStringHack(
-                          default_download_path.GetValue());
     }
-    prefs->RegisterFilePathPref(prefs::kSaveFileDefaultDirectory,
-                                default_save_path);
+    if (url_path.empty())
+      url_path = url.host();
+    name_with_proper_ext = FilePath::FromWStringHack(UTF8ToWide(url_path));
   }
 
-  // Get the directory from preference.
-  StringPrefMember save_file_path;
-  save_file_path.Init(prefs::kSaveFileDefaultDirectory, prefs, NULL);
-  DCHECK(!(*save_file_path).empty());
-
   // Ask user for getting final saving name.
-  FilePath name_with_proper_ext =
-      can_save_as_complete ? EnsureHtmlExtension(name) : name;
+  if (can_save_as_complete)
+    name_with_proper_ext = EnsureHtmlExtension(name_with_proper_ext);
+
   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' ');
   TrimWhitespace(file_name, TRIM_ALL, &file_name);
-  FilePath suggest_name = FilePath::FromWStringHack(save_file_path.GetValue());
-  suggest_name = suggest_name.Append(FilePath::FromWStringHack(file_name));
 
-  return suggest_name;
+  return FilePath::FromWStringHack(file_name);
 }
 
 FilePath SavePackage::EnsureHtmlExtension(const FilePath& name) {
@@ -1008,6 +1002,38 @@ FilePath SavePackage::EnsureHtmlExtension(const FilePath& name) {
   return name;
 }
 
+// static.
+// Check whether the preference has the preferred directory for saving file. If
+// not, initialize it with default directory.
+FilePath SavePackage::GetSaveDirPreference(PrefService* prefs) {
+  DCHECK(prefs);
+
+  if (!prefs->IsPrefRegistered(prefs::kSaveFileDefaultDirectory)) {
+    FilePath default_save_path;
+    if (!prefs->IsPrefRegistered(prefs::kDownloadDefaultDirectory)) {
+      if (!PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS,
+                            &default_save_path)) {
+        NOTREACHED();
+      }
+    } else {
+      StringPrefMember default_download_path;
+      default_download_path.Init(prefs::kDownloadDefaultDirectory,
+                                 prefs, NULL);
+      default_save_path =
+          FilePath::FromWStringHack(default_download_path.GetValue());
+    }
+    prefs->RegisterFilePathPref(prefs::kSaveFileDefaultDirectory,
+                                default_save_path);
+  }
+
+  // Get the directory from preference.
+  StringPrefMember save_file_path;
+  save_file_path.Init(prefs::kSaveFileDefaultDirectory, prefs, NULL);
+  DCHECK(!(*save_file_path).empty());
+
+  return FilePath::FromWStringHack(*save_file_path);
+}
+
 void SavePackage::GetSaveInfo() {
   // Use "Web Page, Complete" option as default choice of saving page.
   int file_type_index = 2;
@@ -1022,9 +1048,10 @@ void SavePackage::GetSaveInfo() {
 
   FilePath title =
       FilePath::FromWStringHack(UTF16ToWideHack(tab_contents_->GetTitle()));
+  FilePath save_dir =
+      GetSaveDirPreference(tab_contents_->profile()->GetPrefs());
   FilePath suggested_path =
-      GetSuggestNameForSaveAs(tab_contents_->profile()->GetPrefs(), title,
-                              can_save_as_complete);
+      save_dir.Append(GetSuggestedNameForSaveAs(title, can_save_as_complete));
 
   // If the contents can not be saved as complete-HTML, do not show the
   // file filters.
diff --git a/chrome/browser/download/save_package.h b/chrome/browser/download/save_package.h
index 8d120602..c448bef 100644
--- a/chrome/browser/download/save_package.h
+++ b/chrome/browser/download/save_package.h
@@ -236,11 +236,13 @@ class SavePackage : public base::RefCountedThreadSafe<SavePackage>,
                             saved_failed_items_.size());
   }
 
+  // Retrieve the preference for the directory to save pages to.
+  static FilePath GetSaveDirPreference(PrefService* prefs);
+
   // 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);
+  // suggested name is determined by the web document's title.
+  static FilePath GetSuggestedNameForSaveAs(const FilePath& name,
+                                            bool can_save_as_complete);
 
   // Ensure that the file name has a proper extension for HTML by adding ".htm"
   // if necessary.
@@ -306,6 +308,7 @@ class SavePackage : public base::RefCountedThreadSafe<SavePackage>,
   scoped_refptr<SelectFileDialog> select_file_dialog_;
 
   friend class SavePackageTest;
+
   DISALLOW_COPY_AND_ASSIGN(SavePackage);
 };
 
diff --git a/chrome/browser/download/save_package_unittest.cc b/chrome/browser/download/save_package_unittest.cc
index 6522a80..d13bc61 100644
--- a/chrome/browser/download/save_package_unittest.cc
+++ b/chrome/browser/download/save_package_unittest.cc
@@ -92,6 +92,11 @@ class SavePackageTest : public testing::Test {
     return SavePackage::EnsureHtmlExtension(name);
   }
 
+  FilePath GetSuggestedNameForSaveAs(const FilePath& title,
+                                     bool ensure_html_extension) {
+    return SavePackage::GetSuggestedNameForSaveAs(title, ensure_html_extension);
+  }
+
  private:
   // SavePackage for successfully generating file name.
   scoped_refptr<SavePackage> save_package_success_;
@@ -213,3 +218,33 @@ TEST_F(SavePackageTest, TestEnsureHtmlExtension) {
         kExtensionTestCases[i].page_title;
   }
 }
+
+// Test that the suggested names generated by SavePackage are reasonable:
+// If the name is a URL, retrieve only the path component since the path name
+// generation code will turn the entire URL into the file name leading to bad
+// extension names. For example, a page with no title and a URL:
+// http://www.foo.com/a/path/name.txt will turn into file:
+// "http www.foo.com a path name.txt", when we want to save it as "name.txt".
+
+static const struct {
+  const FilePath::CharType* page_title;
+  const FilePath::CharType* expected_name;
+  bool ensure_html_extension;
+} kSuggestedSaveNames[] = {
+  {FPL("A page title"), FPL("A page title") FPL_HTML_EXTENSION, true},
+  {FPL("A page title with.ext"), FPL("A page title with.ext"), false},
+  {FPL("http://www.foo.com/path/title.txt"), FPL("title.txt"), false},
+  {FPL("http://www.foo.com/path/"), FPL("path"), false},
+  {FPL("http://www.foo.com/"), FPL("www.foo.com"), false},
+};
+
+TEST_F(SavePackageTest, TestSuggestedSaveNames) {
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kSuggestedSaveNames); ++i) {
+    FilePath title = FilePath(kSuggestedSaveNames[i].page_title);
+    FilePath save_name =
+        GetSuggestedNameForSaveAs(title,
+                                  kSuggestedSaveNames[i].ensure_html_extension);
+    EXPECT_EQ(save_name.value(), kSuggestedSaveNames[i].expected_name);
+  }
+}
+