Save Page As MHTML

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 9bdb48aa7bfbe2382d1e3b34056dbf0f6c79d308..9d67769fc6d271b020880251f2a08d7939a13e49 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -444,11 +444,18 @@ DownloadItem* DownloadManagerImpl::CreateSavePackageDownloadItem(
     const FilePath& main_file_path,
     const GURL& page_url,
     bool is_otr,
+    const std::string& mime_type,
     DownloadItem::Observer* observer) {
   net::BoundNetLog bound_net_log =
       net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
   DownloadItem* download = new DownloadItemImpl(
-      this, main_file_path, page_url, is_otr, GetNextId(), bound_net_log);
+      this,
+      main_file_path,
+      page_url,
+      is_otr,
+      GetNextId(),
+      mime_type,
+      bound_net_log);
 
   download->AddObserver(observer);
 
@@ -594,6 +601,10 @@ void DownloadManagerImpl::AssertStateConsistent(DownloadItem* download) const {
 }
 
 bool DownloadManagerImpl::IsDownloadReadyForCompletion(DownloadItem* download) {

[asanka, 2012/04/26 16:06:20]

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.

[asanka, 2012/04/26 16:06:20]

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]

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]

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]

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:48]

Ah, nope, sorry.
The fact that MaybeCompleteDownload() runs for gdata SavePackages is an artifact
of the interaction with the GDataDownloadObserver, not intentional functional
change.
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]

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]

PTAL, Thanks!

+  VLOG(20) << __FUNCTION__ << " all_data_saved=" << download->AllDataSaved()
+    << " safety=" << download->GetSafetyState()
+    << " active=" << (active_downloads_.count(download->GetId()) == 0)
+    << " persisted=" << download->IsPersisted();
   // If we don't have all the data, the download is not ready for
   // completion.
   if (!download->AllDataSaved())