Remove Windows "Save As" dialogs from Save Page code....

Remove Windows "Save As" dialogs from Save Page code.In this change:- convert SavePackage to use async SelectFileDialog- return the chosen filter index in the FileSelected callback- some clean up of save_package.ccBUG=8691
(http://crbug.com/8691)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12799

[brettw, 2009-03-27 20:38:29]

http://codereview.chromium.org/45048/diff/2042/2058
File chrome/browser/download/save_package.h (right):

http://codereview.chromium.org/45048/diff/2042/2058#newcode94
Line 94: SavePackage(WebContents* web_content);
Is it necessary to have two constructors? If so, can you document when you would
use one or the other?

http://codereview.chromium.org/45048/diff/2042/2054
File chrome/browser/shell_dialogs.h (right):

http://codereview.chromium.org/45048/diff/2042/2054#newcode55
Line 55: unsigned index, void* params) = 0;
This shouldn't be an unsigned type.

http://codereview.chromium.org/45048/diff/2042/2054#newcode104
Line 104: virtual void SelectFileWithFilterIndex(Type type,
I wonder if it is possible to merge this with the above function? It seems like
extra complexity to have two versions, and the few callers of this should just
set up all the args they need for the general case.

[Paul Godavari, 2009-03-27 23:11:30]

http://codereview.chromium.org/45048/diff/2042/2058
File chrome/browser/download/save_package.h (right):

http://codereview.chromium.org/45048/diff/2042/2058#newcode94
Line 94: SavePackage(WebContents* web_content);
On 2009/03/27 20:38:30, brettw wrote:
> Is it necessary to have two constructors? If so, can you document when you
would
> use one or the other?

Added a comment to each constructor. One is used in testing where we want to
bypass the filename generation and sanitization by providing a path the test can
check against. It skips prompting the user for the save name.

http://codereview.chromium.org/45048/diff/2042/2054
File chrome/browser/shell_dialogs.h (right):

http://codereview.chromium.org/45048/diff/2042/2054#newcode55
Line 55: unsigned index, void* params) = 0;
On 2009/03/27 20:38:30, brettw wrote:
> This shouldn't be an unsigned type.

Fixed.

http://codereview.chromium.org/45048/diff/2042/2054#newcode104
Line 104: virtual void SelectFileWithFilterIndex(Type type,
On 2009/03/27 20:38:30, brettw wrote:
> I wonder if it is possible to merge this with the above function? It seems
like
> extra complexity to have two versions, and the few callers of this should just
> set up all the args they need for the general case.

Merged into one method and callers use the default value of 0.

[brettw, 2009-03-30 15:01:11]

http://codereview.chromium.org/45048/diff/4093/4108
File chrome/browser/download/save_package.cc (right):

http://codereview.chromium.org/45048/diff/4093/4108#newcode990
Line 990: unsigned filter_index = 2;
And this?

http://codereview.chromium.org/45048/diff/4093/4108#newcode1009
Line 1009: default_extension = L"htm";
I wonder if we can use "html" now? I think "htm" would probably be inappropriate
for Mac & Linux, and "html" would be nicer on Windows as well.

http://codereview.chromium.org/45048/diff/4093/4109
File chrome/browser/download/save_package.h (right):

http://codereview.chromium.org/45048/diff/4093/4109#newcode141
Line 141: unsigned index);
Can this be int?

[brettw, 2009-03-30 15:01:25]

LGTM with previous comments fixed.

[Paul Godavari, 2009-03-30 18:11:38]

http://codereview.chromium.org/45048/diff/4093/4108
File chrome/browser/download/save_package.cc (right):

http://codereview.chromium.org/45048/diff/4093/4108#newcode990
Line 990: unsigned filter_index = 2;
On 2009/03/30 15:01:11, brettw wrote:
> And this?

Done.

http://codereview.chromium.org/45048/diff/4093/4108#newcode1009
Line 1009: default_extension = L"htm";
On 2009/03/30 15:01:11, brettw wrote:
> I wonder if we can use "html" now? I think "htm" would probably be
inappropriate
> for Mac & Linux, and "html" would be nicer on Windows as well.

My preference is to leave it for now, since both Firefox and IE8 use "htm" and
this chunk of code is ifdef'd to Windows.

http://codereview.chromium.org/45048/diff/4093/4109
File chrome/browser/download/save_package.h (right):

http://codereview.chromium.org/45048/diff/4093/4109#newcode141
Line 141: unsigned index);
On 2009/03/30 15:01:11, brettw wrote:
> Can this be int?

Done.