Save Page As MHTML

Save Page As MHTML

--save-page-as-mhtml disables saving pages as html-only and complete with sub-resources in a sibling directory, and enables only saving pages as complete with sub-resources in a single MHTML (MIME-HTML) text file.

The SavePackage class supports all three options, wich will allow saving complete pages to gdata, and may simplify file management for users.

If few enough users need the many-files option, we may be able to remove it and most/all of the SavePackage system.

The generated MHTML is viewable in Chrome on all 4 OSs and Opera (at least on windows).
The generated MHTML is not viewable in chrome when loaded via http[s], only via file://. http://code.google.com/p/chromium-os/issues/detail?id=28654
Chrome recognizes both '.mht' and '.mhtml' as MHTML, however '.mhtml' is more google-able, so that is the default extension when saving MHTML files. Chrome does not sniff '.html' files to see if they are actually MHTML, so it will incorrectly render such mis-named MHTML files.

It is not viewable in Firefox. Firefox tries to help you either open it in another program or download it. There are extensions that provide support for MHTML: MAFF, and UnMHT are two.

It is not viewable in Safari. Safari does not recognize "mht" or "mhtml" as extensions that it can render. Changing the extension to "html" allows Safari to open it, but Safari does not seem to understand the file format so it's rendered incorrectly. A port of UnMHT is available for Safari.

It is not viewable in IE: The webpage cannot be displayed. Most likely cause: •Some content or files on this webpage require a program that you don't have installed.
IE supports MHTML, so it may be possible to tweak the mhtml generator to appease both Opera and IE. http://crbug.com/125477

Regarding OOMs: about:memory showed no change when I saved cnn.com as mhtml (1.4MB), so if there was a spike, it either lasted <200ms or was lost in the noise.

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

[asanka, 2012-04-12 15:32:18]

A couple of high level comments:

- Have you thought about how this would interact with
save_package_file_picker_chromeos.cc, which has similar logic?

- We'll want to end up calling SavePackage::Finish() at some point.

[benjhayden, 2012-04-12 16:52:08]

PTAL, but I'm still working on how to make mhtml the default option, and
how/whether to test this change.

On 2012/04/12 15:32:18, asanka wrote:
> A couple of high level comments:
> 
> - Have you thought about how this would interact with
> save_package_file_picker_chromeos.cc, which has similar logic?

It looks like that file already generates MHTML, so I presume you're asking
about the long-term plans for these dialogs and the save-package system.
My understanding of the long-term plans is that we'd like to eventually unify
the file pickers to both offer a choice between html-only and
complete-single-file, and trash the SavePackage system and the
complete-many-files option. This plan might not be feasible if many users depend
on the many-files option for some reason, unless we can offer a simple
cross-platform way to unpack the MHTML into many files, which would be useful
for MHTMLs downloaded from the web.

Or were you suggesting making the file picker generate the MHTML directly,
without involving the SavePackage? That would require duplicating the logic to
create a DownloadItem, which is not big chunk of logic, so I could do that if
y'all think it's a good idea.

> 
> - We'll want to end up calling SavePackage::Finish() at some point.

PTAL.

[cbentzel, 2012-04-12 17:29:07]

Please fill in more details on the CL description and include a BUG=/TEST= line

[Randy Smith (Not in Mondays), 2012-04-12 17:36:55]

On 2012/04/12 16:52:08, benjhayden_chromium wrote:
> PTAL, but I'm still working on how to make mhtml the default option, and
> how/whether to test this change.
> 
> On 2012/04/12 15:32:18, asanka wrote:
> > A couple of high level comments:
> > 
> > - Have you thought about how this would interact with
> > save_package_file_picker_chromeos.cc, which has similar logic?
> 
> It looks like that file already generates MHTML, so I presume you're asking
> about the long-term plans for these dialogs and the save-package system.

I'd like to add to that question the obvious shorter term one of just checking
to make sure this change won't mess up what they're currently doing.

> My understanding of the long-term plans is that we'd like to eventually unify
> the file pickers to both offer a choice between html-only and
> complete-single-file, and trash the SavePackage system and the
> complete-many-files option. This plan might not be feasible if many users
depend
> on the many-files option for some reason, unless we can offer a simple
> cross-platform way to unpack the MHTML into many files, which would be useful
> for MHTMLs downloaded from the web.

We also want to steer by the medium-term star of integrating the MHTML
generation for ChromeOS into save package, so that we don't duplicate code. 
This CL/you don't need to do that, but you should make sure that Achuith is on
top of what you're doing so that he can integrate the ChromeOS stuff when you're
done.  (Added as reviewer.)

> Or were you suggesting making the file picker generate the MHTML directly,
> without involving the SavePackage? That would require duplicating the logic to
> create a DownloadItem, which is not big chunk of logic, so I could do that if
> y'all think it's a good idea.

This strikes me as a bad way to go--if you want to do this, grab me and a
whiteboard and convince me it's a good idea?

[cbentzel, 2012-04-12 17:44:13]

I'm gonna remove myself as a reviewer - don't think I'll be able to add much
here.

I'm glad that it's easy to get a basic version up and running!

One thought on browser/black-box testing - could we navigate to a page, do "Save
Page As" to MHTML, load that file back up, and confirm that some of the contents
are the same?

[benjhayden, 2012-04-12 17:45:57]

On 2012/04/12 17:36:55, rdsmith wrote:
> On 2012/04/12 16:52:08, benjhayden_chromium wrote:
> > PTAL, but I'm still working on how to make mhtml the default option, and
> > how/whether to test this change.
> > 
> > On 2012/04/12 15:32:18, asanka wrote:
> > > A couple of high level comments:
> > > 
> > > - Have you thought about how this would interact with
> > > save_package_file_picker_chromeos.cc, which has similar logic?
> > 
> > It looks like that file already generates MHTML, so I presume you're asking
> > about the long-term plans for these dialogs and the save-package system.
> 
> I'd like to add to that question the obvious shorter term one of just checking
> to make sure this change won't mess up what they're currently doing.
> 
> > My understanding of the long-term plans is that we'd like to eventually
unify
> > the file pickers to both offer a choice between html-only and
> > complete-single-file, and trash the SavePackage system and the
> > complete-many-files option. This plan might not be feasible if many users
> depend
> > on the many-files option for some reason, unless we can offer a simple
> > cross-platform way to unpack the MHTML into many files, which would be
useful
> > for MHTMLs downloaded from the web.
> 
> We also want to steer by the medium-term star of integrating the MHTML
> generation for ChromeOS into save package, so that we don't duplicate code. 
> This CL/you don't need to do that, but you should make sure that Achuith is on
> top of what you're doing so that he can integrate the ChromeOS stuff when
you're
> done.  (Added as reviewer.)

Oops, looks like I modified the CL description at the same time. Would you mind
re-adding him?

> 
> > Or were you suggesting making the file picker generate the MHTML directly,
> > without involving the SavePackage? That would require duplicating the logic
to
> > create a DownloadItem, which is not big chunk of logic, so I could do that
if
> > y'all think it's a good idea.
> 
> This strikes me as a bad way to go--if you want to do this, grab me and a
> whiteboard and convince me it's a good idea?

It might make it easier to remove SavePackage someday if SavePackage just does
the many-files thing.
I don't know SavePackage well enough to have an opinion on whether it's a good
idea.

[Randy Smith (Not in Mondays), 2012-04-12 17:48:47]

On 2012/04/12 17:45:57, benjhayden_chromium wrote:
> On 2012/04/12 17:36:55, rdsmith wrote:
> > On 2012/04/12 16:52:08, benjhayden_chromium wrote:
> > > PTAL, but I'm still working on how to make mhtml the default option, and
> > > how/whether to test this change.
> > > 
> > > On 2012/04/12 15:32:18, asanka wrote:
> > > > A couple of high level comments:
> > > > 
> > > > - Have you thought about how this would interact with
> > > > save_package_file_picker_chromeos.cc, which has similar logic?
> > > 
> > > It looks like that file already generates MHTML, so I presume you're
asking
> > > about the long-term plans for these dialogs and the save-package system.
> > 
> > I'd like to add to that question the obvious shorter term one of just
checking
> > to make sure this change won't mess up what they're currently doing.
> > 
> > > My understanding of the long-term plans is that we'd like to eventually
> unify
> > > the file pickers to both offer a choice between html-only and
> > > complete-single-file, and trash the SavePackage system and the
> > > complete-many-files option. This plan might not be feasible if many users
> > depend
> > > on the many-files option for some reason, unless we can offer a simple
> > > cross-platform way to unpack the MHTML into many files, which would be
> useful
> > > for MHTMLs downloaded from the web.
> > 
> > We also want to steer by the medium-term star of integrating the MHTML
> > generation for ChromeOS into save package, so that we don't duplicate code. 
> > This CL/you don't need to do that, but you should make sure that Achuith is
on
> > top of what you're doing so that he can integrate the ChromeOS stuff when
> you're
> > done.  (Added as reviewer.)
> 
> Oops, looks like I modified the CL description at the same time. Would you
mind
> re-adding him?

Done.

> > > Or were you suggesting making the file picker generate the MHTML directly,
> > > without involving the SavePackage? That would require duplicating the
logic
> to
> > > create a DownloadItem, which is not big chunk of logic, so I could do that
> if
> > > y'all think it's a good idea.
> > 
> > This strikes me as a bad way to go--if you want to do this, grab me and a
> > whiteboard and convince me it's a good idea?
> 
> It might make it easier to remove SavePackage someday if SavePackage just does
> the many-files thing.
> I don't know SavePackage well enough to have an opinion on whether it's a good
> idea.

I don't think it'll make it a lot harder to remove SavePackage, and one of my
beefs with the ChromeOS implementation is that the file picker does a lot more
than picking files, which violates the principle of least astonishment pretty
severely.  So my preference is to actually do this inside of SavePackage.

[asanka, 2012-04-12 17:52:34]

On 2012/04/12 17:48:47, rdsmith wrote:
> On 2012/04/12 17:45:57, benjhayden_chromium wrote:
> > On 2012/04/12 17:36:55, rdsmith wrote:
> > > On 2012/04/12 16:52:08, benjhayden_chromium wrote:
> > > > PTAL, but I'm still working on how to make mhtml the default option, and
> > > > how/whether to test this change.
> > > > 
> > > > On 2012/04/12 15:32:18, asanka wrote:
> > > > > A couple of high level comments:
> > > > > 
> > > > > - Have you thought about how this would interact with
> > > > > save_package_file_picker_chromeos.cc, which has similar logic?
> > > > 
> > > > It looks like that file already generates MHTML, so I presume you're
> asking
> > > > about the long-term plans for these dialogs and the save-package system.
> > > 
> > > I'd like to add to that question the obvious shorter term one of just
> checking
> > > to make sure this change won't mess up what they're currently doing.
> > > 
> > > > My understanding of the long-term plans is that we'd like to eventually
> > unify
> > > > the file pickers to both offer a choice between html-only and
> > > > complete-single-file, and trash the SavePackage system and the
> > > > complete-many-files option. This plan might not be feasible if many
users
> > > depend
> > > > on the many-files option for some reason, unless we can offer a simple
> > > > cross-platform way to unpack the MHTML into many files, which would be
> > useful
> > > > for MHTMLs downloaded from the web.
> > > 
> > > We also want to steer by the medium-term star of integrating the MHTML
> > > generation for ChromeOS into save package, so that we don't duplicate
code. 
> > > This CL/you don't need to do that, but you should make sure that Achuith
is
> on
> > > top of what you're doing so that he can integrate the ChromeOS stuff when
> > you're
> > > done.  (Added as reviewer.)
> > 
> > Oops, looks like I modified the CL description at the same time. Would you
> mind
> > re-adding him?
> 
> Done.
> 
> > > > Or were you suggesting making the file picker generate the MHTML
directly,
> > > > without involving the SavePackage? That would require duplicating the
> logic
> > to
> > > > create a DownloadItem, which is not big chunk of logic, so I could do
that
> > if
> > > > y'all think it's a good idea.
> > > 
> > > This strikes me as a bad way to go--if you want to do this, grab me and a
> > > whiteboard and convince me it's a good idea?
> > 
> > It might make it easier to remove SavePackage someday if SavePackage just
does
> > the many-files thing.
> > I don't know SavePackage well enough to have an opinion on whether it's a
good
> > idea.
> 
> I don't think it'll make it a lot harder to remove SavePackage, and one of my
> beefs with the ChromeOS implementation is that the file picker does a lot more
> than picking files, which violates the principle of least astonishment pretty
> severely.  So my preference is to actually do this inside of SavePackage.

+1. I also prefer to have the MHTML generation logic in SavePackage and let the
file picker be the file picker.

[Randy Smith (Not in Mondays), 2012-04-12 18:00:02]

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker.cc:86:
suggested_path.Extension().compare(FILE_PATH_LITERAL("html"))) {
I may be being naive, but doesn't the logic as written mean that if the
suggested file name is .htm/.html that'll show up as an option for the .mht
save?  I'd think you'd want the .mht options as exclusions here as well?

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker.cc:94:
l10n_util::GetStringUTF16(kIndexToIDS[kSelectFileHtmlOnlyIndex]));
Why did you need to add an override here?  (To be fair, if I look at the code
the way it used to be, it looks like the select file complete override was added
at the select file html only index location--did it used to be broken?  If so,
that answers my question.)

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker.cc:104: 
nit: Remove extra blank line.

[achuithb, 2012-04-12 19:14:51]

On 2012/04/12 17:52:34, asanka wrote:
> On 2012/04/12 17:48:47, rdsmith wrote:
> > On 2012/04/12 17:45:57, benjhayden_chromium wrote:
> > > On 2012/04/12 17:36:55, rdsmith wrote:
> > > > On 2012/04/12 16:52:08, benjhayden_chromium wrote:
> > > > > PTAL, but I'm still working on how to make mhtml the default option,
and
> > > > > how/whether to test this change.
> > > > > 
> > > > > On 2012/04/12 15:32:18, asanka wrote:
> > > > > > A couple of high level comments:
> > > > > > 
> > > > > > - Have you thought about how this would interact with
> > > > > > save_package_file_picker_chromeos.cc, which has similar logic?
> > > > > 
> > > > > It looks like that file already generates MHTML, so I presume you're
> > asking
> > > > > about the long-term plans for these dialogs and the save-package
system.
> > > > 
> > > > I'd like to add to that question the obvious shorter term one of just
> > checking
> > > > to make sure this change won't mess up what they're currently doing.
> > > > 
> > > > > My understanding of the long-term plans is that we'd like to
eventually
> > > unify
> > > > > the file pickers to both offer a choice between html-only and
> > > > > complete-single-file, and trash the SavePackage system and the
> > > > > complete-many-files option. This plan might not be feasible if many
> users
> > > > depend
> > > > > on the many-files option for some reason, unless we can offer a simple
> > > > > cross-platform way to unpack the MHTML into many files, which would be
> > > useful
> > > > > for MHTMLs downloaded from the web.
> > > > 
> > > > We also want to steer by the medium-term star of integrating the MHTML
> > > > generation for ChromeOS into save package, so that we don't duplicate
> code. 
> > > > This CL/you don't need to do that, but you should make sure that Achuith
> is
> > on
> > > > top of what you're doing so that he can integrate the ChromeOS stuff
when
> > > you're
> > > > done.  (Added as reviewer.)
> > > 
> > > Oops, looks like I modified the CL description at the same time. Would you
> > mind
> > > re-adding him?
> > 
> > Done.
> > 
> > > > > Or were you suggesting making the file picker generate the MHTML
> directly,
> > > > > without involving the SavePackage? That would require duplicating the
> > logic
> > > to
> > > > > create a DownloadItem, which is not big chunk of logic, so I could do
> that
> > > if
> > > > > y'all think it's a good idea.
> > > > 
> > > > This strikes me as a bad way to go--if you want to do this, grab me and
a
> > > > whiteboard and convince me it's a good idea?
> > > 
> > > It might make it easier to remove SavePackage someday if SavePackage just
> does
> > > the many-files thing.
> > > I don't know SavePackage well enough to have an opinion on whether it's a
> good
> > > idea.
> > 
> > I don't think it'll make it a lot harder to remove SavePackage, and one of
my
> > beefs with the ChromeOS implementation is that the file picker does a lot
more
> > than picking files, which violates the principle of least astonishment
pretty
> > severely.  So my preference is to actually do this inside of SavePackage.
> 
> +1. I also prefer to have the MHTML generation logic in SavePackage and let
the
> file picker be the file picker.

I'm not defending what we did for SavePackageFilePickerChromeos - we had to take
some shortcuts before the branch point.

But one of the counter-arguments I heard was that 'Save As' MHTML does not
actually download anything - it takes the existing resources of the page and
writes them to disk. With that perspective, it's worth considering not creating
a DownloadItem or going through downloads code for this path.

[asanka, 2012-04-12 19:28:48]

On 2012/04/12 19:14:51, achuith.bhandarkar wrote:
> On 2012/04/12 17:52:34, asanka wrote:
> > +1. I also prefer to have the MHTML generation logic in SavePackage and let
> the
> > file picker be the file picker.
> 
> I'm not defending what we did for SavePackageFilePickerChromeos - we had to
take
> some shortcuts before the branch point.

Understood. :) We agreed that it was a reasonable approach given the
constraints.

> But one of the counter-arguments I heard was that 'Save As' MHTML does not
> actually download anything - it takes the existing resources of the page and
> writes them to disk. With that perspective, it's worth considering not
creating
> a DownloadItem or going through downloads code for this path.

The creation of the fake DownloadItem allows the rest of the UI to work with the
SavePackage download and also allows the item to be added to history. While
these weren't directly applicable to GData downloads (they didn't need to be
persisted in the UI or in history), these are things we would like to have for
regular MHTML downloads. Since all that logic is already in SavePackage it makes
sense to add the MHTML support there and try to reuse the logic for CrOS. At
least that's my thinking.

[Randy Smith (Not in Mondays), 2012-04-12 19:30:17]

On 2012/04/12 19:14:51, achuith.bhandarkar wrote:
> On 2012/04/12 17:52:34, asanka wrote:
> > On 2012/04/12 17:48:47, rdsmith wrote:
> > > On 2012/04/12 17:45:57, benjhayden_chromium wrote:
> > > > On 2012/04/12 17:36:55, rdsmith wrote:
> > > > > On 2012/04/12 16:52:08, benjhayden_chromium wrote:
> > > > > > PTAL, but I'm still working on how to make mhtml the default option,
> and
> > > > > > how/whether to test this change.
> > > > > > 
> > > > > > On 2012/04/12 15:32:18, asanka wrote:
> > > > > > > A couple of high level comments:
> > > > > > > 
> > > > > > > - Have you thought about how this would interact with
> > > > > > > save_package_file_picker_chromeos.cc, which has similar logic?
> > > > > > 
> > > > > > It looks like that file already generates MHTML, so I presume you're
> > > asking
> > > > > > about the long-term plans for these dialogs and the save-package
> system.
> > > > > 
> > > > > I'd like to add to that question the obvious shorter term one of just
> > > checking
> > > > > to make sure this change won't mess up what they're currently doing.
> > > > > 
> > > > > > My understanding of the long-term plans is that we'd like to
> eventually
> > > > unify
> > > > > > the file pickers to both offer a choice between html-only and
> > > > > > complete-single-file, and trash the SavePackage system and the
> > > > > > complete-many-files option. This plan might not be feasible if many
> > users
> > > > > depend
> > > > > > on the many-files option for some reason, unless we can offer a
simple
> > > > > > cross-platform way to unpack the MHTML into many files, which would
be
> > > > useful
> > > > > > for MHTMLs downloaded from the web.
> > > > > 
> > > > > We also want to steer by the medium-term star of integrating the MHTML
> > > > > generation for ChromeOS into save package, so that we don't duplicate
> > code. 
> > > > > This CL/you don't need to do that, but you should make sure that
Achuith
> > is
> > > on
> > > > > top of what you're doing so that he can integrate the ChromeOS stuff
> when
> > > > you're
> > > > > done.  (Added as reviewer.)
> > > > 
> > > > Oops, looks like I modified the CL description at the same time. Would
you
> > > mind
> > > > re-adding him?
> > > 
> > > Done.
> > > 
> > > > > > Or were you suggesting making the file picker generate the MHTML
> > directly,
> > > > > > without involving the SavePackage? That would require duplicating
the
> > > logic
> > > > to
> > > > > > create a DownloadItem, which is not big chunk of logic, so I could
do
> > that
> > > > if
> > > > > > y'all think it's a good idea.
> > > > > 
> > > > > This strikes me as a bad way to go--if you want to do this, grab me
and
> a
> > > > > whiteboard and convince me it's a good idea?
> > > > 
> > > > It might make it easier to remove SavePackage someday if SavePackage
just
> > does
> > > > the many-files thing.
> > > > I don't know SavePackage well enough to have an opinion on whether it's
a
> > good
> > > > idea.
> > > 
> > > I don't think it'll make it a lot harder to remove SavePackage, and one of
> my
> > > beefs with the ChromeOS implementation is that the file picker does a lot
> more
> > > than picking files, which violates the principle of least astonishment
> pretty
> > > severely.  So my preference is to actually do this inside of SavePackage.
> > 
> > +1. I also prefer to have the MHTML generation logic in SavePackage and let
> the
> > file picker be the file picker.
> 
> I'm not defending what we did for SavePackageFilePickerChromeos - we had to
take
> some shortcuts before the branch point.
> 
> But one of the counter-arguments I heard was that 'Save As' MHTML does not
> actually download anything - it takes the existing resources of the page and
> writes them to disk. With that perspective, it's worth considering not
creating
> a DownloadItem or going through downloads code for this path.

I think the place we have to start from is producing a consistent user
experience.  I'd rather not have inconsistencies between ChromeOS and Chrome,
but there may be arguments in favor of such inconsistencies.  But I think it's
pretty clearly a bad user experience if changing the file type in a dialog box
produces noticeably different behavior within Chrome (specifically, Save as only
HTML will produce something on the shelf, and if we avoid creating a download
item, save as MHTML will not).  Am I missing something?

[achuithb, 2012-04-12 19:40:02]

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker.cc:36:
content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML_SINGLE_FILE_MHTML,
Just asking here - does this need to be so long? Isn't SAVE_PAGE_TYPE_AS_MHTML
sufficient?

http://codereview.chromium.org/10069014/diff/5001/content/browser/download/sa...
File content/browser/download/save_package.h (right):

http://codereview.chromium.org/10069014/diff/5001/content/browser/download/sa...
content/browser/download/save_package.h:121: void MHTMLGenerated(const FilePath&
path, int64 size);
Please add a comment here saying that this is a callback for
WebContents::MHTMLGenerated.

[achuithb, 2012-04-12 19:41:46]

I think this should continue to work with the Cros save page file picker. The
cros file picker has never offered a selection of save options. 

It should be fairly straightforward to convert the cros save page file picker to
callback into SavePackage and not save the mhtml directly.

[achuithb, 2012-04-12 19:42:22]

On 2012/04/12 19:30:17, rdsmith wrote:
> On 2012/04/12 19:14:51, achuith.bhandarkar wrote:
> > On 2012/04/12 17:52:34, asanka wrote:
> > > On 2012/04/12 17:48:47, rdsmith wrote:
> > > > On 2012/04/12 17:45:57, benjhayden_chromium wrote:
> > > > > On 2012/04/12 17:36:55, rdsmith wrote:
> > > > > > On 2012/04/12 16:52:08, benjhayden_chromium wrote:
> > > > > > > PTAL, but I'm still working on how to make mhtml the default
option,
> > and
> > > > > > > how/whether to test this change.
> > > > > > > 
> > > > > > > On 2012/04/12 15:32:18, asanka wrote:
> > > > > > > > A couple of high level comments:
> > > > > > > > 
> > > > > > > > - Have you thought about how this would interact with
> > > > > > > > save_package_file_picker_chromeos.cc, which has similar logic?
> > > > > > > 
> > > > > > > It looks like that file already generates MHTML, so I presume
you're
> > > > asking
> > > > > > > about the long-term plans for these dialogs and the save-package
> > system.
> > > > > > 
> > > > > > I'd like to add to that question the obvious shorter term one of
just
> > > > checking
> > > > > > to make sure this change won't mess up what they're currently doing.
> > > > > > 
> > > > > > > My understanding of the long-term plans is that we'd like to
> > eventually
> > > > > unify
> > > > > > > the file pickers to both offer a choice between html-only and
> > > > > > > complete-single-file, and trash the SavePackage system and the
> > > > > > > complete-many-files option. This plan might not be feasible if
many
> > > users
> > > > > > depend
> > > > > > > on the many-files option for some reason, unless we can offer a
> simple
> > > > > > > cross-platform way to unpack the MHTML into many files, which
would
> be
> > > > > useful
> > > > > > > for MHTMLs downloaded from the web.
> > > > > > 
> > > > > > We also want to steer by the medium-term star of integrating the
MHTML
> > > > > > generation for ChromeOS into save package, so that we don't
duplicate
> > > code. 
> > > > > > This CL/you don't need to do that, but you should make sure that
> Achuith
> > > is
> > > > on
> > > > > > top of what you're doing so that he can integrate the ChromeOS stuff
> > when
> > > > > you're
> > > > > > done.  (Added as reviewer.)
> > > > > 
> > > > > Oops, looks like I modified the CL description at the same time. Would
> you
> > > > mind
> > > > > re-adding him?
> > > > 
> > > > Done.
> > > > 
> > > > > > > Or were you suggesting making the file picker generate the MHTML
> > > directly,
> > > > > > > without involving the SavePackage? That would require duplicating
> the
> > > > logic
> > > > > to
> > > > > > > create a DownloadItem, which is not big chunk of logic, so I could
> do
> > > that
> > > > > if
> > > > > > > y'all think it's a good idea.
> > > > > > 
> > > > > > This strikes me as a bad way to go--if you want to do this, grab me
> and
> > a
> > > > > > whiteboard and convince me it's a good idea?
> > > > > 
> > > > > It might make it easier to remove SavePackage someday if SavePackage
> just
> > > does
> > > > > the many-files thing.
> > > > > I don't know SavePackage well enough to have an opinion on whether
it's
> a
> > > good
> > > > > idea.
> > > > 
> > > > I don't think it'll make it a lot harder to remove SavePackage, and one
of
> > my
> > > > beefs with the ChromeOS implementation is that the file picker does a
lot
> > more
> > > > than picking files, which violates the principle of least astonishment
> > pretty
> > > > severely.  So my preference is to actually do this inside of
SavePackage.
> > > 
> > > +1. I also prefer to have the MHTML generation logic in SavePackage and
let
> > the
> > > file picker be the file picker.
> > 
> > I'm not defending what we did for SavePackageFilePickerChromeos - we had to
> take
> > some shortcuts before the branch point.
> > 
> > But one of the counter-arguments I heard was that 'Save As' MHTML does not
> > actually download anything - it takes the existing resources of the page and
> > writes them to disk. With that perspective, it's worth considering not
> creating
> > a DownloadItem or going through downloads code for this path.
> 
> I think the place we have to start from is producing a consistent user
> experience.  I'd rather not have inconsistencies between ChromeOS and Chrome,
> but there may be arguments in favor of such inconsistencies.  But I think it's
> pretty clearly a bad user experience if changing the file type in a dialog box
> produces noticeably different behavior within Chrome (specifically, Save as
only
> HTML will produce something on the shelf, and if we avoid creating a download
> item, save as MHTML will not).  Am I missing something?

No, you're right.

[achuithb, 2012-04-12 19:52:09]

Any plan to add a browser test for this in save_page_browsertest.cc?

[asanka, 2012-04-13 15:28:19]

http://codereview.chromium.org/10069014/diff/5001/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10069014/diff/5001/content/browser/download/sa...
content/browser/download/save_package.cc:318: void
SavePackage::MHTMLGenerated(const FilePath& path, int64 size) {
I think the size will be -1 if there was an error during generation. If
unsuccessful we could call Cancel(false) instead of Finish().

Also on successful completion, we could set wrote_to_completed_file_ so that the
SAVE_PACKAGE_WRITE_TO_COMPLETED stat is recorded (unless you are considering a
separate set of stats for MHTML?)

[benjhayden, 2012-04-13 20:44:11]

PTAL
LMK if I forgot anything besides figuring out whether this might increase OOMs.

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker.cc:36:
content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML_SINGLE_FILE_MHTML,
On 2012/04/12 19:40:02, achuith.bhandarkar wrote:
> Just asking here - does this need to be so long? Isn't SAVE_PAGE_TYPE_AS_MHTML
> sufficient?

Done.

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker.cc:86:
suggested_path.Extension().compare(FILE_PATH_LITERAL("html"))) {
On 2012/04/12 18:00:02, rdsmith wrote:
> I may be being naive, but doesn't the logic as written mean that if the
> suggested file name is .htm/.html that'll show up as an option for the .mht
> save?  I'd think you'd want the .mht options as exclusions here as well?

Sorry, I don't quite understand.
I don't think that suggested_path will ever be mht.
To be sure, there are a few bugs in the most recent patch set's file picker's
handling of mhtml. I just don't know how to deal with them.

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker.cc:94:
l10n_util::GetStringUTF16(kIndexToIDS[kSelectFileHtmlOnlyIndex]));
On 2012/04/12 18:00:02, rdsmith wrote:
> Why did you need to add an override here?  (To be fair, if I look at the code
> the way it used to be, it looks like the select file complete override was
added
> at the select file html only index location--did it used to be broken?  If so,
> that answers my question.)

I didn't actually change any functionality here.
I just moved this override up a few lines and changed how it's spelled: from
'kSelectFileCompleteIndex - 1' to 'kSelectFileHtmlOnlyIndex'.

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker.cc:104: 
On 2012/04/12 18:00:02, rdsmith wrote:
> nit: Remove extra blank line.

Done.

http://codereview.chromium.org/10069014/diff/5001/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10069014/diff/5001/content/browser/download/sa...
content/browser/download/save_package.cc:318: void
SavePackage::MHTMLGenerated(const FilePath& path, int64 size) {
On 2012/04/13 15:28:19, asanka wrote:
> I think the size will be -1 if there was an error during generation. If
> unsuccessful we could call Cancel(false) instead of Finish().
> 
> Also on successful completion, we could set wrote_to_completed_file_ so that
the
> SAVE_PACKAGE_WRITE_TO_COMPLETED stat is recorded (unless you are considering a
> separate set of stats for MHTML?)

Done.

http://codereview.chromium.org/10069014/diff/5001/content/browser/download/sa...
File content/browser/download/save_package.h (right):

http://codereview.chromium.org/10069014/diff/5001/content/browser/download/sa...
content/browser/download/save_package.h:121: void MHTMLGenerated(const FilePath&
path, int64 size);
On 2012/04/12 19:40:02, achuith.bhandarkar wrote:
> Please add a comment here saying that this is a callback for
> WebContents::MHTMLGenerated.

Done.

[Randy Smith (Not in Mondays), 2012-04-16 17:45:46]

Two high level comments:
a) Chris agreed to do the state logic review for save_package.cc; I've re-added
him for that purpose.

b) It's worthwhile having an issue to target.  Could you add BUG=120416?

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker.cc:86:
suggested_path.Extension().compare(FILE_PATH_LITERAL("html"))) {
On 2012/04/13 20:44:11, benjhayden_chromium wrote:
> On 2012/04/12 18:00:02, rdsmith wrote:
> > I may be being naive, but doesn't the logic as written mean that if the
> > suggested file name is .htm/.html that'll show up as an option for the .mht
> > save?  I'd think you'd want the .mht options as exclusions here as well?
> 
> Sorry, I don't quite understand.
> I don't think that suggested_path will ever be mht.

I guess the question is whether you're sure that gun was really fired six times
:-}.  The suggested_path logic is convoluted, the internet is a cesspool, and I
suspect the behavior is "undefined" if the file picker is passed two separate
extension types where there's an overlap in strings between them.  So it would
seem worth protecting against to me.  

> To be sure, there are a few bugs in the most recent patch set's file picker's
> handling of mhtml. I just don't know how to deal with them.

Well, you've got me curious.  What's up?  (Feel free to ignore this question if
you'd rather spend them time, say, fixing the bugs :-}).

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:92:
file_type_info.extensions.resize(3);
nit, suggestion (i.e. up to you), while you're here: Symbolic constant?

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:93: 
nit, while you're here: Could you put in a comment explaining why we're
sometimes offsetting by 1?  (Just something indicating that the arguments to
SelectFileDialog is index origin 1, but the extension description arrays are
index origin 0).

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:95:
l10n_util::GetStringUTF16(kIndexToIDS[kSelectFileHtmlOnlyIndex]));
nit, suggestion, while you're here: We're dealing with these two arrays
(.extensions and .extension_description) differently, using push_back for one,
and resize() + direct index for the other.  I think the resize()+direct index
option is clearer; maybe switch over to using that for both?

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:69:
prefs::kDownloadDefaultDirectory, save_dir_.path());
What's the context for this change?

http://codereview.chromium.org/10069014/diff/17003/content/browser/download/s...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10069014/diff/17003/content/browser/download/s...
content/browser/download/save_package.cc:739: if (save_type_ !=
content::SAVE_PAGE_TYPE_AS_MHTML) {
I'm somewhat torn about this and related logic.  It looks like the behavior
we're going to end up with is:
* HTML only: Size == count of items == 1
* HTML complete: Size == count of items == n
* MHTML: Size == number of bytes downloads

Before we were broken but consistent (size was always count of items).  Now
we're inconsistent, and to my mind, inconsistently inconsistent (because the
other single file case is still count of items rather than number of bytes).  Is
this really the behavior we want?

http://codereview.chromium.org/10069014/diff/17003/content/browser/download/s...
content/browser/download/save_package.cc:740:
download_->OnAllDataSaved(all_save_items_count_,
nit: If there's an easy, not too ugly way to do it, I'd like all the
OnAllDataSaved calls to be in the same place.  That way a) there's less chance
of us ending up with two or zero calls at some change in the future, and b)
whatever behavior we decide on with regard to size will be very visible at one
place in the code.

http://codereview.chromium.org/10069014/diff/17003/content/browser/download/s...
File content/browser/download/save_package.h (right):

http://codereview.chromium.org/10069014/diff/17003/content/browser/download/s...
content/browser/download/save_package.h:38: // complete-html or
complete-html-single-file and providing the information for
nit, suggestion: I think of the target audience for comments as being other
developers, and I think of other developers as having an easier time with a save
format description of MHTML than with complete-html-single-file.  And there's
also the change in enum.  Would it make sense to change this to refer to MHTML?

[benjhayden, 2012-04-16 19:08:05]

Hiding the extension broke deduplication. Looking for a simple fix.

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/5001/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker.cc:86:
suggested_path.Extension().compare(FILE_PATH_LITERAL("html"))) {
On 2012/04/16 17:45:47, rdsmith wrote:
> On 2012/04/13 20:44:11, benjhayden_chromium wrote:
> > On 2012/04/12 18:00:02, rdsmith wrote:
> > > I may be being naive, but doesn't the logic as written mean that if the
> > > suggested file name is .htm/.html that'll show up as an option for the
.mht
> > > save?  I'd think you'd want the .mht options as exclusions here as well?
> > 
> > Sorry, I don't quite understand.
> > I don't think that suggested_path will ever be mht.
> 
> I guess the question is whether you're sure that gun was really fired six
times
> :-}.  The suggested_path logic is convoluted, the internet is a cesspool, and
I
> suspect the behavior is "undefined" if the file picker is passed two separate
> extension types where there's an overlap in strings between them.  So it would
> seem worth protecting against to me.  

Sorry, you're right, the suggested_path can be .mht/.mhtml if the URL ends with
.mht/.mhtml.

However, it appears that the extensions vector is only used (at least on gtk) to
filter which existing files are displayed. So I'm still not sure what you mean
by "if the suggested file name is .htm/.html that'll show up as an option for
the .mht save".

> 
> > To be sure, there are a few bugs in the most recent patch set's file
picker's
> > handling of mhtml. I just don't know how to deal with them.
> 
> Well, you've got me curious.  What's up?  (Feel free to ignore this question
if
> you'd rather spend them time, say, fixing the bugs :-}).

I think that hiding the extension will effectively deal with the issues.

Ah, however, it breaks the deduplication...

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:92:
file_type_info.extensions.resize(3);
On 2012/04/16 17:45:47, rdsmith wrote:
> nit, suggestion (i.e. up to you), while you're here: Symbolic constant?

Done.

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:93: 
On 2012/04/16 17:45:47, rdsmith wrote:
> nit, while you're here: Could you put in a comment explaining why we're
> sometimes offsetting by 1?  (Just something indicating that the arguments to
> SelectFileDialog is index origin 1, but the extension description arrays are
> index origin 0).

Done.

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:95:
l10n_util::GetStringUTF16(kIndexToIDS[kSelectFileHtmlOnlyIndex]));
On 2012/04/16 17:45:47, rdsmith wrote:
> nit, suggestion, while you're here: We're dealing with these two arrays
> (.extensions and .extension_description) differently, using push_back for one,
> and resize() + direct index for the other.  I think the resize()+direct index
> option is clearer; maybe switch over to using that for both?

Done.

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10069014/diff/17003/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:69:
prefs::kDownloadDefaultDirectory, save_dir_.path());
On 2012/04/16 17:45:47, rdsmith wrote:
> What's the context for this change?

I noticed it was leaving files in my ~/Downloads/.

http://codereview.chromium.org/10069014/diff/17003/content/browser/download/s...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10069014/diff/17003/content/browser/download/s...
content/browser/download/save_package.cc:739: if (save_type_ !=
content::SAVE_PAGE_TYPE_AS_MHTML) {
On 2012/04/16 17:45:47, rdsmith wrote:
> I'm somewhat torn about this and related logic.  It looks like the behavior
> we're going to end up with is:
> * HTML only: Size == count of items == 1
> * HTML complete: Size == count of items == n
> * MHTML: Size == number of bytes downloads
> 
> Before we were broken but consistent (size was always count of items).  Now
> we're inconsistent, and to my mind, inconsistently inconsistent (because the
> other single file case is still count of items rather than number of bytes). 
Is
> this really the behavior we want?

Done.

http://codereview.chromium.org/10069014/diff/17003/content/browser/download/s...
content/browser/download/save_package.cc:740:
download_->OnAllDataSaved(all_save_items_count_,
On 2012/04/16 17:45:47, rdsmith wrote:
> nit: If there's an easy, not too ugly way to do it, I'd like all the
> OnAllDataSaved calls to be in the same place.  That way a) there's less chance
> of us ending up with two or zero calls at some change in the future, and b)
> whatever behavior we decide on with regard to size will be very visible at one
> place in the code.

Done.

http://codereview.chromium.org/10069014/diff/17003/content/browser/download/s...
File content/browser/download/save_package.h (right):

http://codereview.chromium.org/10069014/diff/17003/content/browser/download/s...
content/browser/download/save_package.h:38: // complete-html or
complete-html-single-file and providing the information for
On 2012/04/16 17:45:47, rdsmith wrote:
> nit, suggestion: I think of the target audience for comments as being other
> developers, and I think of other developers as having an easier time with a
save
> format description of MHTML than with complete-html-single-file.  And there's
> also the change in enum.  Would it make sense to change this to refer to
MHTML?

Done.

[benjhayden, 2012-04-16 19:23:53]

On 2012/04/16 19:08:05, benjhayden_chromium wrote:
> Hiding the extension broke deduplication. Looking for a simple fix.

select_file_dialog_impl_gtk.cc uses
gtk_file_chooser_set_do_overwrite_confirmation(), which suggests that there is
not a way to fix the overwrite confirmation without digging into gtk.

[benjhayden, 2012-04-16 19:37:28]

Hi John! It looks like you wrote the SavePackageFilePicker.
I'm trying to make it handle both 'html' and 'mht' and finding a
moving-carpet-bulge problem.
I think that the behavior that we want is for the suggested_path to change from
.html to .mht when the user chooses 'Complete, Single File' in the file-type
select box on the dialog. I am afraid that if it doesn't, then users will save
MHTML files as '.html', and chrome will render them incorrectly (as HTML instead
of MHTML).
Would you agree? If so, can you suggest how to do that?
Thanks!

[jam, 2012-04-17 05:00:13]

I actually just separated the code from one file to two (and one stayed in
chrome while the other moved to content). I suggest checking the change where I
did this to see where this code came from (I assume
chrome/browser/download/save_package.cc) and look through that svn log

[cbentzel, 2012-04-17 14:45:34]

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:60:
IDS_SAVE_PAGE_DESC_COMPLETE_SINGLE_FILE,
Would be nice to call this MHTML to be consistent - enums such as SavePageType
indicate MHTML, whereas the descriptor here references COMPLETE_SINGLE_FILE.

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:82: if
(can_save_as_complete) {
When is |can_save_as_complete| false? Should we be doing MHTML in that case?

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:93:
file_type_info.extensions.resize(kNumberExtensions);
This resize and direct indexing is kinda gross. Yeah, it existed before but it
would be nice to clean up by just doing push_back's into the data structures.
Seems less prone to problems as well.

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:154:
suggested_path.RemoveExtension(),
Why are you doing this here? Looks like different behavior from before.

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:209: FilePath mutable_path =
path;
Why is this done here? Why not done before preparing the data for SelectFile()?
It seems like this would override what the user is intending for the final path
to be.

http://codereview.chromium.org/10069014/diff/24001/content/browser/download/s...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10069014/diff/24001/content/browser/download/s...
content/browser/download/save_package.cc:321: download_->SetTotalBytes(1);
Could you SetTotalBytes at ::Init time instead? This would match the
non-complete save type.

http://codereview.chromium.org/10069014/diff/24001/content/browser/download/s...
File content/browser/download/save_package.h (right):

http://codereview.chromium.org/10069014/diff/24001/content/browser/download/s...
content/browser/download/save_package.h:124: void MHTMLGenerated(const FilePath&
path, int64 size);
Perhaps prefix with On to make it clear that this is a callback handler.

http://codereview.chromium.org/10069014/diff/24001/content/public/browser/sav...
File content/public/browser/save_page_type.h (right):

http://codereview.chromium.org/10069014/diff/24001/content/public/browser/sav...
content/public/browser/save_page_type.h:18: // User chose to save complete-html
page in a single text file.
Nit: I think you could just say as MHTML.

[benjhayden, 2012-04-17 15:48:44]

Looks like all of the changes to the file picker logic are "just" tiny, simple
changes, all the way back to 33628 (before initial.commit) by "jnd", in China.
http://chrome-corpsvn.mtv.corp.google.com/viewvc/trunk/chrome/browser/save_pa...

It doesn't seem like he thought terribly hard about the file picker UX. He
picked up win_util::SaveFileAsWithFilter() and kept going.
There's also the 12-hour time difference.
It looks to me like there is no file picker wisdom to be gained by engaging
anybody in particular.

[benjhayden, 2012-04-17 17:14:35]

Please hold the review while I try something.

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:60:
IDS_SAVE_PAGE_DESC_COMPLETE_SINGLE_FILE,
On 2012/04/17 14:45:34, cbentzel wrote:
> Would be nice to call this MHTML to be consistent - enums such as SavePageType
> indicate MHTML, whereas the descriptor here references COMPLETE_SINGLE_FILE.

Done.

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:82: if
(can_save_as_complete) {
On 2012/04/17 14:45:34, cbentzel wrote:
> When is |can_save_as_complete| false? Should we be doing MHTML in that case?

can_save_as_complete = ((page_mime == "text/html") || (page_mime ==
"application/xhtml+xml"))

Can you think of any other mimes that we might want to save as mhtml?

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:93:
file_type_info.extensions.resize(kNumberExtensions);
On 2012/04/17 14:45:34, cbentzel wrote:
> This resize and direct indexing is kinda gross. Yeah, it existed before but it
> would be nice to clean up by just doing push_back's into the data structures.
> Seems less prone to problems as well. 

If this used push_back and somebody re-ordered them, then FileSelected()'s use
of |index| would be broken.

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:154:
suggested_path.RemoveExtension(),
On 2012/04/17 14:45:34, cbentzel wrote:
> Why are you doing this here? Looks like different behavior from before.

Randy suggested doing this so that we could ensure that the extension for MHTML
files is 'mht'. Changing the save_type in the file picker dialog to mhtml does
not and can not change the extension of the suggested filename to mht, so it's
likely that users would accidentally save MHTML files with the default 'html'
extension. Chrome would not render such mis-named files correctly.
This is what gimp does when you first save new files, but not when you save
existing files.
I'm thinking about other ways of doing this, and I'm open to suggestions.

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:209: FilePath mutable_path =
path;
On 2012/04/17 14:45:34, cbentzel wrote:
> Why is this done here? Why not done before preparing the data for
SelectFile()?
> It seems like this would override what the user is intending for the final
path
> to be.

See my reply to your comment on L154 of this PS.

http://codereview.chromium.org/10069014/diff/24001/content/browser/download/s...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10069014/diff/24001/content/browser/download/s...
content/browser/download/save_package.cc:321: download_->SetTotalBytes(1);
On 2012/04/17 14:45:34, cbentzel wrote:
> Could you SetTotalBytes at ::Init time instead? This would match the
> non-complete save type.

Done.

http://codereview.chromium.org/10069014/diff/24001/content/browser/download/s...
File content/browser/download/save_package.h (right):

http://codereview.chromium.org/10069014/diff/24001/content/browser/download/s...
content/browser/download/save_package.h:124: void MHTMLGenerated(const FilePath&
path, int64 size);
On 2012/04/17 14:45:34, cbentzel wrote:
> Perhaps prefix with On to make it clear that this is a callback handler. 

Done.

http://codereview.chromium.org/10069014/diff/24001/content/public/browser/sav...
File content/public/browser/save_page_type.h (right):

http://codereview.chromium.org/10069014/diff/24001/content/public/browser/sav...
content/public/browser/save_page_type.h:18: // User chose to save complete-html
page in a single text file.
On 2012/04/17 14:45:34, cbentzel wrote:
> Nit: I think you could just say as MHTML. 

Done.

[benjhayden, 2012-04-17 19:36:19]

PTAL

[benjhayden, 2012-04-17 20:44:10]

After much discussion, the plan seems to be to make
SavePackageFilePickerChromeOS use SavePackage (which supports html-only,
html-complete, and mhtml) in this CL and keep mhtml behind a flag for the other
platforms until we can iron out some wrinkles in the SelectFileDialogImpls. Once
all the SelectFileDialogImpls correctly support switching save-as file formats
and extensions, we can remove the flag and users will be able to save html pages
as their choice of html-only, html-complete, and mhtml. Until then, the ui
remains unchanged unless users flip --save-as-mhtml, which disables html-only
and html-complete, and enables mhtml.
Please speak up if that sounds incorrect or incomplete.

[cbentzel, 2012-04-17 20:47:40]

This sounds good to me.

On Tue, Apr 17, 2012 at 4:44 PM, <benjhayden@chromium.org> wrote:

> After much discussion, the plan seems to be to make
> SavePackageFilePickerChromeOS use SavePackage (which supports html-only,
> html-complete, and mhtml) in this CL and keep mhtml behind a flag for the
> other
> platforms until we can iron out some wrinkles in the
> SelectFileDialogImpls. Once
> all the SelectFileDialogImpls correctly support switching save-as file
> formats
> and extensions, we can remove the flag and users will be able to save html
> pages
> as their choice of html-only, html-complete, and mhtml. Until then, the ui
> remains unchanged unless users flip --save-as-mhtml, which disables
> html-only
> and html-complete, and enables mhtml.
> Please speak up if that sounds incorrect or incomplete.
>
>
http://codereview.chromium.**org/10069014/<http://codereview.chromium.org/100...
>

[cbentzel, 2012-04-17 20:53:33]

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/24001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:93:
file_type_info.extensions.resize(kNumberExtensions);
On 2012/04/17 17:14:36, benjhayden_chromium wrote:
> On 2012/04/17 14:45:34, cbentzel wrote:
> > This resize and direct indexing is kinda gross. Yeah, it existed before but
it
> > would be nice to clean up by just doing push_back's into the data
structures.
> > Seems less prone to problems as well. 
> 
> If this used push_back and somebody re-ordered them, then FileSelected()'s use
> of |index| would be broken.

I'm not too worried about that case, but it is reasonable. Could you at least
use a local to have the 0-based index to make this clearer?

http://codereview.chromium.org/10069014/diff/14005/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/14005/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:88: if (ShouldSaveAsMHTML())
{
So you want to support MHTML only _or_ HTML/HTML-complete only? Why not MTHML in
addition to HTML/HTML-complete?

[Randy Smith (Not in Mondays), 2012-04-18 18:00:55]

The biggest question I have is how this handles the issue of delaying apparent
completion of the download item until the upload has completed.  That took some
work on the download side, and I suspect it'll take some work here.  Achuith,
Asanka, I think you're both better people than I to judge this issue; any
thoughts?

Beyond that, it looks good modulo the nits below.  I'm going to delay my stamp
until I understand the above issue, but it's looking good.

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:81: FilePath::StringType
default_extension = default_extension_const;
Why do this instead of just converting the parameters to be non-const non-ref? 
It's less code, and I think it means the same thing.

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:410:
command_line->AppendSwitch(switches::kSavePageAsMHTML);
Don't you need to switch this back at the end of the test, lest you change state
for tests following this one?

[benjhayden, 2012-04-18 18:11:18]

http://codereview.chromium.org/10069014/diff/14005/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/14005/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:88: if (ShouldSaveAsMHTML())
{
On 2012/04/17 20:53:33, cbentzel wrote:
> So you want to support MHTML only _or_ HTML/HTML-complete only? Why not MTHML
in
> addition to HTML/HTML-complete?

Commented.

[benjhayden, 2012-04-18 18:19:01]

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:410:
command_line->AppendSwitch(switches::kSavePageAsMHTML);
On 2012/04/18 18:00:55, rdsmith wrote:
> Don't you need to switch this back at the end of the test, lest you change
state
> for tests following this one?

It doesn't look like any other tests do that, and InProcessBrowserTest doesn't
mention it.
If anybody makes any more SavePageAsMHTMLBrowserTest TEST_Fs, then they'll still
want the switch. After the last SavePageAsMHTMLBrowserTest runs, it will shut
down its browser, taking this switch with it.

[asanka, 2012-04-18 18:34:59]

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:39:
callback_.Run(path, content::SAVE_PAGE_TYPE_AS_MHTML);
How does the file get uploaded to GData if it was meant to be a GData upload?

[benjhayden, 2012-04-18 18:38:28]

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:39:
callback_.Run(path, content::SAVE_PAGE_TYPE_AS_MHTML);
On 2012/04/18 18:34:59, asanka wrote:
> How does the file get uploaded to GData if it was meant to be a GData upload?

My understanding is that there's a DownloadManager::Observer that looks out for
downloads to the gdata mountpoint and takes them over.

[asanka, 2012-04-18 18:50:24]

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:39:
callback_.Run(path, content::SAVE_PAGE_TYPE_AS_MHTML);
On 2012/04/18 18:38:28, benjhayden_chromium wrote:
> On 2012/04/18 18:34:59, asanka wrote:
> > How does the file get uploaded to GData if it was meant to be a GData
upload?
> 
> My understanding is that there's a DownloadManager::Observer that looks out
for
> downloads to the gdata mountpoint and takes them over.

It's not automatic. See DownloadFilePickerChromeOS::FileSelected()
(chrome/browser/download/download_file_picker_chromeos.cc). You have to:
- set the GData path using GDataDownloadObserver::SetGDataPath() on the
DownloadItem.
- get a temporary download path for the download.
- set the display name of the download since the filename is now a temporary
name.

This might be made more complicated for the SavePage case because the
DownloadItem isn't created until later.

[achuithb, 2012-04-19 20:07:26]

FYI, the http access issue of mhtml files is being worked on:
http://code.google.com/p/chromium-os/issues/detail?id=28654

[benjhayden, 2012-04-23 16:16:29]

PTAL

[benjhayden, 2012-04-23 16:49:45]

Apologies, it seems that I still have some things to figure out in the chromeos
file picker.
Feel free to review everything besides save_package_file_picker_chromeos.

[benjhayden, 2012-04-23 18:32:04]

PTAL
gdata seems to be working now.

[Randy Smith (Not in Mondays), 2012-04-24 17:23:54]

Could you give a quick sketch of how you're handling the delaying of the
download completion until the upload happens?  Asanka's the expert here, so I'm
tempted to just piggy back on his stamp, but I would have expected to see code
in save_package.cc for that, and I didn't.

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:81: FilePath::StringType
default_extension = default_extension_const;
On 2012/04/18 18:00:55, rdsmith wrote:
> Why do this instead of just converting the parameters to be non-const non-ref?

> It's less code, and I think it means the same thing.

I didn't see an answer to this question?

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:410:
command_line->AppendSwitch(switches::kSavePageAsMHTML);
On 2012/04/18 18:19:02, benjhayden_chromium wrote:
> On 2012/04/18 18:00:55, rdsmith wrote:
> > Don't you need to switch this back at the end of the test, lest you change
> state
> > for tests following this one?
> 
> It doesn't look like any other tests do that, and InProcessBrowserTest doesn't
> mention it.
> If anybody makes any more SavePageAsMHTMLBrowserTest TEST_Fs, then they'll
still
> want the switch. After the last SavePageAsMHTMLBrowserTest runs, it will shut
> down its browser, taking this switch with it.

Actually, now that I get my memory system working, I think a new process is
created for each separate test (i.e. SavePageAsMHTML, etc.)  So yeah, this is
fine.

[benjhayden, 2012-04-24 18:11:41]

On 2012/04/24 17:23:54, rdsmith wrote:
> Could you give a quick sketch of how you're handling the delaying of the
> download completion until the upload happens?  Asanka's the expert here, so
I'm
> tempted to just piggy back on his stamp, but I would have expected to see code
> in save_package.cc for that, and I didn't.

I'd assumed that the GDataDownloadObserver would take care of it, but it looks
like it only gets me halfway.
ChromeDownloadManagerDelegate::ShouldCompleteDownload() calls
gdata::GDataDownloadObserver::IsReadyToComplete().

I'm going to try waiting for SavePackage::OnDownloadUpdated().

> 
>
http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
> File chrome/browser/download/save_package_file_picker.cc (right):
> 
>
http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
> chrome/browser/download/save_package_file_picker.cc:81: FilePath::StringType
> default_extension = default_extension_const;
> On 2012/04/18 18:00:55, rdsmith wrote:
> > Why do this instead of just converting the parameters to be non-const
non-ref?
> 
> > It's less code, and I think it means the same thing.
> 
> I didn't see an answer to this question?
> 
>
http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
> File chrome/browser/download/save_page_browsertest.cc (right):
> 
>
http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
> chrome/browser/download/save_page_browsertest.cc:410:
> command_line->AppendSwitch(switches::kSavePageAsMHTML);
> On 2012/04/18 18:19:02, benjhayden_chromium wrote:
> > On 2012/04/18 18:00:55, rdsmith wrote:
> > > Don't you need to switch this back at the end of the test, lest you change
> > state
> > > for tests following this one?
> > 
> > It doesn't look like any other tests do that, and InProcessBrowserTest
doesn't
> > mention it.
> > If anybody makes any more SavePageAsMHTMLBrowserTest TEST_Fs, then they'll
> still
> > want the switch. After the last SavePageAsMHTMLBrowserTest runs, it will
shut
> > down its browser, taking this switch with it.
> 
> Actually, now that I get my memory system working, I think a new process is
> created for each separate test (i.e. SavePageAsMHTML, etc.)  So yeah, this is
> fine.

[benjhayden, 2012-04-24 18:11:53]

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:81: FilePath::StringType
default_extension = default_extension_const;
On 2012/04/18 18:00:55, rdsmith wrote:
> Why do this instead of just converting the parameters to be non-const non-ref?

> It's less code, and I think it means the same thing.

Done.

http://codereview.chromium.org/10069014/diff/38001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:81: FilePath::StringType
default_extension = default_extension_const;
On 2012/04/24 17:23:54, rdsmith wrote:
> On 2012/04/18 18:00:55, rdsmith wrote:
> > Why do this instead of just converting the parameters to be non-const
non-ref?
> 
> > It's less code, and I think it means the same thing.
> 
> I didn't see an answer to this question?

Done.

[achuithb, 2012-04-24 22:06:09]

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.h (right):

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.h:19: FilePath suggested_path,
I don't think you should do this. If you need to modify this value, I'd prefer
creating a local temporary instead.

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:23: void
OnSavePackageDownloadCreated(
Could you add a function comment explaining that what this is a callback for and
why it's necessary?

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:24: const FilePath&
true_path,
Could we call this gdata_path instead?

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:31: void
ContinueSettingUpGDataDownload(
Function comment please.

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:77: //
GetGDataTempDownloadPath ->
This is a bit cryptic. A line explaining that this is the sequence of events,
etc.

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker_chromeos.h (right):

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.h:26:
content::SaveFilePathPickedCallback callback);
I would pass in a const reference.

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:371: #if !defined(OS_CHROMEOS)
Why does this not work on chromeos?

[achuithb, 2012-04-24 22:26:05]

http://codereview.chromium.org/10069014/diff/75001/content/browser/download/s...
File content/browser/download/save_package.h (right):

http://codereview.chromium.org/10069014/diff/75001/content/browser/download/s...
content/browser/download/save_package.h:94: bool
Init(content::SaveFileDownloadCreatedCallback download_created_callback);
I'd use a const reference. Also, if you could please update the function comment
to note the callback.

http://codereview.chromium.org/10069014/diff/75001/content/browser/download/s...
content/browser/download/save_package.h:197:
content::SaveFileDownloadCreatedCallback download_created_callback);
const ref

http://codereview.chromium.org/10069014/diff/75001/content/public/browser/dow...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/75001/content/public/browser/dow...
content/public/browser/download_manager_delegate.h:22: typedef
base::Callback<void(DownloadItem*)>
comments explaining what these callbacks are for, please.

[achuithb, 2012-04-24 22:28:14]

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.h (right):

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.h:19: FilePath suggested_path,
On 2012/04/24 22:06:09, achuith.bhandarkar wrote:
> I don't think you should do this. If you need to modify this value, I'd prefer
> creating a local temporary instead.

Just saw Randy's note. Personally not a fan of function arguments that are not
either const references (in params), or pointers (out params), but this is fine.
Sorry for the trouble.

[achuithb, 2012-04-24 22:30:06]

I was mostly interested in SavePackageFilePickerChromeOS, and it's looking good.
Have you had a chance to test it out to make sure it works?

[benjhayden, 2012-04-26 15:05:57]

PTAL
The test now works on both cros and non-cros.

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:23: void
OnSavePackageDownloadCreated(
On 2012/04/24 22:06:09, achuith.bhandarkar wrote:
> Could you add a function comment explaining that what this is a callback for
and
> why it's necessary?

Done.

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:24: const FilePath&
true_path,
On 2012/04/24 22:06:09, achuith.bhandarkar wrote:
> Could we call this gdata_path instead?

Done.

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:31: void
ContinueSettingUpGDataDownload(
On 2012/04/24 22:06:09, achuith.bhandarkar wrote:
> Function comment please.

Done.

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.cc:77: //
GetGDataTempDownloadPath ->
On 2012/04/24 22:06:09, achuith.bhandarkar wrote:
> This is a bit cryptic. A line explaining that this is the sequence of events,
> etc.

Done.

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker_chromeos.h (right):

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker_chromeos.h:26:
content::SaveFilePathPickedCallback callback);
On 2012/04/24 22:06:09, achuith.bhandarkar wrote:
> I would pass in a const reference.

Done.

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10069014/diff/75001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:371: #if !defined(OS_CHROMEOS)
On 2012/04/24 22:06:09, achuith.bhandarkar wrote:
> Why does this not work on chromeos?

Done.

http://codereview.chromium.org/10069014/diff/75001/content/browser/download/s...
File content/browser/download/save_package.h (right):

http://codereview.chromium.org/10069014/diff/75001/content/browser/download/s...
content/browser/download/save_package.h:94: bool
Init(content::SaveFileDownloadCreatedCallback download_created_callback);
On 2012/04/24 22:26:05, achuith.bhandarkar wrote:
> I'd use a const reference. Also, if you could please update the function
comment
> to note the callback.

Done.

http://codereview.chromium.org/10069014/diff/75001/content/browser/download/s...
content/browser/download/save_package.h:197:
content::SaveFileDownloadCreatedCallback download_created_callback);
On 2012/04/24 22:26:05, achuith.bhandarkar wrote:
> const ref

Done.

http://codereview.chromium.org/10069014/diff/75001/content/public/browser/dow...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/75001/content/public/browser/dow...
content/public/browser/download_manager_delegate.h:22: typedef
base::Callback<void(DownloadItem*)>
On 2012/04/24 22:26:05, achuith.bhandarkar wrote:
> comments explaining what these callbacks are for, please.

Done.

[asanka, 2012-04-26 16:06:19]

http://codereview.chromium.org/10069014/diff/87005/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10069014/diff/87005/chrome/browser/about_flags...
chrome/browser/about_flags.cc:405: "save-page-as-mhtml",  // FLAGS::RECORD_UMA
Don't forget to run extract_actions.py

http://codereview.chromium.org/10069014/diff/87005/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.h (left):

http://codereview.chromium.org/10069014/diff/87005/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.h:20: const FilePath&
suggested_path,
Why do this? This will trigger a Coverity warning.

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (left):

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
content/browser/download/download_manager_impl.cc:596: bool
DownloadManagerImpl::IsDownloadReadyForCompletion(DownloadItem* download) {
MaybeCompleteDownload() requires that the download be in active_downloads_
(checked in DownloadManagerImpl::IsDownloadReadyForCompletion). The purpose of
the check is to verify that the download hasn't been canceled/interrupted.

You would want to change that check to also check the save_page_downloads_
queue. That way you can get rid of the UpdateObservers() below and the
additional completion pathway. You might have to change a few of the DCHECKs()
in DownloadManagerImpl() because I think these downloads don't end up on the
in_progress list either.

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
content/browser/download/download_manager_impl.cc:596: bool
DownloadManagerImpl::IsDownloadReadyForCompletion(DownloadItem* download) {
Also add a DCHECK() here to verify that a completing save_page_downloads_ item
doesn't need a final rename (i.e. DI::NeedsRename() is false).

[asanka, 2012-04-26 16:11:28]

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (left):

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
content/browser/download/download_manager_impl.cc:596: bool
DownloadManagerImpl::IsDownloadReadyForCompletion(DownloadItem* download) {
On 2012/04/26 16:06:20, asanka wrote:
> That way you can get rid of the UpdateObservers() below and the
> additional completion pathway.

I'm referring to the additional UpdateObsevers() in
GDataDownloadObserver::OnUploadComplete and the extra completion logic in
SavePackage::OnDownloadUpdated.

[benjhayden, 2012-04-26 16:33:36]

http://codereview.chromium.org/10069014/diff/87005/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.h (left):

http://codereview.chromium.org/10069014/diff/87005/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.h:20: const FilePath&
suggested_path,
On 2012/04/26 16:06:20, asanka wrote:
> Why do this? This will trigger a Coverity warning.

Which/what kind of warning? Can you elaborate?

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (left):

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
content/browser/download/download_manager_impl.cc:596: bool
DownloadManagerImpl::IsDownloadReadyForCompletion(DownloadItem* download) {
On 2012/04/26 16:06:20, asanka wrote:
> MaybeCompleteDownload() requires that the download be in active_downloads_
> (checked in DownloadManagerImpl::IsDownloadReadyForCompletion). The purpose of
> the check is to verify that the download hasn't been canceled/interrupted.
> 
> You would want to change that check to also check the save_page_downloads_
> queue. That way you can get rid of the UpdateObservers() below and the
> additional completion pathway. You might have to change a few of the DCHECKs()
> in DownloadManagerImpl() because I think these downloads don't end up on the
> in_progress list either.

You're suggesting that SavePackage not go through Finish()? That seems like it
will require quite a lot more work.
Is there a specific reason that save pages should go through
MaybeCompleteDownload()?

[asanka, 2012-04-26 18:15:25]

http://codereview.chromium.org/10069014/diff/87005/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.h (left):

http://codereview.chromium.org/10069014/diff/87005/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.h:20: const FilePath&
suggested_path,
On 2012/04/26 16:33:36, benjhayden_chromium wrote:
> On 2012/04/26 16:06:20, asanka wrote:
> > Why do this? This will trigger a Coverity warning.
> 
> Which/what kind of warning? Can you elaborate?

The warning is "Big parameter passed by value". Grep for "pass by value" through
the commit logs and you'll see people converting these to const refs.

The fact that this might trigger a warning, by itself, isn't a reason to avoid
passing by value. But chances are people are going to see those warnings and
they are going to spend time trying to address it.

If you really want to go this route, I think it would be helpful to have a
comment here explaining why.

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (left):

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
content/browser/download/download_manager_impl.cc:596: bool
DownloadManagerImpl::IsDownloadReadyForCompletion(DownloadItem* download) {
On 2012/04/26 16:33:36, benjhayden_chromium wrote:
> On 2012/04/26 16:06:20, asanka wrote:
> > MaybeCompleteDownload() requires that the download be in active_downloads_
> > (checked in DownloadManagerImpl::IsDownloadReadyForCompletion). The purpose
of
> > the check is to verify that the download hasn't been canceled/interrupted.
> > 
> > You would want to change that check to also check the save_page_downloads_
> > queue. That way you can get rid of the UpdateObservers() below and the
> > additional completion pathway. You might have to change a few of the
DCHECKs()
> > in DownloadManagerImpl() because I think these downloads don't end up on the
> > in_progress list either.
> 
> You're suggesting that SavePackage not go through Finish()? That seems like it
> will require quite a lot more work.
> Is there a specific reason that save pages should go through
> MaybeCompleteDownload()?

I was actually under the impression that you were trying to do that for the
GData case.

With this CL, GData SavePackage downloads already go through
MaybeCompleteDownload(). Then they are implicitly turned away at
IsDownloadReadyForCompletion(). The GDataDownloadObserver::OnUploadComplete()
then issues an extra UpdateObservers() call which causes the
SavePackage::OnDownloadUpdated() handler to complete the download.

Fixing MaybeCompleteDownload() might help avoid this extra completion pathway.
Or perhaps you can make the alternate pathway more explicit.

[benjhayden, 2012-04-26 19:00:47]

http://codereview.chromium.org/10069014/diff/87005/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.h (left):

http://codereview.chromium.org/10069014/diff/87005/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.h:20: const FilePath&
suggested_path,
On 2012/04/26 18:15:25, asanka wrote:
> On 2012/04/26 16:33:36, benjhayden_chromium wrote:
> > On 2012/04/26 16:06:20, asanka wrote:
> > > Why do this? This will trigger a Coverity warning.
> > 
> > Which/what kind of warning? Can you elaborate?
> 
> The warning is "Big parameter passed by value". Grep for "pass by value"
through
> the commit logs and you'll see people converting these to const refs.
> 
> The fact that this might trigger a warning, by itself, isn't a reason to avoid
> passing by value. But chances are people are going to see those warnings and
> they are going to spend time trying to address it.
> 
> If you really want to go this route, I think it would be helpful to have a
> comment here explaining why.

Done.

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (left):

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
content/browser/download/download_manager_impl.cc:596: bool
DownloadManagerImpl::IsDownloadReadyForCompletion(DownloadItem* download) {
On 2012/04/26 18:15:25, asanka wrote:
> On 2012/04/26 16:33:36, benjhayden_chromium wrote:
> > On 2012/04/26 16:06:20, asanka wrote:
> > > MaybeCompleteDownload() requires that the download be in active_downloads_
> > > (checked in DownloadManagerImpl::IsDownloadReadyForCompletion). The
purpose
> of
> > > the check is to verify that the download hasn't been canceled/interrupted.
> > > 
> > > You would want to change that check to also check the save_page_downloads_
> > > queue. That way you can get rid of the UpdateObservers() below and the
> > > additional completion pathway. You might have to change a few of the
> DCHECKs()
> > > in DownloadManagerImpl() because I think these downloads don't end up on
the
> > > in_progress list either.
> > 
> > You're suggesting that SavePackage not go through Finish()? That seems like
it
> > will require quite a lot more work.
> > Is there a specific reason that save pages should go through
> > MaybeCompleteDownload()?
> 
> I was actually under the impression that you were trying to do that for the
> GData case.

Ah, nope, sorry.
The fact that MaybeCompleteDownload() runs for gdata SavePackages is an artifact
of the interaction with the GDataDownloadObserver, not intentional functional
change.


> 
> With this CL, GData SavePackage downloads already go through
> MaybeCompleteDownload(). Then they are implicitly turned away at
> IsDownloadReadyForCompletion(). The GDataDownloadObserver::OnUploadComplete()
> then issues an extra UpdateObservers() call which causes the
> SavePackage::OnDownloadUpdated() handler to complete the download.
> 
> Fixing MaybeCompleteDownload() might help avoid this extra completion pathway.
> Or perhaps you can make the alternate pathway more explicit.

The "alternate"/"extra completion" pathway is what SavePackage has already been
doing: using its own Finish() instead of MaybeCompleteDownload(). I'm trying to
preserve existing pathways instead of switch to new ones -- this cl is already a
little too big.

I'll add a comment to IsDownloadReadyForCompletion() about GData SavePackages.
Is there anything else that I can do to make the situation more explicit?

[asanka, 2012-04-26 19:20:26]

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (left):

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
content/browser/download/download_manager_impl.cc:596: bool
DownloadManagerImpl::IsDownloadReadyForCompletion(DownloadItem* download) {
On 2012/04/26 19:00:48, benjhayden_chromium wrote:
> I'll add a comment to IsDownloadReadyForCompletion() about GData SavePackages.
> Is there anything else that I can do to make the situation more explicit?

Thanks!

A short note in GDataDownloadObserver::OnUploadCompleted() explaining that:
- MaybeCompleteDownload() only works for regular downloads.
- SavePackage downloads are routed through the SavePackage::OnDownloadUpdated()
handler, which ends up calling SP::Finish()

... would be helpful.

[benjhayden, 2012-04-26 20:09:30]

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (left):

http://codereview.chromium.org/10069014/diff/87005/content/browser/download/d...
content/browser/download/download_manager_impl.cc:596: bool
DownloadManagerImpl::IsDownloadReadyForCompletion(DownloadItem* download) {
On 2012/04/26 19:20:26, asanka wrote:
> On 2012/04/26 19:00:48, benjhayden_chromium wrote:
> > I'll add a comment to IsDownloadReadyForCompletion() about GData
SavePackages.
> > Is there anything else that I can do to make the situation more explicit?
> 
> Thanks!
> 
> A short note in GDataDownloadObserver::OnUploadCompleted() explaining that:
> - MaybeCompleteDownload() only works for regular downloads.
> - SavePackage downloads are routed through the
SavePackage::OnDownloadUpdated()
> handler, which ends up calling SP::Finish()
> 
> ... would be helpful.

PTAL, Thanks!

[cbentzel, 2012-04-26 20:35:16]

I'm going to remove myself as a reviewer (again). It looks like you have all the
correct people reviewing this and I want to make sure I don't hold it up.

http://codereview.chromium.org/10069014/diff/99004/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10069014/diff/99004/chrome/browser/about_flags...
chrome/browser/about_flags.cc:408: kOsAll,
Is this supported on Android?

http://codereview.chromium.org/10069014/diff/99004/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/99004/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:70: return
can_save_as_complete_ &&
Nit: could you just pass this in as an argument rather than adding state to the
FilePicker object?

[benjhayden, 2012-04-26 20:49:44]

http://codereview.chromium.org/10069014/diff/99004/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10069014/diff/99004/chrome/browser/about_flags...
chrome/browser/about_flags.cc:408: kOsAll,
On 2012/04/26 20:35:16, cbentzel wrote:
> Is this supported on Android?

Done.

http://codereview.chromium.org/10069014/diff/99004/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/99004/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:70: return
can_save_as_complete_ &&
On 2012/04/26 20:35:16, cbentzel wrote:
> Nit: could you just pass this in as an argument rather than adding state to
the
> FilePicker object?

This is used in the FileSelected() method. I could pass it along via |void*
params|, but this seems cleaner.

[achuithb, 2012-04-26 21:01:45]

http://codereview.chromium.org/10069014/diff/99004/chrome/browser/download/sa...
File chrome/browser/download/save_package_file_picker.cc (right):

http://codereview.chromium.org/10069014/diff/99004/chrome/browser/download/sa...
chrome/browser/download/save_package_file_picker.cc:70: return
can_save_as_complete_ &&
On 2012/04/26 20:49:44, benjhayden_chromium wrote:
> On 2012/04/26 20:35:16, cbentzel wrote:
> > Nit: could you just pass this in as an argument rather than adding state to
> the
> > FilePicker object?
> 
> This is used in the FileSelected() method. I could pass it along via |void*
> params|, but this seems cleaner.

Please don't use the void* params. I think they are currently unused, and we
should get rid of them altogether.

[asanka, 2012-04-27 16:00:55]

https://chromiumcodereview.appspot.com/10069014/diff/95007/content/browser/do...
File content/browser/download/save_package.cc (right):

https://chromiumcodereview.appspot.com/10069014/diff/95007/content/browser/do...
content/browser/download/save_package.cc:1358: Finish();
Note that this does a state transition on the DownloadItem while it's in the
middle of UpdateObsevers(). This causes a nested UpdateObservers() to run
causing SavePackage::OnDownloadUpdated() to re-enter.

This can be problematic. Can we post a task to call Finish() instead?

[benjhayden, 2012-04-27 17:28:52]

I had thought that checking download->IsCompleted() would prevent re-entering
Finish(), but I hadn't thought that re-entering OnDownloadUpdated might cause
problems for other observers or DownloadItemImpl itself. Good catch!

https://chromiumcodereview.appspot.com/10069014/diff/95007/content/browser/do...
File content/browser/download/save_package.cc (right):

https://chromiumcodereview.appspot.com/10069014/diff/95007/content/browser/do...
content/browser/download/save_package.cc:1358: Finish();
On 2012/04/27 16:00:55, asanka wrote:
> Note that this does a state transition on the DownloadItem while it's in the
> middle of UpdateObsevers(). This causes a nested UpdateObservers() to run
> causing SavePackage::OnDownloadUpdated() to re-enter.
> 
> This can be problematic. Can we post a task to call Finish() instead?

Done.

[Randy Smith (Not in Mondays), 2012-04-27 18:24:04]

Several points of concern over use of callbacks.  Feel free to grab me and a
whiteboard if either I'm confused (very possible, and in one case, certain) or I
haven't made any sense.

http://codereview.chromium.org/10069014/diff/95007/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/95007/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.h:93: #endif
It looks like CDMD has to date only used ifdefs to null out subsections of code
rather than whole function APIs.  If Noe' et al have an opinion, it should be
followed, but in the absence of this are you willing to declare this function
but make its behavior null if Safe browsing isn't defined, similar to
GetDownloadProtectionService?

http://codereview.chromium.org/10069014/diff/95007/content/browser/download/d...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10069014/diff/95007/content/browser/download/d...
content/browser/download/download_item_impl.h:108: const std::string& mime_type,
How is this used?  I dug around and couldn't figure it out (at least not
easily).

http://codereview.chromium.org/10069014/diff/95007/content/browser/download/s...
File content/browser/download/save_package.h (right):

http://codereview.chromium.org/10069014/diff/95007/content/browser/download/s...
content/browser/download/save_package.h:94: // |download_created_callback| will
be called when the DownloadItem is
nit: There is no entity named download_created_callback in the below
declaration.

http://codereview.chromium.org/10069014/diff/95007/content/public/browser/dow...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/95007/content/public/browser/dow...
content/public/browser/download_manager_delegate.h:25:
SaveFileDownloadCreatedCallback;
Nit: There are many places where we save files (e.g. Save Link As ...) but this
is only used for SavePackage; I'd use SavePackage in its names rather than
SaveFile.

http://codereview.chromium.org/10069014/diff/107001/chrome/browser/chromeos/g...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10069014/diff/107001/chrome/browser/chromeos/g...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:342:
download->MaybeCompleteDownload();
So this really makes me twitch--we're calling into a routine that's arguable an
implementation detail of the download item, and we're doing so that knowing in
one of the two cases in question it won't do anything (in fact, it makes no
sense and we have to do something else).  Can we change this flow so that
"DMD::ShouldCompleteDownload" is passed a callback as appropriate to the
situation that is then called when completion should be checked for again and
avoid this hackery?

http://codereview.chromium.org/10069014/diff/107001/chrome/browser/download/s...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/10069014/diff/107001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker_chromeos.cc:109:
base::Bind(&ContinueSettingUpGDataDownload,
What do you get by doing the trampoline rather than going directly?

http://codereview.chromium.org/10069014/diff/107001/chrome/tools/chromeaction...
File chrome/tools/chromeactions.txt (right):

http://codereview.chromium.org/10069014/diff/107001/chrome/tools/chromeaction...
chrome/tools/chromeactions.txt:1: 0x5d4affbdc9e636fe	AboutChrome
What are the changes in this file about?  What's reviewing them?  They're
certainly greek to me.

http://codereview.chromium.org/10069014/diff/107001/content/browser/download/...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10069014/diff/107001/content/browser/download/...
content/browser/download/download_manager_impl.cc:636: // complete.
Is there a way to do this via callback so that we don't have to be resilient to
a pretty arbitrary call to a random method (at least, it's random in the
SavePackage context).

http://codereview.chromium.org/10069014/diff/107001/content/browser/download/...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10069014/diff/107001/content/browser/download/...
content/browser/download/save_package.cc:1367: base::Bind(&SavePackage::Finish,
this));
This makes me twitchy--a complicated conditional, a PostTask(), and a boolean to
handle accidental calling repeats makes me want to look for a better way.  I
*think* the suggestion I made elsewhere of passing a callback to
DMD::ShouldCompleteDownload() will render this unnecessary, as that callback can
just call SavePackage::Finish() directly.  If not, I'd like to come back here
(or for you to come back here, or for us to come back here) and see if there's
some other way to think about this problem that would lead to simpler code.

http://codereview.chromium.org/10069014/diff/107001/content/public/browser/do...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/107001/content/public/browser/do...
content/public/browser/download_manager_delegate.h:25:
SaveFileDownloadCreatedCallback;
So I had a heck of a time figuring out how this thing was used ("A callback
taking a callback passed through a callback?  We must go deeper." :-}) so I'd
rather not do it if there's some other way to arrange it.   If I'm reading the
code correctly (which I am *not* confident of, which is the basic problem :-})
it looks as if this callback exists:
* On Non-Cros: to disable safe browsing on this particular download.
* On Cros: To set the display name, and set it to temporary.

Why does this need to happen in a callback created by the file picker?  The file
picker should pick the file (and that's pretty much it).  Can we just put these
in a conditional, maybe ifdef'd, in SavePackage where this type of logic more
arguably belongs?  It'll save future maintainers (including possibly yourself)
headache and blood pressure med costs.

[Randy Smith (Not in Mondays), 2012-04-27 18:25:20]

Sorry, one other thing: At some point we need to look deeper into why our HTML
isn't being understood by other browsers.  I don't think that needs to be for
this CL, but I would like it not to be completely dropped on the floor.  Could
you file a bug (or more than one, if you think that's appropriate) on the issues
you raise in the CL description?

[benjhayden, 2012-04-28 19:09:03]

My linux desktop is turned off for the weekend so I'll handle the comments
requesting changes on Monday.

Also, see http://crbug.com/125477

http://codereview.chromium.org/10069014/diff/95007/content/browser/download/d...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10069014/diff/95007/content/browser/download/d...
content/browser/download/download_item_impl.h:108: const std::string& mime_type,
On 2012/04/27 18:24:04, rdsmith wrote:
> How is this used?  I dug around and couldn't figure it out (at least not
> easily).

GDataUploader::UploadFile() requires it to be not empty.

http://codereview.chromium.org/10069014/diff/107001/chrome/browser/chromeos/g...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/10069014/diff/107001/chrome/browser/chromeos/g...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:342:
download->MaybeCompleteDownload();
On 2012/04/27 18:24:04, rdsmith wrote:
> So this really makes me twitch--we're calling into a routine that's arguable
an
> implementation detail of the download item, and we're doing so that knowing in
> one of the two cases in question it won't do anything (in fact, it makes no
> sense and we have to do something else).  Can we change this flow so that
> "DMD::ShouldCompleteDownload" is passed a callback as appropriate to the
> situation that is then called when completion should be checked for again and
> avoid this hackery?

I'm not sure that I completely understand your proposal, but it sounds like a
non-trivial change.
Could we talk on Monday about doing this in another CL?

http://codereview.chromium.org/10069014/diff/107001/chrome/browser/download/s...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/10069014/diff/107001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker_chromeos.cc:109:
base::Bind(&ContinueSettingUpGDataDownload,
On 2012/04/27 18:24:04, rdsmith wrote:
> What do you get by doing the trampoline rather than going directly?

The clarity of purpose.
The flexibility to do something in ContinueSettingUpGDataDownload in addition to
trampolining.
Also I'm not entirely sure that I can spell it without
ContinueSettingUpGDataDownload(). There's a point dereference that I'm not sure
I can convince base::Bind to do *after* the callback is run instead of *before*.

http://codereview.chromium.org/10069014/diff/107001/chrome/tools/chromeaction...
File chrome/tools/chromeactions.txt (right):

http://codereview.chromium.org/10069014/diff/107001/chrome/tools/chromeaction...
chrome/tools/chromeactions.txt:1: 0x5d4affbdc9e636fe	AboutChrome
On 2012/04/27 18:24:04, rdsmith wrote:
> What are the changes in this file about?  What's reviewing them?  They're
> certainly greek to me.

Greek to me, too. I just ran extract_actions.py as Asanka suggested. I was
hoping he would let me know if I did it wrong.

http://codereview.chromium.org/10069014/diff/107001/content/browser/download/...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10069014/diff/107001/content/browser/download/...
content/browser/download/download_manager_impl.cc:636: // complete.
On 2012/04/27 18:24:04, rdsmith wrote:
> Is there a way to do this via callback so that we don't have to be resilient
to
> a pretty arbitrary call to a random method (at least, it's random in the
> SavePackage context).

SavePackage could pass base::Bind(&SavePackage::MaybeFinish) to
delegate()->ChooseSavePath(), CDMD could give that callback to
GDataDownloadObserver, GDataDownloadObserver could store that callback in a map.

I thought that adding a map to GDataDownloadObserver would be unnecessarily
heavy and sketchy in an already heavy, sketchy CL, but I can try it if you want.

http://codereview.chromium.org/10069014/diff/107001/content/public/browser/do...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/107001/content/public/browser/do...
content/public/browser/download_manager_delegate.h:25:
SaveFileDownloadCreatedCallback;
On 2012/04/27 18:24:04, rdsmith wrote:
> So I had a heck of a time figuring out how this thing was used ("A callback
> taking a callback passed through a callback?  We must go deeper." :-}) so I'd
> rather not do it if there's some other way to arrange it.   If I'm reading the
> code correctly (which I am *not* confident of, which is the basic problem :-})
> it looks as if this callback exists:
> * On Non-Cros: to disable safe browsing on this particular download.
> * On Cros: To set the display name, and set it to temporary.
> 
> Why does this need to happen in a callback created by the file picker?  The
file
> picker should pick the file (and that's pretty much it).  Can we just put
these
> in a conditional, maybe ifdef'd, in SavePackage where this type of logic more
> arguably belongs?  It'll save future maintainers (including possibly yourself)
> headache and blood pressure med costs.

Actually, my bp is on the low end. My doctor suggested eating *more*
cholesterol, but maintaining even more bad code would be just as good.

SavePackage is in content, GDataDownloadObserver is in chrome.
How does adding a new method
DownloadManagerDelegate::SavePackageDownloadItemCreated(DownloadItem*) look?

[benjhayden, 2012-04-30 13:37:19]

PTAL

http://codereview.chromium.org/10069014/diff/95007/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/95007/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.h:93: #endif
On 2012/04/27 18:24:04, rdsmith wrote:
> It looks like CDMD has to date only used ifdefs to null out subsections of
code
> rather than whole function APIs.  If Noe' et al have an opinion, it should be
> followed, but in the absence of this are you willing to declare this function
> but make its behavior null if Safe browsing isn't defined, similar to
> GetDownloadProtectionService?

Done.

http://codereview.chromium.org/10069014/diff/95007/content/browser/download/s...
File content/browser/download/save_package.h (right):

http://codereview.chromium.org/10069014/diff/95007/content/browser/download/s...
content/browser/download/save_package.h:94: // |download_created_callback| will
be called when the DownloadItem is
On 2012/04/27 18:24:04, rdsmith wrote:
> nit: There is no entity named download_created_callback in the below
> declaration.

Done.

[Randy Smith (Not in Mondays), 2012-05-01 17:53:50]

LGTM with nits below and Very Next CL (:-}) trying to fix the
MaybeCompleteDownload mess.

http://codereview.chromium.org/10069014/diff/95007/content/public/browser/dow...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/95007/content/public/browser/dow...
content/public/browser/download_manager_delegate.h:25:
SaveFileDownloadCreatedCallback;
On 2012/04/27 18:24:04, rdsmith wrote:
> Nit: There are many places where we save files (e.g. Save Link As ...) but
this
> is only used for SavePackage; I'd use SavePackage in its names rather than
> SaveFile.

Didn't see a response to this note?

http://codereview.chromium.org/10069014/diff/113002/content/public/browser/do...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/113002/content/public/browser/do...
content/public/browser/download_manager_delegate.h:27: // Called by
SavePackageFilePicker when the user picks a filename.
I think I'd like to rephrase and amplify this comment.  I don't think the DMD
should have references to the SavePackageFilePicker; that's an implementation
detail of the CDMD.  Instead, I'd say something like "Will be called
asynchronously with the results of the ChooseSavePath operation.  If the
delegate wants notification of the download item created in response to this
operation, the SaveFileDownloadCreatedCallback will be non-null."

(Except that similarly to "DMD shouldn't refer to the SavePackageFilePicker",
DMD also shouldn't refer to SaveFilePathPicked.  Maybe just
DownloadCreatedCallback and ChooseSavePathCallback?)

[benjhayden, 2012-05-01 18:34:58]

http://codereview.chromium.org/10069014/diff/95007/content/public/browser/dow...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/95007/content/public/browser/dow...
content/public/browser/download_manager_delegate.h:25:
SaveFileDownloadCreatedCallback;
On 2012/05/01 17:53:50, rdsmith wrote:
> On 2012/04/27 18:24:04, rdsmith wrote:
> > Nit: There are many places where we save files (e.g. Save Link As ...) but
> this
> > is only used for SavePackage; I'd use SavePackage in its names rather than
> > SaveFile.
> 
> Didn't see a response to this note?

Done.

http://codereview.chromium.org/10069014/diff/113002/content/public/browser/do...
File content/public/browser/download_manager_delegate.h (right):

http://codereview.chromium.org/10069014/diff/113002/content/public/browser/do...
content/public/browser/download_manager_delegate.h:27: // Called by
SavePackageFilePicker when the user picks a filename.
On 2012/05/01 17:53:50, rdsmith wrote:
> I think I'd like to rephrase and amplify this comment.  I don't think the DMD
> should have references to the SavePackageFilePicker; that's an implementation
> detail of the CDMD.  Instead, I'd say something like "Will be called
> asynchronously with the results of the ChooseSavePath operation.  If the
> delegate wants notification of the download item created in response to this
> operation, the SaveFileDownloadCreatedCallback will be non-null."
> 
> (Except that similarly to "DMD shouldn't refer to the SavePackageFilePicker",
> DMD also shouldn't refer to SaveFilePathPicked.  Maybe just
> DownloadCreatedCallback and ChooseSavePathCallback?)

Done.

[achuithb, 2012-05-01 19:28:05]

lgtm

[asanka, 2012-05-02 19:26:09]

LGTM w/nits.

http://codereview.chromium.org/10069014/diff/127002/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10069014/diff/127002/chrome/browser/about_flag...
chrome/browser/about_flags.cc:402: "save-page-as-mhtml",  // FLAGS::RECORD_UMA
FLAGS:RECORD_UMA (a single colon).

And then re-run extract_actions.py. You should see a new entry for this flag in
chromeactions.txt.

http://codereview.chromium.org/10069014/diff/127002/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10069014/diff/127002/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:205: state->pending
= false;
Shouldn't this only be done if state == NULL?

Ideally, I think you should DCHECK that state == NULL if someone is trying to
disable safe browsing.

[benjhayden, 2012-05-03 16:56:48]

PTAL

http://codereview.chromium.org/10069014/diff/127002/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10069014/diff/127002/chrome/browser/about_flag...
chrome/browser/about_flags.cc:402: "save-page-as-mhtml",  // FLAGS::RECORD_UMA
On 2012/05/02 19:26:10, asanka wrote:
> FLAGS:RECORD_UMA (a single colon).
> 
> And then re-run extract_actions.py. You should see a new entry for this flag
in
> chromeactions.txt.

Done.

http://codereview.chromium.org/10069014/diff/127002/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10069014/diff/127002/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:205: state->pending
= false;
On 2012/05/02 19:26:10, asanka wrote:
> Shouldn't this only be done if state == NULL?
> 
> Ideally, I think you should DCHECK that state == NULL if someone is trying to
> disable safe browsing.

Done.

[asanka, 2012-05-03 17:32:14]

LGTM

[commit-bot: I haz the power, 2012-05-03 20:45:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10069014/133029

[commit-bot: I haz the power, 2012-05-03 20:45:28]

Presubmit check for 10069014-133029 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser
    content/test

Presubmit checks took 4.8s to calculate.

[benjhayden, 2012-05-03 21:01:31]

Scott: Would you mind reviewing these changes to content/browser/ and
content/test for OWNERShip?
Thanks!

[sky, 2012-05-03 21:12:34]

LGTM

[commit-bot: I haz the power, 2012-05-04 17:39:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10069014/146004

[commit-bot: I haz the power, 2012-05-04 18:00:49]

Try job failure for 10069014-146004 (retry) on win for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-05-04 18:14:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10069014/146004

[commit-bot: I haz the power, 2012-05-04 19:35:32]

Change committed as 135390