Saving a file with a different but known file extension will allow the select...

Saving a file with a different but known file extension will allow the selected extension.

The new extension will only be used if the mime-type exists on the system. This addresses the case where a user *does not* change the file type to "All files" but changes the file extension.

BUG=7499
TEST=Right-click on link, choose "Save link as...", leave the file type as the default but change the file name to have a different extension (different mime-type as well).

[weinjared, 2010-06-21 21:09:19]

Adding Peter as a reviewer.

@Peter, do you think the download shelf should be used to confirm the file
extension change?

[Peter Kasting, 2010-06-21 21:38:01]

We would never want to confirm that the user really meant to do what he just did
w.r.t. changing a file extension.

This functionality should be looked at on Mac and Linux too, not just Windows.

http://codereview.chromium.org/2825010/diff/1/2
File chrome/browser/views/shell_dialogs_win.cc (right):

http://codereview.chromium.org/2825010/diff/1/2#newcode70
chrome/browser/views/shell_dialogs_win.cc:70: if (selected_mime_type.empty()) {
This is needlessly confusing and convoluted.  This whole function should be
rewritten, probably like this:

std::wstring AppendExtensionIfNeeded(const std::wstring& filename,
                                     const std::wstring& suggested_extension) {
  // If the user didn't give us a known extension, and we wanted one, add it.
  std::string selected_mime_type;
  if (!net::GetMimeTypeFromExtension(
          file_util::GetFileExtensionFromPath(filename), &selected_mime_type) &&
      !suggested_extension.empty()) {
    return (filename.empty() || filename[filename.length() - 1] != '.') ?
        filename + L"." + suggested_extension : filename + suggested_extension;
  }

  // Strip any trailing dots, which Windows doesn't allow.
  return filename.substr(0, filename.find_last_not_of(L'.'));
}

This probably breaks something, though.  You should test in all kinds of
scenarios.  I'm also interested in whether we can DCHECK() that |filename| is
non-empty on entrance to the function.

[Peter Kasting, 2010-07-05 03:26:30]

You rewrote the function basically in the way I suggested, which is hard to
complain about.  My question is whether this actually behaves like we want in
all the edge cases, or if I thought through it wrong.  How much testing have you
done?

[Peter Kasting, 2010-07-05 03:26:44]

Oh yeah, you should also update the change description.

[weinjared, 2010-07-05 04:53:17]

On 2010/07/05 03:26:30, Peter Kasting wrote:
> You rewrote the function basically in the way I suggested, which is hard to
> complain about.  My question is whether this actually behaves like we want in
> all the edge cases, or if I thought through it wrong.  How much testing have
you
> done?

I tested it with:
- filenames that ended in multiple periods ("sample.txt....")
- filenames that ended in unrecognizable mime-types ("sample.sample")
- filenames that ended in recognizable mime-types ("sample.html")
- filenames that were preexisting ("sample.html" and "sample.html")
- filenames that consisted of only periods (".........")
- and also when using the "All files" filter

In all cases, the actual filename was equal to what I would expect. I should
note that it would be nice to create some unit tests to document the expected
behavior.

[Peter Kasting, 2010-07-05 17:06:04]

Unit tests would be great.  Could you go ahead and add one in this patch?

Otherwise LGTM.

http://codereview.chromium.org/2825010/diff/4001/5001
File chrome/browser/views/shell_dialogs_win.cc (right):

http://codereview.chromium.org/2825010/diff/4001/5001#newcode35
chrome/browser/views/shell_dialogs_win.cc:35: // will return 'foo.jpg'. It
respects MIME types, so if you pass in 'foo.jpeg'
Nit: These comments are kind of run together and meaningless now.  Could you
rewrite them to accurately describe the new functionality?

http://codereview.chromium.org/2825010/diff/4001/5001#newcode50
chrome/browser/views/shell_dialogs_win.cc:50:
file_util::GetFileExtensionFromPath(filename), &selected_mime_type)) {
Nit: Indent 2 more spaces.

[Peter Kasting, 2010-07-05 19:13:25]

To test this you could probably declare it in an accessible scope (via an
extern) instead of putting it in an anonymous namespace, and then simply write a
separate file that relies on that declaration.

[Peter Kasting, 2010-07-07 02:41:44]

LGTM with comments

http://codereview.chromium.org/2825010/diff/13002/19001
File chrome/browser/views/shell_dialogs_win.cc (right):

http://codereview.chromium.org/2825010/diff/13002/19001#newcode39
chrome/browser/views/shell_dialogs_win.cc:39: std::wstring
AppendExtensionIfNeeded(const std::wstring& filename,
Nit: To be completely safe, you should probably add an explicit extern
declaration of this function in the header with a comment about a unittest
needing it, and then have your test file #include the header rather than simply
re-declare the function itself.

http://codereview.chromium.org/2825010/diff/13002/19002
File chrome/browser/views/shell_dialogs_win_unittest.cc (right):

http://codereview.chromium.org/2825010/diff/13002/19002#newcode19
chrome/browser/views/shell_dialogs_win_unittest.cc:19:
ASSERT_STREQ(L"sample.txt.txt", actual_filename.c_str());
Nit: Use ASSERT_EQ() and drop the c_str().  You may need to add an explicit
std::wstring() around the string constants to make this work.

http://codereview.chromium.org/2825010/diff/13002/19002#newcode64
chrome/browser/views/shell_dialogs_win_unittest.cc:64: }
You should also test that a filename "txt" with a suggested extension of "txt"
gets named "txt.txt" instead of just "txt".

[Peter Kasting, 2010-07-07 22:31:17]

Do you need someone to land this?

[weinjared, 2010-07-07 22:59:13]

On 2010/07/07 22:31:17, Peter Kasting wrote:
> Do you need someone to land this?

Yeah. Can you land this for me?

[Peter Kasting, 2010-07-07 23:07:27]

On 2010/07/07 22:59:13, weinjared wrote:
> Yeah. Can you land this for me?

Sure, but the tree is closed right now.

[weinjared, 2010-07-07 23:11:00]

On 2010/07/07 23:07:27, Peter Kasting wrote:
> On 2010/07/07 22:59:13, weinjared wrote:
> > Yeah. Can you land this for me?
> 
> Sure, but the tree is closed right now.

Ok, no rush. Thanks for the review!

[Peter Kasting, 2010-07-08 18:49:45]

Landed in r51870, closing.