Save Page As MHTML

content/browser/download/download_manager_impl.cc

 // 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 "content/browser/download/download_manager_impl.h"
 
 #include <iterator>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 426 matching lines @@
   downloads_.insert(download);
   active_downloads_[download_id] = download;
 
   return bound_net_log;
 }
 
 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);
 
   DCHECK(!ContainsKey(save_page_downloads_, download->GetId()));
   downloads_.insert(download);
   save_page_downloads_[download->GetId()] = download;
 
   // Will notify the observer in the callback.
   delegate_->AddItemToPersistentStore(download);
 
@@ skipping 124 matching lines @@
   if (ContainsKey(active_downloads_, download->GetId())) {
     if (download->IsPersisted())
       CHECK_EQ(DownloadItem::IN_PROGRESS, download->GetState());
     if (DownloadItem::IN_PROGRESS != download->GetState())
       CHECK_EQ(DownloadItem::kUninitializedHandle, download->GetDbHandle());
   }
   if (DownloadItem::IN_PROGRESS == download->GetState())
     CHECK(ContainsKey(active_downloads_, download->GetId()));
 }
 
 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())
     return false;
 
   // If the download is dangerous, but not yet validated, it's not ready for
   // completion.
   if (download->GetSafetyState() == DownloadItem::DANGEROUS)
     return false;
 
@@ skipping 601 matching lines @@
     if (it->second->IsComplete() && !it->second->GetOpened())
       ++num_unopened;
   }
   download_stats::RecordOpensOutstanding(num_unopened);
 }
 
 void DownloadManagerImpl::SetFileManagerForTesting(
       DownloadFileManager* file_manager) {
   file_manager_ = file_manager;
 }