Adding a unit test for the AppendExtensionIfNeeded part

of the SaveFileAsWithFilter function (after splitting
it out to a separate function).

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

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

3 files changed, 81 insertions, 14 deletions

diff --git a/chrome/common/win_util.cc b/chrome/common/win_util.cc
index 44d3769..4cfec85 100644
--- a/chrome/common/win_util.cc
+++ b/chrome/common/win_util.cc
@@ -449,11 +449,9 @@ bool SaveFileAsWithFilter(HWND owner,
     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|).
+  // was opened).
   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
@@ -461,30 +459,43 @@ bool SaveFileAsWithFilter(HWND owner,
   if (suggested_ext.empty())
     suggested_ext = def_ext;
 
+  *final_name =
+      AppendExtensionIfNeeded(*final_name, filter_selected, suggested_ext);
+  return true;
+}
+
+std::wstring AppendExtensionIfNeeded(const std::wstring& filename,
+                                     const std::wstring& filter_selected,
+                                     const std::wstring& suggested_ext) {
+  std::wstring return_value = filename;
+
+  // Get the extension the user ended up selecting.
+  std::wstring selected_ext = file_util::GetFileExtensionFromPath(filename);
+
   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);
+    size_t index = return_value.find_last_not_of(L'.');
+    if (index < return_value.size() - 1)
+      return_value.resize(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 &&
+    if (suggested_ext != selected_ext &&
         (!net::GetMimeTypeFromExtension(suggested_ext, &suggested_mime_type) ||
-         !net::GetMimeTypeFromExtension(final_ext, &selected_mime_type) ||
+         !net::GetMimeTypeFromExtension(selected_ext, &selected_mime_type) ||
          suggested_mime_type != selected_mime_type)) {
-      final_name->append(L".");
-      final_name->append(suggested_ext);
+      return_value.append(L".");
+      return_value.append(suggested_ext);
     }
   }
 
-  return true;
+  return return_value;
 }
 
 // Adjust the window to fit, returning true if the window was resized or moved.
@@ -701,11 +712,11 @@ bool IsNumPadDigit(int key_code, bool extended_key) {
   if (key_code >= VK_NUMPAD0 && key_code <= VK_NUMPAD9)
     return true;
 
-  // Check for num pad keys without Num Lock.
+  // Check for num pad keys without NumLock.
   // Note: there is no easy way to know if a the key that was pressed comes from
   //       the num pad or the rest of the keyboard.  Investigating how
   //       TranslateMessage() generates the WM_KEYCHAR from an
-  //       ALT + <numpad sequences> it appears it looks at the extended key flag
+  //       ALT + <NumPad sequences> it appears it looks at the extended key flag
   //       (which is on if the key pressed comes from one of the 3 clusters to
   //       the left of the numeric keypad).  So we use it as well.
   return !extended_key &&
diff --git a/chrome/common/win_util.h b/chrome/common/win_util.h
index 030d48a..2aac0a1 100644
--- a/chrome/common/win_util.h
+++ b/chrome/common/win_util.h
@@ -164,6 +164,20 @@ bool SaveFileAsWithFilter(HWND owner,
                           unsigned* index,
                           std::wstring* final_name);
 
+// This function takes the output of a SaveAs dialog: a filename, a filter and
+// the extension originally suggested to the user (shown in the dialog box) and
+// returns back the filename with the appropriate extension tacked on. For
+// example, if you pass in 'foo' as filename with filter '*.jpg' this function
+// will return 'foo.jpg'. It respects MIME types, so if you pass in 'foo.jpeg'
+// with filer '*.jpg' it will return 'foo.jpeg' (will not append .jpg).
+// |filename| should contain the filename selected in the SaveAs dialog box and
+// may include the path, |filter_selected| should be '*.something', for example
+// '*.*' or it can be blank (which is treated as *.*). |suggested_ext| should
+// contain the extension without the dot (.) in front, for example 'jpg'.
+std::wstring AppendExtensionIfNeeded(const std::wstring& filename,
+                                     const std::wstring& filter_selected,
+                                     const std::wstring& suggested_ext);
+
 // If the window does not fit on the default monitor, it is moved and possibly
 // resized appropriately.
 void AdjustWindowToFit(HWND hwnd);
@@ -203,7 +217,7 @@ void SetChildBounds(HWND child_window, HWND parent_window,
 gfx::Rect GetMonitorBoundsForRect(const gfx::Rect& rect);
 
 // Returns true if the virtual key code is a digit coming from the numeric
-// keypad (with or without Num Lock on).  |extended_key| should be set to the
+// keypad (with or without NumLock on).  |extended_key| should be set to the
 // extended key flag specified in the WM_KEYDOWN/UP where the |key_code|
 // originated.
 bool IsNumPadDigit(int key_code, bool extended_key);
diff --git a/chrome/common/win_util_unittest.cc b/chrome/common/win_util_unittest.cc
index 016e391..2452c31 100644
--- a/chrome/common/win_util_unittest.cc
+++ b/chrome/common/win_util_unittest.cc
@@ -50,3 +50,45 @@ TEST(WinUtilTest, EnsureRectIsVisibleInRect) {
     EXPECT_EQ(gfx::Rect(20, 20, 100, 380), child_rect);
   }
 }
+
+static const struct filename_case {
+  const wchar_t* filename;
+  const wchar_t* filter_selected;
+  const wchar_t* suggested_ext;
+  const wchar_t* result;
+} filename_cases[] = {
+  // Test a specific filter (*.jpg).
+  {L"f",         L"*.jpg", L"jpg", L"f.jpg"},
+  {L"f.",        L"*.jpg", L"jpg", L"f..jpg"},
+  {L"f..",       L"*.jpg", L"jpg", L"f...jpg"},
+  {L"f.jpeg",    L"*.jpg", L"jpg", L"f.jpeg"},
+  // Further guarantees.
+  {L"f.jpg.jpg", L"*.jpg", L"jpg", L"f.jpg.jpg"},
+  {L"f.exe.jpg", L"*.jpg", L"jpg", L"f.exe.jpg"},
+  {L"f.jpg.exe", L"*.jpg", L"jpg", L"f.jpg.exe.jpg"},
+  {L"f.exe..",   L"*.jpg", L"jpg", L"f.exe...jpg"},
+  {L"f.jpg..",   L"*.jpg", L"jpg", L"f.jpg...jpg"},
+  // Test the All Files filter (*.jpg).
+  {L"f",         L"*.*",   L"jpg", L"f"},
+  {L"f.",        L"*.*",   L"jpg", L"f"},
+  {L"f..",       L"*.*",   L"jpg", L"f"},
+  {L"f.jpg",     L"*.*",   L"jpg", L"f.jpg"},
+  {L"f.jpeg",    L"*.*",   L"jpg", L"f.jpeg"},  // Same MIME type (diff. ext).
+  // Test the empty filter, which should behave identically to the
+  // All Files filter.
+  {L"f",         L"",      L"jpg", L"f"},
+  {L"f.",        L"",      L"jpg", L"f"},
+  {L"f..",       L"",      L"jpg", L"f"},
+  {L"f.jpg",     L"",      L"jpg", L"f.jpg"},
+  {L"f.jpeg",    L"",      L"jpg", L"f.jpeg"},
+};
+
+TEST(WinUtilTest, AppendingExtensions) {
+  for (unsigned int i = 0; i < arraysize(filename_cases); ++i) {
+    const filename_case& value = filename_cases[i];
+    std::wstring result =
+        win_util::AppendExtensionIfNeeded(value.filename, value.filter_selected,
+                                          value.suggested_ext);
+    EXPECT_EQ(value.result, result);
+  }
+}