Fix issue 4829: No file extensions listed when saving images

We were not supplying a filter list for the Save As dialog. 

Now that we do, the user can select between, for example, say *.jpg and *.* as filters and we behave as follows:

With a *.jpg filter active, user enters foo and we save the file as: foo.jpg
With a *.jpg filter active, user enters foo. and we save the file as: foo..jpg (which is consistent with IE)
With a *.jpg filter active, user enters foo.jpg, we save the file as: foo.jpg
With a *.jpg filter active, user enters foo.jpeg, we save the file as: foo.jpeg (not foo.jpg or foo.jpeg.jpg)

With a *.* filter active, we respect whatever user enters as extension, except if the filename contains one or more trailing dots then we strip out all those trailing dots.

Also test filenames created when saving web pages, as opposed to images (should work as before).

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

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

2 files changed, 24 insertions, 20 deletions

diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 6780581..6e46896 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -59,8 +59,8 @@ static const int kUpdateTimeMs = 1000;
 
 // Our download table ID starts at 1, so we use 0 to represent a download that
 // has started, but has not yet had its data persisted in the table. We use fake
-// database handles in incognito mode starting at -1 and progressly getting more
-// negative.
+// database handles in incognito mode starting at -1 and progressively getting
+// more negative.
 static const int kUninitializedHandle = 0;
 
 // Appends the passed the number between parenthesis the path before the
@@ -198,7 +198,7 @@ void DownloadItem::Update(int64 bytes_so_far) {
   UpdateObservers();
 }
 
-// Triggered by a user action
+// Triggered by a user action.
 void DownloadItem::Cancel(bool update_history) {
   if (state_ != IN_PROGRESS) {
     // Small downloads might be complete before this method has a chance to run.
@@ -224,7 +224,7 @@ void DownloadItem::Remove(bool delete_on_disk) {
   if (delete_on_disk)
     manager_->DeleteDownload(full_path_);
   manager_->RemoveDownload(db_handle_);
-  // We are now deleted.
+  // We have now been deleted.
 }
 
 void DownloadItem::StartProgressTimer() {
@@ -307,7 +307,7 @@ void DownloadManager::RegisterUserPrefs(PrefService* prefs) {
   // the user if he really wants it on an unsafe place such as the desktop.
 
   if (!prefs->GetBoolean(prefs::kDownloadDirUpgraded)) {
-    std::wstring current_download_dir = 
+    std::wstring current_download_dir =
         prefs->GetString(prefs::kDownloadDefaultDirectory);
     if (DownloadPathIsDangerous(current_download_dir)) {
       prefs->SetString(prefs::kDownloadDefaultDirectory,
@@ -571,7 +571,7 @@ void DownloadManager::CheckIfSuggestedPathExists(DownloadCreateInfo* info) {
 
   info->path_uniquifier = GetUniquePathNumber(info->suggested_path);
 
-  // If the download is deemmed dangerous, we'll use a temporary name for it.
+  // If the download is deemed dangerous, we'll use a temporary name for it.
   if (info->is_dangerous) {
     info->original_name = file_util::GetFilenameFromPath(info->suggested_path);
     // Create a temporary file to hold the file until the user approves its
@@ -615,11 +615,12 @@ void DownloadManager::OnPathExistenceAvailable(DownloadCreateInfo* info) {
 
     TabContents* contents = tab_util::GetTabContentsByID(
         info->render_process_id, info->render_view_id);
+    std::wstring filter = win_util::GetFileFilterFromPath(info->suggested_path);
     HWND owning_hwnd =
         contents ? GetAncestor(contents->GetContainerHWND(), GA_ROOT) : NULL;
     select_file_dialog_->SelectFile(SelectFileDialog::SELECT_SAVEAS_FILE,
                                     std::wstring(), info->suggested_path,
-                                    std::wstring(), std::wstring(),
+                                    filter, std::wstring(),
                                     owning_hwnd, info);
   } else {
     // No prompting for download, just continue with the suggested name.
@@ -677,7 +678,7 @@ void DownloadManager::ContinueStartDownload(DownloadCreateInfo* info,
     OnCreateDownloadEntryComplete(*info, fake_db_handle--);
   } else {
     // Update the history system with the new download.
-    // FIXME(acw|paulg) see bug 958058. EXPLICIT_ACCESS below is wrong.
+    // FIXME(paulg) see bug 958058. EXPLICIT_ACCESS below is wrong.
     HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
     if (hs) {
       hs->CreateDownload(
@@ -696,7 +697,7 @@ void DownloadManager::UpdateHistoryForDownload(DownloadItem* download) {
   if (download->db_handle() <= kUninitializedHandle)
     return;
 
-  // FIXME(acw|paulg) see bug 958058. EXPLICIT_ACCESS below is wrong.
+  // FIXME(paulg) see bug 958058. EXPLICIT_ACCESS below is wrong.
   HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
   if (hs) {
     hs->UpdateDownload(download->received_bytes(),
@@ -707,7 +708,7 @@ void DownloadManager::UpdateHistoryForDownload(DownloadItem* download) {
 
 void DownloadManager::RemoveDownloadFromHistory(DownloadItem* download) {
   DCHECK(download);
-  // FIXME(acw|paulg) see bug 958058. EXPLICIT_ACCESS below is wrong.
+  // FIXME(paulg) see bug 958058. EXPLICIT_ACCESS below is wrong.
   HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
   if (download->db_handle() > kUninitializedHandle && hs)
     hs->RemoveDownload(download->db_handle());
@@ -715,7 +716,7 @@ void DownloadManager::RemoveDownloadFromHistory(DownloadItem* download) {
 
 void DownloadManager::RemoveDownloadsFromHistoryBetween(const Time remove_begin,
                                                         const Time remove_end) {
-  // FIXME(acw|paulg) see bug 958058. EXPLICIT_ACCESS below is wrong.
+  // FIXME(paulg) see bug 958058. EXPLICIT_ACCESS below is wrong.
   HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
   if (hs)
     hs->RemoveDownloadsBetween(remove_begin, remove_end);
@@ -770,7 +771,7 @@ void DownloadManager::DownloadFinished(int32 download_id, int64 size) {
   }
 
   if (download->safety_state() == DownloadItem::DANGEROUS_BUT_VALIDATED) {
-    // We first need to rename the donwloaded file from its temporary name to
+    // We first need to rename the downloaded file from its temporary name to
     // its final name before we can continue.
     file_loop_->PostTask(FROM_HERE,
         NewRunnableMethod(
@@ -823,7 +824,7 @@ void DownloadManager::ProceedWithFinishedDangerousDownload(
   } else {
     NOTREACHED();
   }
-  
+
   ui_loop_->PostTask(FROM_HERE,
       NewRunnableMethod(this, &DownloadManager::DangerousDownloadRenamed,
                         download_handle, success, new_path, uniquifier));
@@ -855,7 +856,7 @@ void DownloadManager::DangerousDownloadRenamed(int64 download_handle,
 
 // static
 // We have to tell the ResourceDispatcherHost to cancel the download from this
-// thread, since we can't forward tasks from the file thread to the io thread
+// thread, since we can't forward tasks from the file thread to the IO thread
 // reliably (crash on shutdown race condition).
 void DownloadManager::CancelDownloadRequest(int render_process_id,
                                             int request_id) {
@@ -946,7 +947,7 @@ void DownloadManager::RenameDownload(DownloadItem* download,
   if (download->db_handle() <= kUninitializedHandle)
     return;
 
-  // FIXME(acw|paulg) see bug 958058. EXPLICIT_ACCESS below is wrong.
+  // FIXME(paulg) see bug 958058. EXPLICIT_ACCESS below is wrong.
   HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
   if (hs)
     hs->UpdateDownloadPath(new_path, download->db_handle());
@@ -1089,7 +1090,7 @@ void DownloadManager::GenerateExtension(const std::wstring& file_name,
   if (extension.empty()) {
     net::GetPreferredExtensionForMimeType(mime_type, &extension);
   } else {
-    // Append entension generated from the mime type if:
+    // Append extension generated from the mime type if:
     // 1. New extension is not ".txt"
     // 2. New extension is not the same as the already existing extension.
     // 3. New extension is not executable. This action mitigates the case when
@@ -1346,7 +1347,7 @@ void DownloadManager::OnSearchComplete(HistoryService::Handle handle,
   requestor->SetDownloads(searched_downloads);
 }
 
-// Clears the last download path, used to initialize "save as" dialogs.  
+// Clears the last download path, used to initialize "save as" dialogs.
 void DownloadManager::ClearLastDownloadPath() {
   last_download_path_.clear();
 }
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index 52f9485..9eaf7fc 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -920,7 +920,7 @@ bool SavePackage::GetSaveInfo(const std::wstring& suggest_name,
   // Use "Web Page, Complete" option as default choice of saving page.
   unsigned index = 2;
 
-  // If the conetnts can not be saved as complete-HTML, do not show the
+  // If the contents can not be saved as complete-HTML, do not show the
   // file filters.
   if (CanSaveAsComplete(param->current_tab_mime_type)) {
     // Create filter string.
@@ -930,8 +930,11 @@ bool SavePackage::GetSaveInfo(const std::wstring& suggest_name,
     filter[filter.size() - 2] = L'\0';
 
     if (!win_util::SaveFileAsWithFilter(container_hwnd,
-        suggest_name, filter.c_str(), L"htm",
-        &index, &param->saved_main_file_path))
+                                        suggest_name,
+                                        filter,
+                                        L"htm",
+                                        &index,
+                                        &param->saved_main_file_path))
       return false;
   } else {
     if (!win_util::SaveFileAs(container_hwnd, suggest_name,