From 6cade219e58c8806bdbd02316d78f49c8fa97996 Mon Sep 17 00:00:00 2001 From: "finnur@google.com" Date: Wed, 3 Dec 2008 00:32:22 +0000 Subject: 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 --- chrome/browser/download/download_manager.cc | 35 +++++++-------- chrome/browser/download/save_package.cc | 9 ++-- chrome/browser/views/shell_dialogs.cc | 3 +- chrome/common/win_util.cc | 68 +++++++++++++++++++++++------ chrome/common/win_util.h | 13 +++--- 5 files changed, 85 insertions(+), 43 deletions(-) (limited to 'chrome') 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, ¶m->saved_main_file_path)) + suggest_name, + filter, + L"htm", + &index, + ¶m->saved_main_file_path)) return false; } else { if (!win_util::SaveFileAs(container_hwnd, suggest_name, diff --git a/chrome/browser/views/shell_dialogs.cc b/chrome/browser/views/shell_dialogs.cc index a9e8c53..e38d81f 100644 --- a/chrome/browser/views/shell_dialogs.cc +++ b/chrome/browser/views/shell_dialogs.cc @@ -291,10 +291,9 @@ void SelectFileDialogImpl::ExecuteSelectFile( if (type == SELECT_FOLDER) { success = RunSelectFolderDialog(title, run_state.owner, &path); } else if (type == SELECT_SAVEAS_FILE) { - const wchar_t* filter_string = filter.empty() ? NULL : filter.c_str(); unsigned index = 0; success = win_util::SaveFileAsWithFilter(run_state.owner, default_path, - filter_string, default_extension, &index, &path); + filter, default_extension, &index, &path); DisableOwner(run_state.owner); } else if (type == SELECT_OPEN_FILE) { success = RunOpenFileDialog(title, filter, run_state.owner, &path); diff --git a/chrome/common/win_util.cc b/chrome/common/win_util.cc index 676a0b8..e9a4144 100644 --- a/chrome/common/win_util.cc +++ b/chrome/common/win_util.cc @@ -20,6 +20,7 @@ #include "base/string_util.h" #include "base/win_util.h" #include "chrome/common/l10n_util.h" +#include "net/base/mime_util.h" #include "generated_resources.h" // Ensure that we pick up this link library. @@ -361,7 +362,7 @@ bool SaveFileAs(HWND owner, unsigned index = 1; return SaveFileAsWithFilter(owner, suggested_name, - filter.c_str(), + filter, L"", &index, final_name); @@ -369,11 +370,14 @@ bool SaveFileAs(HWND owner, bool SaveFileAsWithFilter(HWND owner, const std::wstring& suggested_name, - const wchar_t* filter, + const std::wstring& filter, const std::wstring& def_ext, unsigned* index, std::wstring* final_name) { DCHECK(final_name); + // Having an empty filter makes for a bad user experience. We should always + // specify a filter when saving. + DCHECK(!filter.empty()); std::wstring file_part = file_util::GetFilenameFromPath(suggested_name); // The size of the in/out buffer in number of characters we pass to win32 @@ -394,7 +398,7 @@ bool SaveFileAsWithFilter(HWND owner, save_as.hwndOwner = owner; save_as.hInstance = NULL; - save_as.lpstrFilter = filter; + save_as.lpstrFilter = filter.empty() ? NULL : filter.c_str(); save_as.lpstrCustomFilter = NULL; save_as.nMaxCustFilter = 0; @@ -431,16 +435,53 @@ bool SaveFileAsWithFilter(HWND owner, final_name->assign(save_as.lpstrFile); *index = save_as.nFilterIndex; - std::wstring file_ext = file_util::GetFileExtensionFromPath(suggested_name); - if (save_as.nFileExtension == 0) { - // No extension is specified. Append the default extension. - final_name->append(L"."); - final_name->append(file_ext); - } else if (save_as.nFileExtension == wcslen(save_as.lpstrFile)) { - // The path ends with a ".". This is not supported on windows and since - // we don't use a windows API to create the file, it will make the file - // impossible to open. - final_name->resize(final_name->size() - 1); + // Figure out what filter got selected from the vector with embedded nulls. + // NOTE: The filter contains a string with embedded nulls, such as: + // JPG Image\0*.jpg\0All files\0*.*\0\0 + // The filter index is 1-based index for which pair got selected. So, using + // the example above, if the first index was selected we need to skip 1 + // instance of null to get to "*.jpg". + std::vector filters; + if (!filter.empty() && save_as.nFilterIndex > 0) + SplitString(filter, '\0', &filters); + std::wstring filter_selected; + if (!filters.empty()) + filter_selected = filters[(2 * (save_as.nFilterIndex - 1)) + 1]; + + // Get the extension that was suggested to the user (when the Save As dialog + // was opened) and the extension the user ended up selecting (|final_ext|). + std::wstring suggested_ext = + file_util::GetFileExtensionFromPath(suggested_name); + std::wstring final_ext = + file_util::GetFileExtensionFromPath(*final_name); + // If we can't get the extension from the suggested_name, we use the default + // extension passed in. This is to cover cases like when saving a web page, + // where we get passed in a name without an extension and a default extension + // along with it. + if (suggested_ext.empty()) + suggested_ext = def_ext; + + if (filter_selected.empty() || filter_selected == L"*.*") { + // If the user selects 'All files' we respect any extension given to us from + // the File Save dialog. We also strip any trailing dots, which matches + // Windows Explorer and is needed because Windows doesn't allow filenames + // to have trailing dots. The GetSaveFileName dialog will not return a + // string with only one or more dots. + size_t index = final_name->find_last_not_of(L'.'); + if (index < final_name->size() - 1) + *final_name = final_name->substr(0, index + 1); + } else { + // User selected a specific filter (not *.*) so we need to check if the + // extension provided has the same mime type. If it doesn't we append the + // extension. + std::string suggested_mime_type, selected_mime_type; + if (suggested_ext != final_ext && + (!net::GetMimeTypeFromExtension(suggested_ext, &suggested_mime_type) || + !net::GetMimeTypeFromExtension(final_ext, &selected_mime_type) || + suggested_mime_type != selected_mime_type)) { + final_name->append(L"."); + final_name->append(suggested_ext); + } } return true; @@ -820,4 +861,3 @@ ChromeFont GetWindowTitleFont() { } } // namespace win_util - diff --git a/chrome/common/win_util.h b/chrome/common/win_util.h index 5b0fc58..030d48a 100644 --- a/chrome/common/win_util.h +++ b/chrome/common/win_util.h @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef CHROME_COMMON_WIN_UTIL_H__ -#define CHROME_COMMON_WIN_UTIL_H__ +#ifndef CHROME_COMMON_WIN_UTIL_H_ +#define CHROME_COMMON_WIN_UTIL_H_ #include @@ -52,7 +52,7 @@ class CoMemReleaser { private: T* mem_ptr_; - DISALLOW_EVIL_CONSTRUCTORS(CoMemReleaser); + DISALLOW_COPY_AND_ASSIGN(CoMemReleaser); }; // Initializes COM in the constructor, and uninitializes COM in the @@ -68,7 +68,7 @@ class ScopedCOMInitializer { } private: - DISALLOW_EVIL_CONSTRUCTORS(ScopedCOMInitializer); + DISALLOW_COPY_AND_ASSIGN(ScopedCOMInitializer); }; // Creates a string interpretation of the time of day represented by the given @@ -159,7 +159,7 @@ bool SaveFileAs(HWND owner, // extension. bool SaveFileAsWithFilter(HWND owner, const std::wstring& suggested_name, - const wchar_t* filter, + const std::wstring& filter, const std::wstring& def_ext, unsigned* index, std::wstring* final_name); @@ -239,5 +239,4 @@ ChromeFont GetWindowTitleFont(); } // namespace win_util -#endif // WIN_COMMON_WIN_UTIL_H__ - +#endif // CHROME_COMMON_WIN_UTIL_H_ -- cgit v1.1