[Offline Pages] Generate MHTML header in the browser process.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 005ade583726e7001e4fd9ab0c3fb817992e2ccb..18cf95342384e56bd8f943810833b3d5f474e270 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -5621,36 +5621,20 @@ void RenderFrameImpl::OnSerializeAsMHTML(
   std::set<std::string> serialized_resources_uri_digests;
   MHTMLPartsGenerationDelegate delegate(params,
                                         &serialized_resources_uri_digests);
-
   MhtmlSaveStatus save_status = MhtmlSaveStatus::SUCCESS;
-  bool has_some_data = false;
-
-  // Generate MHTML header if needed.
-  if (IsMainFrame()) {
-    TRACE_EVENT0("page-serialization",
-                 "RenderFrameImpl::OnSerializeAsMHTML header");

[carlosk, 2017/04/25 01:28:08]

Could you make this header serialization tracing survive this change like it was
done for the footer change? (see
MHTMLGenerationManager::Job::FinalizeAndCloseFileOnFileThread)

Ideally it would encompass the header generation/serialization too but that
might not be ideal for this case given all the data needed to generate it.

-    // The returned data can be empty if the main frame should be skipped. If
-    // the main frame is skipped, then the whole archive is bad.
-    mhtml_contents.emplace_back(WebFrameSerializer::GenerateMHTMLHeader(
-        mhtml_boundary, GetWebFrame(), &delegate));
-    if (mhtml_contents.back().IsEmpty())
-      save_status = MhtmlSaveStatus::FRAME_SERIALIZATION_FORBIDDEN;
-    else
-      has_some_data = true;
-  }
 
   // Generate MHTML parts.  Note that if this is not the main frame, then even
   // skipping the whole parts generation step is not an error - it simply
   // results in an omitted resource in the final file.
-  if (save_status == MhtmlSaveStatus::SUCCESS) {
-    TRACE_EVENT0("page-serialization",
-                 "RenderFrameImpl::OnSerializeAsMHTML parts serialization");
-    // The returned data can be empty if the frame should be skipped, but this
-    // is OK.
-    mhtml_contents.emplace_back(WebFrameSerializer::GenerateMHTMLParts(
-        mhtml_boundary, GetWebFrame(), &delegate));
-    has_some_data |= !mhtml_contents.back().IsEmpty();
-  }
+  TRACE_EVENT0("page-serialization",
+               "RenderFrameImpl::OnSerializeAsMHTML parts serialization");
+  // The returned data can be empty if the frame should be skipped, but this
+  // is OK.
+  mhtml_contents.emplace_back(WebFrameSerializer::GenerateMHTMLParts(
+      mhtml_boundary, GetWebFrame(), &delegate));
+  bool has_some_data = !mhtml_contents.back().IsEmpty();
+  if (IsMainFrame() && !has_some_data)
+    save_status = MhtmlSaveStatus::FRAME_SERIALIZATION_FORBIDDEN;
 
   // Note: the MHTML footer is written by the browser process, after the last
   // frame is serialized by a renderer process.

[carlosk, 2017/04/25 01:28:08]

nit: we should either remove this comment above or add a similar one for the
MTHML header. I'm slightly favoring the former as this comment only makes sense
because the footer used to be generated here and that doesn't matter anymore.

@@ -5663,7 +5647,7 @@ void RenderFrameImpl::OnSerializeAsMHTML(
       "PageSerialization.MhtmlGeneration.RendererMainThreadTime.SingleFrame",
       main_thread_use_time);
 
-  if (save_status == MhtmlSaveStatus::SUCCESS && has_some_data) {
+  if (has_some_data) {
     base::PostTaskAndReplyWithResult(
         RenderThreadImpl::current()->GetFileThreadTaskRunner().get(), FROM_HERE,
         base::Bind(&WriteMHTMLToDisk, base::Passed(&mhtml_contents),