Collect loading signal data from the background loader offliner

chrome/browser/android/offline_pages/background_loader_offliner.cc

Index: chrome/browser/android/offline_pages/background_loader_offliner.cc
diff --git a/chrome/browser/android/offline_pages/background_loader_offliner.cc b/chrome/browser/android/offline_pages/background_loader_offliner.cc
index 3ff7ad6e177676c6b2319a9e80513ae8ad16b41f..d294371fcf19869007e217383be754c6cc7cc409 100644
--- a/chrome/browser/android/offline_pages/background_loader_offliner.cc
+++ b/chrome/browser/android/offline_pages/background_loader_offliner.cc
@@ -4,16 +4,20 @@
 
 #include "chrome/browser/android/offline_pages/background_loader_offliner.h"
 
+#include "base/json/json_writer.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
+#include "base/time/time.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offliner_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/offline_pages/core/background/offliner_policy.h"
 #include "components/offline_pages/core/background/save_page_request.h"
 #include "components/offline_pages/core/client_namespace_constants.h"
+#include "components/offline_pages/core/offline_page_feature.h"
 #include "components/offline_pages/core/offline_page_model.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/mhtml_extra_parts.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
@@ -24,6 +28,10 @@ namespace offline_pages {
 namespace {
 const long kOfflinePageDelayMs = 2000;
 const long kOfflineDomContentLoadedMs = 25000;
+const char kContentType[] = "text/plain";
+const char kContentTransferEncodingBinary[] =
+    "Content-Transfer-Encoding: binary";
+const char kXHeaderForSignals[] = "X-Chrome-Loading-Metrics-Data: 1";
 
 class OfflinerData : public content::WebContentsUserData<OfflinerData> {
  public:
@@ -159,6 +167,8 @@ bool BackgroundLoaderOffliner::LoadAndSave(
   if (!loader_)
     ResetState();
 
+  MarkLoadStartTime();
+
   // Track copy of pending request.
   pending_request_.reset(new SavePageRequest(request));
   completion_callback_ = completion_callback;
@@ -218,9 +228,16 @@ bool BackgroundLoaderOffliner::HandleTimeout(const SavePageRequest& request) {
   return false;
 }
 
+void BackgroundLoaderOffliner::MarkLoadStartTime() {
+  load_start_time_ = base::TimeTicks::Now();
+}
+
 void BackgroundLoaderOffliner::DocumentAvailableInMainFrame() {
   snapshot_controller_->DocumentAvailableInMainFrame();
   is_low_bar_met_ = true;
+
+  // Add this signal to signal_data_.
+  AddLoadingSignal("DocumentAvailableInMainFrame");
 }
 
 void BackgroundLoaderOffliner::DocumentOnLoadCompletedInMainFrame() {
@@ -229,6 +246,9 @@ void BackgroundLoaderOffliner::DocumentOnLoadCompletedInMainFrame() {
     return;
   }
 
+  // Add this signal to signal_data_.
+  AddLoadingSignal("DocumentOnLoadCompletedInMainFrame");
+
   snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
 }
 
@@ -296,6 +316,9 @@ void BackgroundLoaderOffliner::StartSnapshot() {
     return;
   }
 
+  // Add this signal to signal_data_.
+  AddLoadingSignal("Snapshotting");
+
   SavePageRequest request(*pending_request_.get());
   // If there was an error navigating to page, return loading failed.
   if (page_load_state_ != SUCCESS) {
@@ -315,6 +338,7 @@ void BackgroundLoaderOffliner::StartSnapshot() {
         NOTREACHED();
         status = Offliner::RequestStatus::LOADING_FAILED;
     }
+
     completion_callback_.Run(request, status);
     ResetState();
     return;
@@ -324,6 +348,27 @@ void BackgroundLoaderOffliner::StartSnapshot() {
   content::WebContents* web_contents(
       content::WebContentsObserver::web_contents());
 
+  // Add loading signal into the MHTML that will be generated if the command

[fgorski, 2017/04/19 15:45:51]

This whole section is a copy-paste from prerendering offliner, therefore I don't
believe it belongs either here or there.

Please extract the code to its own class and call it from here and PO.

[Pete Williamson, 2017/04/19 17:45:49]

We will be removing the prerendering offliner when we are happy with the
background loading offliner. I think that the code is better where it is than
refactored out.  Do you still think this would be better pulled out, even once
the prerendering offliner is gone?

[fgorski, 2017/04/20 16:59:02]

As we discussed yesterday. Duplication still tells me that this code belongs
someplace else. I think SignalCollector interface would do a great deal here for
collection and the it should allow a serializer to attach to it for this
printout. I am OK with not changing it now, but we should pay attention to
things like that.
Let's add a note in signal collector document so that we don't lose track of
this.

[Pete Williamson, 2017/04/20 17:06:36]

Done.

+  // line flag is set for it.
+  if (IsOfflinePagesLoadSignalCollectingEnabled()) {
+    // Stash loading signals for writing when we write out the MHTML.
+    std::string headers = base::StringPrintf(
+        "%s\r\n%s\r\n\r\n", kContentTransferEncodingBinary, kXHeaderForSignals);

[chili, 2017/04/19 06:40:29]

Just out of curiosity why \r\n rather than just \n? Does the \r provide any
additional advantage/readability?

[Pete Williamson, 2017/04/19 17:45:48]

It makes the headers in MHTML comply with the spec.

As far as whether it is more readable, it depends on the system.  On Windows,
CRLF is the norm for text files, but not on linux.  In either case, we expect
this to be read by a machine, not a human, so keeping consistent with the other
headers in the MHTML is the higher priority than readability.

+    std::string body;
+    base::JSONWriter::Write(signal_data_, &body);
+    std::string content_type = kContentType;
+    std::string content_location = base::StringPrintf(
+        "cid:signal-data-%" PRId64 "@mhtml.blink", request.request_id());
+
+    content::MHTMLExtraParts* extra_parts =
+        content::MHTMLExtraParts::FromWebContents(web_contents);
+    DCHECK(extra_parts);
+    if (extra_parts != nullptr) {
+      extra_parts->AddExtraMHTMLPart(content_type, content_location,
+                                     headers + body);
+    }
+  }
+
   std::unique_ptr<OfflinePageArchiver> archiver(
       new OfflinePageMHTMLArchiver(web_contents));
 
@@ -419,6 +464,14 @@ void BackgroundLoaderOffliner::HandleApplicationStateChangeCancel(
     return;
   completion_callback_.Run(request, RequestStatus::FOREGROUND_CANCELED);
 }
+
+void BackgroundLoaderOffliner::AddLoadingSignal(const char* signal_name) {
+  base::TimeTicks current_time = base::TimeTicks::Now();
+  base::TimeDelta delay_so_far = current_time - load_start_time_;
+  double delay = delay_so_far.InMilliseconds();
+  signal_data_.SetDouble(signal_name, delay);
+}
+
 }  // namespace offline_pages
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::OfflinerData);