GData save package support with MHTML.

chrome/browser/download/save_package_file_picker_chromeos.cc

Index: chrome/browser/download/save_package_file_picker_chromeos.cc
===================================================================
--- chrome/browser/download/save_package_file_picker_chromeos.cc	(revision 0)
+++ chrome/browser/download/save_package_file_picker_chromeos.cc	(revision 0)
@@ -0,0 +1,109 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/download/save_package_file_picker_chromeos.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "chrome/browser/chromeos/gdata/gdata_download_observer.h"
+#include "chrome/browser/chromeos/gdata/gdata_file_system.h"
+#include "chrome/browser/chromeos/gdata/gdata_system_service.h"
+#include "chrome/browser/chromeos/gdata/gdata_uploader.h"
+#include "chrome/browser/chromeos/gdata/gdata_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
+
+using content::BrowserThread;
+
+SavePackageFilePickerChromeOS::SavePackageFilePickerChromeOS(
+    content::WebContents* web_contents,
+    const FilePath& suggested_path,
+    const FilePath::StringType& default_extension,
+    bool can_save_as_complete,
+    DownloadPrefs* download_prefs,
+    content::SaveFilePathPickedCallback callback) :
+        SavePackageFilePicker(web_contents, suggested_path, default_extension,
+                              can_save_as_complete, download_prefs, callback),

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

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?

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

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.

+        content::WebContentsObserver(web_contents) {
+}
+
+SavePackageFilePickerChromeOS::~SavePackageFilePickerChromeOS() {
+}
+
+void SavePackageFilePickerChromeOS::FileSelected(const FilePath& path,
+                                                 int index,
+                                                 void* params) {
+  if (!web_contents()) {
+    NOTREACHED();
+    return;
+  }
+
+  if (gdata::util::IsUnderGDataMountPoint(path)) {
+    FilePath gdata_tmp_download_dir =
+        GetGDataFileSystem()->GetGDataTempDownloadFolderPath();
+
+    selected_path_ = path;
+    FilePath* gdata_tmp_download_path = new FilePath();
+    BrowserThread::GetBlockingPool()->PostTaskAndReply(FROM_HERE,
+        base::Bind(&gdata::GDataDownloadObserver::GetGDataTempDownloadPath,
+                   gdata_tmp_download_dir,
+                   gdata_tmp_download_path),
+        base::Bind(&SavePackageFilePickerChromeOS::GenerateMHTML,
+                   base::Unretained(this),
+                   base::Owned(gdata_tmp_download_path)));
+  } else {
+    DVLOG(1) << "SavePackageFilePickerChromeOS non-gdata file";
+    SavePackageFilePicker::FileSelected(path, index, params);

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

actually, on ChromeOS we probably want to make saving to gdata consistent with
saving to /Downloads - so we should generate MHTML in both cases

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

Done.

+  }
+}
+
+void SavePackageFilePickerChromeOS::GenerateMHTML(
+    FilePath* gdata_tmp_download_path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!web_contents()) {
+    NOTREACHED();
+    return;
+  }
+
+  DVLOG(1) << "GenerateMHTML " << gdata_tmp_download_path->value();
+  web_contents()->GenerateMHTML(*gdata_tmp_download_path,
+      base::Bind(&SavePackageFilePickerChromeOS::MHTMLGenerated,
+                 base::Unretained(this)));
+}
+
+void SavePackageFilePickerChromeOS::MHTMLGenerated(const FilePath& src_path,
+                                                   int64 file_size) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!web_contents()) {
+    NOTREACHED();
+    return;
+  }
+
+  DVLOG(1) << "TransferFile from " << src_path.value()
+           << " to " << selected_path_.value();
+  GetGDataFileSystem()->TransferFile(src_path, selected_path_,

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

if (!GetGDataFileSystem())
  return;

see comment below

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

Done.

+      base::Bind(&SavePackageFilePickerChromeOS::OnTransferFile,
+                 base::Unretained(this)));
+}
+
+void SavePackageFilePickerChromeOS::OnTransferFile(
+    base::PlatformFileError error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(error, base::PLATFORM_FILE_OK);
+  delete this;
+}
+
+gdata::GDataFileSystem*
+SavePackageFilePickerChromeOS::GetGDataFileSystem() {
+  DCHECK(web_contents());
+  Profile* profile = Profile::FromBrowserContext(
+      web_contents()->GetBrowserContext());
+  DCHECK(profile);
+  gdata::GDataSystemService* system_service =
+      gdata::GDataSystemServiceFactory::GetForProfile(profile);
+  DCHECK(system_service);

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

nit: this will be NULL in incognito mode

if (!system_service)
  return;

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

Done.

+  return system_service->file_system();
+}
Property changes on: chrome/browser/download/save_package_file_picker_chromeos.cc
___________________________________________________________________
Added: svn:eol-style
   + LF