Get signals working in the EXTRA_DATA section of MHTML

chrome/browser/android/offline_pages/prerendering_offliner.cc

Index: chrome/browser/android/offline_pages/prerendering_offliner.cc
diff --git a/chrome/browser/android/offline_pages/prerendering_offliner.cc b/chrome/browser/android/offline_pages/prerendering_offliner.cc
index a6564ad078d662af7b164c009e039d8dd89aa383..2cf2cbe1ae18bc73407ad20799a6e61ffa43b300 100644
--- a/chrome/browser/android/offline_pages/prerendering_offliner.cc
+++ b/chrome/browser/android/offline_pages/prerendering_offliner.cc
@@ -16,8 +16,13 @@
 #include "components/offline_pages/core/downloads/download_ui_adapter.h"
 #include "components/offline_pages/core/offline_page_model.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/mhtml_extra_data.h"
 #include "content/public/browser/web_contents.h"
 
+namespace {
+const char kApplicationJson[] = "application/json";
+}  // namespace
+
 namespace offline_pages {
 
 PrerenderingOffliner::PrerenderingOffliner(
@@ -83,6 +88,21 @@ void PrerenderingOffliner::OnLoadPageDone(
     else if (save_page_params.url != request.url())
       save_page_params.original_url = request.url();
 
+#ifndef NDEBUG

[Dmitry Titov, 2017/03/29 18:53:06]

why this specific part is debug-only? 

[Pete Williamson, 2017/03/29 22:05:42]

My intent here is that we only write the signals into the MHTML file in debug
mode, but that we collect the signals in all modes since we eventually want to
use them for decision making.

Since they are not being used for decision making now, if you prefer, I can make
the other signal gathering code debug only, but since it is a small amount of
code sprinkled throughout, I though it better for readability to leave the code
in for both debug and non-debug.

[Dmitry Titov, 2017/03/29 23:26:29]

So what is the plan? Do we build debug to run in harness to collect data? Do we
have a chrome://flag to enable data collection in mhtml files? I doubt debug
builds of Clank are awesome, especially on low-end phones.

[Pete Williamson, 2017/03/31 00:29:48]

After offline discussion, we decided on a new command line flag.

+    // Stash loading signals for writing when we write out the MHTML.
+    std::string body = SerializeLoadingSignalData();
+    std::string content_type = kApplicationJson;
+    std::string content_location = "cid:signal-data-" +
+                                   std::to_string(request.request_id()) +
+                                   "@mhtml.blink";
+
+    content::MHTMLExtraData* extra_data =
+        content::MHTMLExtraData::FromWebContents(web_contents);
+    DCHECK(extra_data);
+
+    extra_data->AddExtraMHTMLPart(content_type, content_location, body);
+#endif
+
     SavePage(save_page_params, std::move(archiver),
              base::Bind(&PrerenderingOffliner::OnSavePageDone,
                         weak_ptr_factory_.GetWeakPtr(), request));
@@ -94,6 +114,19 @@ void PrerenderingOffliner::OnLoadPageDone(
   }
 }
 
+std::string PrerenderingOffliner::SerializeLoadingSignalData() {
+  // Write the signal data into a single string.
+  std::string signal_string;
+  const std::vector<std::string>& signals = loader_->GetLoadingSignalData();
+
+  // TODO(petewil): Convert this to JSON, use json_writer.h

[Dmitry Titov, 2017/03/29 18:53:06]

either do this in this patch, or it should remain a TODO and content-type should
be text/plain

[Pete Williamson, 2017/03/29 22:05:42]

Changed to text/plain for now.

+  for (std::string signal : signals) {
+    signal_string += signal;
+    signal_string += "\r\n";

[Dmitry Titov, 2017/03/29 18:53:06]

does it have to be \r\n in the body?

[Pete Williamson, 2017/03/29 22:05:42]

No, but it does make it easier to read and parse until we have JSON.

[Dmitry Titov, 2017/03/29 23:26:29]

What I meant is why both, the regular "\n" would suffice. Seeing this special
combo can make others think it's essential to do both.

[Pete Williamson, 2017/03/31 00:29:48]

Ah, that's different.  I was copying what I saw elsewhere in generating the
file.  The same newline combination is used when we generate the footer and the
headers and the body (see MHTMLArchive.cpp, line 165)

[Dmitry Titov, 2017/03/31 18:05:53]

The other places are in header/boundary parts, it's regulated by MHTML spec. The
content of the body is regulated by Content-Type. For images, we just dump
binary data w/o any encoding. For test/plain, it's just text. it can contain
whatever CRLF sequences but I think it's a bit clearer to avoid CRLF in any text
bodies that we generate to avoid perception it's done for some specific purpose.
LF is usually enough.

[Pete Williamson, 2017/03/31 21:58:03]

OK, done, and added the Content-Transfer-Encoding as suggested by DewittJ

+  }
+  return signal_string;
+}
+
 void PrerenderingOffliner::OnSavePageDone(
     const SavePageRequest& request,
     SavePageResult save_result,