Save Page As MHTML

chrome/browser/chromeos/gdata/gdata_download_observer.cc

Index: chrome/browser/chromeos/gdata/gdata_download_observer.cc
diff --git a/chrome/browser/chromeos/gdata/gdata_download_observer.cc b/chrome/browser/chromeos/gdata/gdata_download_observer.cc
index 3cb89fb37a46dfe357cda973f292628911e1781d..b566a009a4a64278e5de1fce5b393a4291bebe61 100644
--- a/chrome/browser/chromeos/gdata/gdata_download_observer.cc
+++ b/chrome/browser/chromeos/gdata/gdata_download_observer.cc
@@ -133,7 +133,7 @@ bool GDataDownloadObserver::IsReadyToComplete(DownloadItem* download) {
   // 2. The upload has completed.
   UploadingExternalData* upload_data = GetUploadingExternalData(download);
   return !IsGDataDownload(download) ||
-      (upload_data && upload_data->is_complete());
+         (upload_data && upload_data->is_complete());
 }
 
 // static
@@ -291,10 +291,10 @@ bool GDataDownloadObserver::ShouldUpload(DownloadItem* download) {
   // Upload if the item is in pending_downloads_,
   // is complete or large enough to stream, and,
   // is not already being uploaded.
-  return pending_downloads_.count(download->GetId()) != 0 &&
+  return (pending_downloads_.count(download->GetId()) != 0) &&
          (download->AllDataSaved() ||
           download->GetReceivedBytes() > kStreamingFileSize) &&
-         GetUploadingExternalData(download) == NULL;
+         (GetUploadingExternalData(download) == NULL);
 }
 
 scoped_ptr<UploadFileInfo> GDataDownloadObserver::CreateUploadFileInfo(
@@ -340,6 +340,14 @@ void GDataDownloadObserver::OnUploadComplete(int32 download_id,
   DCHECK(upload_data);
   upload_data->MarkAsComplete();
   download->MaybeCompleteDownload();

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

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?

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

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?

+  // MaybeCompleteDownload() only works for non-SavePackage downloads.
+  // SavePackage::Finish() is the SavePackage equivalent of
+  // MaybeCompleteDownload(). MHTML SavePackages may upload to GData (see
+  // SavePackageFilePickerChromeOS), so MHTML SavePackages use
+  // OnDownloadUpdated() to wait for the upload to complete before calling
+  // Finish(). Call UpdateObservers() manually now in case this download is an
+  // MHTML SavePackage.
+  download->UpdateObservers();
 }
 
 }  // namespace gdata