OOPIFs: Deduplicating MHTML parts across frames.

content/browser/download/mhtml_generation_manager.cc

Index: content/browser/download/mhtml_generation_manager.cc
diff --git a/content/browser/download/mhtml_generation_manager.cc b/content/browser/download/mhtml_generation_manager.cc
index 9aa39446ba622bbc60f03161edf64375f92ac20d..1bc5256dc4f61f27ea64a1a2543d4fc9b60d9deb 100644
--- a/content/browser/download/mhtml_generation_manager.cc
+++ b/content/browser/download/mhtml_generation_manager.cc
@@ -22,6 +22,8 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_process_host_observer.h"
 #include "content/public/browser/web_contents.h"
+#include "crypto/sha2.h"
+#include "url/gurl.h"
 
 namespace content {
 
@@ -36,6 +38,11 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
 
   GenerateMHTMLCallback callback() const { return callback_; }
 
+  // Handler for ViewHostMsg_SavedPageAsMHTML (a notification from the renderer
+  // that the MHTML generation for previous frame has finished).
+  // Returns |true| upon success; |false| otherwise.
+  bool OnSavedPageAsMHTML(const std::set<GURL>& uris_of_generated_mhtml_parts);
+
   // Sends IPC to the renderer, asking for MHTML generation of the next frame.
   //
   // Returns true if the message was sent successfully; false otherwise.
@@ -83,6 +90,9 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
   // MIME multipart boundary to use in the MHTML doc.
   std::string mhtml_boundary_marker_;
 
+  // URIs of already generated MHTML parts.
+  std::set<GURL> already_serialized_uris_;
+
   // The callback to call once generation is complete.
   GenerateMHTMLCallback callback_;
 
@@ -141,9 +151,13 @@ bool MHTMLGenerationManager::Job::SendToNextRenderFrame() {
   DCHECK(browser_file_.IsValid());
   DCHECK_LT(0u, pending_frame_tree_node_ids_.size());
 
+  FrameMsg_SerializeAsMHTML_Params ipc_params;
+  ipc_params.job_id = job_id_;
+  ipc_params.mhtml_boundary_marker = mhtml_boundary_marker_;
+
   int frame_tree_node_id = pending_frame_tree_node_ids_.front();
   pending_frame_tree_node_ids_.pop();
-  bool is_last_frame = pending_frame_tree_node_ids_.empty();
+  ipc_params.is_last_frame = pending_frame_tree_node_ids_.empty();
 
   FrameTreeNode* ftn = FrameTreeNode::GloballyFindByID(frame_tree_node_id);
   if (!ftn)  // The contents went away.
@@ -154,12 +168,19 @@ bool MHTMLGenerationManager::Job::SendToNextRenderFrame() {
   observed_renderer_process_host_.RemoveAll();
   observed_renderer_process_host_.Add(rfh->GetProcess());
 
-  IPC::PlatformFileForTransit renderer_file = IPC::GetFileHandleForProcess(
+  // Tell the renderer to skip (= deduplicate) already covered MHTML parts.
+  ipc_params.salt = base::GenerateGUID();
+  for (const GURL& uri : already_serialized_uris_) {
+    std::string digest = crypto::SHA256HashString(ipc_params.salt + uri.spec());

[dcheng, 2015/12/11 07:36:05]

Maybe already_serialized_uris should just be pre-hashed, rather than repeating
this calculation on each SendToNextRenderFrame?

[Łukasz Anforowicz, 2015/12/14 19:39:02]

Yes, good point.  Done.

+    ipc_params.digests_of_uris_to_skip.insert(digest);
+  }
+
+  ipc_params.destination_file = IPC::GetFileHandleForProcess(
       browser_file_.GetPlatformFile(), rfh->GetProcess()->GetHandle(),
       false);  // |close_source_handle|.
-  rfh->Send(new FrameMsg_SerializeAsMHTML(
-      rfh->GetRoutingID(), job_id_, renderer_file, mhtml_boundary_marker_,
-      CreateFrameRoutingIdToContentId(rfh->GetSiteInstance()), is_last_frame));
+  ipc_params.frame_routing_id_to_content_id =
+      CreateFrameRoutingIdToContentId(rfh->GetSiteInstance());
+  rfh->Send(new FrameMsg_SerializeAsMHTML(rfh->GetRoutingID(), ipc_params));
   return true;
 }
 
@@ -204,6 +225,19 @@ void MHTMLGenerationManager::Job::CloseFile(
       callback);
 }
 
+bool MHTMLGenerationManager::Job::OnSavedPageAsMHTML(
+    const std::set<GURL>& uris_of_generated_mhtml_parts) {
+  bool success = true;

[dcheng, 2015/12/11 07:36:05]

Remove this.

[Łukasz Anforowicz, 2015/12/14 19:39:02]

Done.

+
+  already_serialized_uris_.insert(uris_of_generated_mhtml_parts.begin(),
+                                  uris_of_generated_mhtml_parts.end());
+
+  if (HasMoreFramesToProcess())
+    success = SendToNextRenderFrame();

[dcheng, 2015/12/11 07:36:05]

And return this directly.

[Łukasz Anforowicz, 2015/12/14 19:39:02]

Done.

+
+  return success;

[dcheng, 2015/12/11 07:36:05]

And return true here. It might be useful in the future, but it doesn't add much
now. I would personally invert this as well, so it's:

if (!HasMoreFramesToProcess())
  return true;
return SendToNextRenderFrame();

[Łukasz Anforowicz, 2015/12/14 19:39:02]

Yes - this is fair feedback.  It does read better this way.  Done.

+}
+
 // static
 int64 MHTMLGenerationManager::Job::CloseFileOnFileThread(base::File file) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
@@ -259,7 +293,8 @@ void MHTMLGenerationManager::SaveMHTML(WebContents* web_contents,
 
 void MHTMLGenerationManager::OnSavedPageAsMHTML(
     int job_id,
-    bool mhtml_generation_in_renderer_succeeded) {
+    bool mhtml_generation_in_renderer_succeeded,
+    const std::set<GURL>& uris_of_generated_mhtml_parts) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (!mhtml_generation_in_renderer_succeeded) {
@@ -271,14 +306,14 @@ void MHTMLGenerationManager::OnSavedPageAsMHTML(
   if (!job)
     return;
 
-  if (job->HasMoreFramesToProcess()) {
-    if (!job->SendToNextRenderFrame()) {
-      JobFinished(job_id, JobStatus::FAILURE);
-    }
+  if (!job->HasMoreFramesToProcess()) {
+    JobFinished(job_id, JobStatus::SUCCESS);
     return;
   }
 
-  JobFinished(job_id, JobStatus::SUCCESS);
+  if (!job->OnSavedPageAsMHTML(uris_of_generated_mhtml_parts)) {
+    JobFinished(job_id, JobStatus::FAILURE);
+  }
 }
 
 // static