Get signals working in the EXTRA_DATA section of MHTML

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 b989af11932a1e2c49ec1451e7d55560874b8f28..9044450707f719d723b351dd9fe62b4d12b5ebad 100644
--- a/content/browser/download/mhtml_generation_manager.cc
+++ b/content/browser/download/mhtml_generation_manager.cc
@@ -32,6 +32,10 @@
 #include "content/public/common/mhtml_generation_params.h"
 #include "net/base/mime_util.h"
 
+namespace {
+const char kApplicationJson[] = "application/json";
+}  // namespace
+
 namespace content {
 
 // The class and all of its members live on the UI thread.  Only static methods
@@ -101,9 +105,16 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
   static std::tuple<MhtmlSaveStatus, int64_t> CloseFileOnFileThread(
       MhtmlSaveStatus save_status,
       const std::string& boundary,
-      base::File file);
+      base::File file,
+      const std::string& extra_data);
   void AddFrame(RenderFrameHost* render_frame_host);
 
+  // If we have any extra data sections to write out, write them into the file
+  // while on the file thread.  Returns true for success.
+  static bool WriteExtraDataSectionOnFileThread(const std::string& boundary,
+                                                base::File& file,
+                                                const std::string& extra_data);
+
   // Creates a new map with values (content ids) the same as in
   // |frame_tree_node_to_content_id_| map, but with the keys translated from
   // frame_tree_node_id into a |site_instance|-specific routing_id.
@@ -355,7 +366,7 @@ void MHTMLGenerationManager::Job::CloseFile(
           &MHTMLGenerationManager::Job::CloseFileOnFileThread, save_status,
           (save_status == MhtmlSaveStatus::SUCCESS ? mhtml_boundary_marker_
                                                    : std::string()),
-          base::Passed(&browser_file_)),
+          base::Passed(&browser_file_), params_.extra_data),
       callback);
 }
 
@@ -401,9 +412,11 @@ MhtmlSaveStatus MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse(
 
 // static
 std::tuple<MhtmlSaveStatus, int64_t>
-MHTMLGenerationManager::Job::CloseFileOnFileThread(MhtmlSaveStatus save_status,
-                                                   const std::string& boundary,
-                                                   base::File file) {
+MHTMLGenerationManager::Job::CloseFileOnFileThread(
+    MhtmlSaveStatus save_status,
+    const std::string& boundary,
+    base::File file,
+    const std::string& extra_data) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   // If no previous error occurred the boundary should have been provided.
@@ -411,6 +424,11 @@ MHTMLGenerationManager::Job::CloseFileOnFileThread(MhtmlSaveStatus save_status,
     TRACE_EVENT0("page-serialization",
                  "MHTMLGenerationManager::Job MHTML footer writing");
     DCHECK(!boundary.empty());
+
+    // Write the extra data into a section of its own, if we have any.
+    if (!WriteExtraDataSectionOnFileThread(boundary, file, extra_data))
+      save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR;
+
     std::string footer = base::StringPrintf("--%s--\r\n", boundary.c_str());
     DCHECK(base::IsStringASCII(footer));
     if (file.WriteAtCurrentPos(footer.data(), footer.size()) < 0)
@@ -430,6 +448,25 @@ MHTMLGenerationManager::Job::CloseFileOnFileThread(MhtmlSaveStatus save_status,
   return std::make_tuple(save_status, file_size);
 }
 
+// static
+bool MHTMLGenerationManager::Job::WriteExtraDataSectionOnFileThread(
+    const std::string& boundary,
+    base::File& file,
+    const std::string& extra_data) {
+  // Write a newline, then a boundary, then the mime type, another newline,
+  // the extra data string, and end with a newline.
+  std::string extra_data_section =
+      base::StringPrintf("--%s--\r\n%s\r\n%s\r\n", boundary.c_str(),

[Dmitry Titov, 2017/03/14 01:16:02]

It looks like you are trying to form a 'legit' MHTML part here, with header and
body subparts separated with \r\n. This is good, and perhaps we should use legit
HTTP headers here as Content-Type: and  Content-Location: (the latter with a
generated CID).

Also, the content type could have been provided by the same code that serializes
the 'extra_data' string, because that code is deciding what kind of
serialization to use. 

[romax, 2017/03/14 19:08:05]

is it possible to have some kind of signs that this section is used for signal
recording? An explicit sign would be pretty helpful when parsing the raw MHTML.

[Pete Williamson, 2017/03/15 22:38:22]

I explicitly kept this section as extra data.  Signals is just one potential
use.  So, from the point of view of the MHTML file, this is just extra data it
doesn't know much about, but from the point of view of the offliner, this is
signal data.  The naming in each file reflects this idea.

[Pete Williamson, 2017/03/15 22:38:22]

I moved the Content-Type to where I build the data.
I've added Content-Location, but not sure what to use for CID - any ideas? 
Maybe just the filename for the MHTML file?

+                         kApplicationJson, extra_data.c_str());
+  // Now actually write the string into the file.
+  DCHECK(base::IsStringASCII(extra_data_section));
+  if (file.WriteAtCurrentPos(extra_data_section.data(),
+                             extra_data_section.size()) < 0)
+    return false;
+
+  return true;
+}
+
 MHTMLGenerationManager* MHTMLGenerationManager::GetInstance() {
   return base::Singleton<MHTMLGenerationManager>::get();
 }