GData save package support with MHTML.

GData save package support with MHTML.

* Introduce SavePackageFilePickerChromeOS, which derives from SavePackageFilePicker. 
* For gdata targets, we save the selected path, create a temporary mhtml file using WebContents::GenerateMHTML, and call GDataFileSystem::TransferFile to transfer the file to gdata.
* For non-gdata targets, we just generate the mhtml file in place.
* DownloadObserver::GetGDataTempDownloadPath is a static helper method to create a temporary gdata download file, used by SavePackageFilePickerChromeOS and DownloadFilePickerChromeOS.


BUG=chromium-os:28210
TEST=Go to a web page, use Save As, or ctrl-s to save a page to docs.
TBR=rdsmith@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129115

[achuithb, 2012-03-26 09:27:13]

Much smaller in scope.

GDataFileSystem::TransferFile seems to be currently broken in some fashion so
I'm unable to test this end to end, but it should work.

[zel, 2012-03-26 16:13:14]

lgtm

few nits below

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:87:
GetGDataFileSystem()->TransferFile(src_path, selected_path_,
if (!GetGDataFileSystem())
  return;

see comment below

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:107:
DCHECK(system_service);
nit: this will be NULL in incognito mode

if (!system_service)
  return;

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker_chromeos.h (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.h:41: void
MHTMLGenerated(const FilePath& file_path, int64 file_size);
nit: OnMHTMLGenerated

[Randy Smith (Not in Mondays), 2012-03-26 16:19:41]

High level question: Would it be possible to allow for MHTML saving from the
general Save package system and make the ChromeOS tweak just disabling the
standard SavePackage stuff?  I'd presume that'd result in less platform specific
code.

[Randy Smith (Not in Mondays), 2012-03-26 16:24:00]

One other high level suggestion (not a request, as I'm not certain that my sense
of architectural "goodness" matches with the teams on this).  I dislike
inheriting a concrete class from another concrete class, and changing "private"
to "protected" has a bit of a code smell to me.  Would it work to make
SavePackageFilePicker an interface that SavePackageFilePickerImpl and
ChromeOsSavePackageFilePickerImpl both inherited from?  I think that would keep
the inheritance cleaner.  

(If you disagree with my aesthetics on this one, I'm willing to yield; I'm
mostly just reacting to the "private" -> "protected" change.)

[zel, 2012-03-26 16:33:41]

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:59:
SavePackageFilePicker::FileSelected(path, index, params);
actually, on ChromeOS we probably want to make saving to gdata consistent with
saving to /Downloads - so we should generate MHTML in both cases

[asanka, 2012-03-26 16:50:13]

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:159: void
GDataDownloadObserver::GetGDataTempDownloadPath(
// static

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_download_observer.h (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_download_observer.h:57: static void
GetGDataTempDownloadPath(const FilePath& gdata_tmp_download_dir,
Should mention here that it will create the temporary download path as well. And
it should be only be called from a thread that allows file I/O.

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_uploader.cc (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_uploader.cc:50:
DCHECK(!upload_file_info->content_type.empty());
For regular downloads, it is possible that we don't have a content_type.

[Randy Smith (Not in Mondays), 2012-03-26 17:13:02]

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:29:
can_save_as_complete, download_prefs, callback),
I apologize, as there's probably something basic I'm missing, but doesn't this
design mean that the callback will never be called?  It's called in the base
class on FileSelected(), but you've overridden that method in this class, and
the override doesn't call the callback.  That makes it look as if you're doing
the file saving logic in one place in the non-CrOS case (SavePackage) and in
another place in the CrOS case (here).  That seems like a recipe for making
future maintenance hard.  

Am I confused?

[Randy Smith (Not in Mondays), 2012-03-26 19:26:56]

Offline conversation quick summary: Achuith and I talked and explored design
alternatives to this CL.  He convinced me that those alternatives were worse
than this in the MS-19 time frame (we'd need to do more save package
integration, not less, in order to fix things like getting the MHTML progress to
show up on the download shelf--this CL won't show MHTML downloads on the
download shelf at all).  So we're going to go with this approach for now; I'm
going to request the comments I'd like to have to minimize people being confused
by the different responsibilities taken in the ChromeOS and non-ChromeOS case,
and I'm going to file a bug with the changes I'd like in the MS-20 timeframe.

[achuithb, 2012-03-26 19:44:44]

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:29:
can_save_as_complete, download_prefs, callback),
On 2012/03/26 17:13:02, rdsmith wrote:
> I apologize, as there's probably something basic I'm missing, but doesn't this
> design mean that the callback will never be called?  It's called in the base
> class on FileSelected(), but you've overridden that method in this class, and
> the override doesn't call the callback.  That makes it look as if you're doing
> the file saving logic in one place in the non-CrOS case (SavePackage) and in
> another place in the CrOS case (here).  That seems like a recipe for making
> future maintenance hard.  
> 
> Am I confused?

Talked about this in person.

This code isn't ideal, and we need a better design for this and save_package.
We'll try to tackle this in M20.

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:87:
GetGDataFileSystem()->TransferFile(src_path, selected_path_,
On 2012/03/26 16:13:14, zel wrote:
> if (!GetGDataFileSystem())
>   return;
> 
> see comment below

Done.

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:107:
DCHECK(system_service);
On 2012/03/26 16:13:14, zel wrote:
> nit: this will be NULL in incognito mode
> 
> if (!system_service)
>   return;

Done.

[achuithb, 2012-03-26 19:51:39]

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_download_observer.cc (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_download_observer.cc:159: void
GDataDownloadObserver::GetGDataTempDownloadPath(
On 2012/03/26 16:50:14, asanka wrote:
> // static

Done.

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_download_observer.h (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_download_observer.h:57: static void
GetGDataTempDownloadPath(const FilePath& gdata_tmp_download_dir,
On 2012/03/26 16:50:14, asanka wrote:
> Should mention here that it will create the temporary download path as well.
And
> it should be only be called from a thread that allows file I/O.

Done.

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_uploader.cc (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_uploader.cc:50:
DCHECK(!upload_file_info->content_type.empty());
On 2012/03/26 16:50:14, asanka wrote:
> For regular downloads, it is possible that we don't have a content_type.

We do need to handle this, but I pulled this file out of the CL.

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker_chromeos.h (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.h:41: void
MHTMLGenerated(const FilePath& file_path, int64 file_size);
On 2012/03/26 16:13:14, zel wrote:
> nit: OnMHTMLGenerated

Done.

[Randy Smith (Not in Mondays), 2012-03-26 21:08:33]

My apologies about all the comment requests, but this CL is messing with the
interface contract for a few functions up and down the stack ....

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/chro...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:314: void
ChromeDownloadManagerDelegate::ChooseSavePath(
Could you put a comment at the declaration of
DownloadManagerDelegate::ChooseSavePath (i.e. in
content/browser/download/download_manager_delegate.h) indicating that in certain
cases the callback will be ignored and the function will take care of saving the
data all on its own?  (Again probably with a TODO() to fix it.)

I'd also like a similar comment in SavePackage::ContinueGetSaveInfo() noting
that in some cases we won't get the callback and the ChooseSavePath operation
will just take care of the save for us.  I think it's ok to point to
SavePackageFilePickerChromeOs from there (as an example of that case).

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker.h (right):

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker.h:16: // Handles showing a
dialog to the user to ask for the filename to save a page.
Could you put a comment here (probably under a TODO(achuith)) indicating that in
the case of the SavePackageFilePickerChromeOS the callback is ignored and the
SavePackageFilePicker does the work of saving the file as well?

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker.h:31: protected:
We didn't talk about this on VC, but (given Zel's request that we just make
ChromeOS always save MHTML) would doing the interface + 2 impls and getting rid
of the private->protected change make sense?  I don't think you're going to need
to do pass-through semantics on ChromeOs anymore.

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:39: if
(!web_contents()) {
It looks to me (WebContentsObserver::TabContentsDestroyed) that this can become
null in normal operation if you race with tab destruction.  If you agree, you
should handle that case (which I think means pulling the NOTREACHED() and
deleting this?).

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:60:
SavePackageFilePicker::FileSelected(path, index, params);
It looks like you haven't incorporated Zel's comment, and I didn't see a comment
in the stream responding to it.  What's the status of saving MHTML always in
ChromeOS whether or not we're going to GDrive?

[achuithb, 2012-03-26 21:35:53]

Going to remove the inheritance relationship between SavePackageFilePicker and
SavePackageFilePickerChromeOS next, and also add the comments requested by
Randy. Feel free to wait for that patch or review what I have now...

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker.h (right):

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker.h:16: // Handles showing a
dialog to the user to ask for the filename to save a page.
On 2012/03/26 21:08:33, rdsmith wrote:
> Could you put a comment here (probably under a TODO(achuith)) indicating that
in
> the case of the SavePackageFilePickerChromeOS the callback is ignored and the
> SavePackageFilePicker does the work of saving the file as well?

Probably shouldn't be doing it here?

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker.h:31: protected:
On 2012/03/26 21:08:33, rdsmith wrote:
> We didn't talk about this on VC, but (given Zel's request that we just make
> ChromeOS always save MHTML) would doing the interface + 2 impls and getting
rid
> of the private->protected change make sense?  I don't think you're going to
need
> to do pass-through semantics on ChromeOs anymore.

That's true, so I'll take a look at doing this next.

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:39: if
(!web_contents()) {
On 2012/03/26 21:08:33, rdsmith wrote:
> It looks to me (WebContentsObserver::TabContentsDestroyed) that this can
become
> null in normal operation if you race with tab destruction.  If you agree, you
> should handle that case (which I think means pulling the NOTREACHED() and
> deleting this?).

Yup. Done.

http://codereview.chromium.org/9809011/diff/1013/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:60:
SavePackageFilePicker::FileSelected(path, index, params);
On 2012/03/26 21:08:33, rdsmith wrote:
> It looks like you haven't incorporated Zel's comment, and I didn't see a
comment
> in the stream responding to it.  What's the status of saving MHTML always in
> ChromeOS whether or not we're going to GDrive?

It's done now.

[achuithb, 2012-03-26 21:37:46]

Last patch didn't have a comment (sorry), but the major item was having mhtml
files created for both gdata and non-gdata save-as paths.

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/9809011/diff/1010/chrome/browser/download/save...
chrome/browser/download/save_package_file_picker_chromeos.cc:59:
SavePackageFilePicker::FileSelected(path, index, params);
On 2012/03/26 16:33:41, zel wrote:
> actually, on ChromeOS we probably want to make saving to gdata consistent with
> saving to /Downloads - so we should generate MHTML in both cases

Done.

[achuithb, 2012-03-26 22:56:01]

SavePackageFilePickerChromeOS no longer inherits from SavePackageFilePicker. As
a result save_package_file_picker.h is no longer part of this CL.

Still need to add comments. I'm sure tests will fail on ChromeOS now that we no
longer have the shelf appearing with SavePackage files, and now that we don't
support save types. Going to look at failing browser tests next.

[achuithb, 2012-03-27 00:25:51]

All the tests pass. This CL is ready for review. PTAL Zel, Randy and Asanka.

[zel, 2012-03-27 00:55:20]

lgtm

[asanka, 2012-03-27 01:54:33]

LGTM.

Coordinate with https://chromiumcodereview.appspot.com/9854011/ because there
will be conflicts.

http://codereview.chromium.org/9809011/diff/10009/chrome/browser/download/sav...
File chrome/browser/download/save_package_file_picker_chromeos.cc (right):

http://codereview.chromium.org/9809011/diff/10009/chrome/browser/download/sav...
chrome/browser/download/save_package_file_picker_chromeos.cc:87: int64
file_size) {
Nit: Indent

[Randy Smith (Not in Mondays), 2012-03-27 17:15:46]

LGTM; thanks!

http://codereview.chromium.org/9809011/diff/10009/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/9809011/diff/10009/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:322: // Note that
we're ignoring the callback here. TODO(achuith): Fix this.
A little bit more information.  Maybe "... ignoring the callback here; the
SavePacakgeFilePickerChromeOS completes the save operation itself."?