Collect loading signal data from the background loader offliner

chrome/browser/android/offline_pages/background_loader_offliner.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "content/public/browser/web_contents_user_data.h"
 
 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:
   static void AddToWebContents(content::WebContents* webcontents,
                                BackgroundLoaderOffliner* offliner) {
     DCHECK(offliner);
     webcontents->SetUserData(UserDataKey(), std::unique_ptr<OfflinerData>(
                                                 new OfflinerData(offliner)));
   }
 
@@ skipping 115 matching lines @@
   }
 
   if (!OfflinePageModel::CanSaveURL(request.url())) {
     DVLOG(1) << "Not able to save page for requested url: " << request.url();
     return false;
   }
 
   if (!loader_)
     ResetState();
 
+  MarkLoadStartTime();
+
   // Track copy of pending request.
   pending_request_.reset(new SavePageRequest(request));
   completion_callback_ = completion_callback;
   progress_callback_ = progress_callback;
 
   // Listen for app foreground/background change.
   app_listener_.reset(new base::android::ApplicationStatusListener(
       base::Bind(&BackgroundLoaderOffliner::OnApplicationStateChange,
                  weak_ptr_factory_.GetWeakPtr())));
 
@@ skipping 39 matching lines @@
       if (save_state_ == NONE) {
         did_snapshot_on_last_retry_ = true;
         StartSnapshot();
       }
       return true;
     }
   }
   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() {
   if (!pending_request_.get()) {
     DVLOG(1) << "DidStopLoading called even though no pending request.";
     return;
   }
 
+  // Add this signal to signal_data_.
+  AddLoadingSignal("DocumentOnLoadCompletedInMainFrame");
+
   snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
 }
 
 void BackgroundLoaderOffliner::RenderProcessGone(
     base::TerminationStatus status) {
   if (pending_request_) {
     SavePageRequest request(*pending_request_.get());
     switch (status) {
       case base::TERMINATION_STATUS_OOM:
       case base::TERMINATION_STATUS_PROCESS_CRASHED:
@@ skipping 47 matching lines @@
     progress_callback_.Run(*pending_request_, network_bytes_);
   }
 }
 
 void BackgroundLoaderOffliner::StartSnapshot() {
   if (!pending_request_.get()) {
     DVLOG(1) << "Pending request was cleared during delay.";
     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) {
     Offliner::RequestStatus status;
     switch (page_load_state_) {
       case RETRIABLE:
         status = Offliner::RequestStatus::LOADING_FAILED;
         break;
       case NONRETRIABLE:
         status = Offliner::RequestStatus::LOADING_FAILED_NO_RETRY;
         break;
       case DELAY_RETRY:
         status = Offliner::RequestStatus::LOADING_FAILED_NO_NEXT;
         break;
       default:
         // We should've already checked for Success before entering here.
         NOTREACHED();
         status = Offliner::RequestStatus::LOADING_FAILED;
     }
+
     completion_callback_.Run(request, status);
     ResetState();
     return;
   }
 
   save_state_ = SAVING;
   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));
 
   OfflinePageModel::SavePageParams params;
   params.url = web_contents->GetLastCommittedURL();
   params.client_id = request.client_id();
   params.proposed_offline_id = request.request_id();
   params.is_background = true;
 
   // Pass in the original URL if it's different from last committed
@@ skipping 75 matching lines @@
 
 void BackgroundLoaderOffliner::HandleApplicationStateChangeCancel(
     const SavePageRequest& request,
     int64_t offline_id) {
   // If for some reason the request was reset during while waiting for callback
   // ignore the completion callback.
   if (pending_request_ && pending_request_->request_id() != offline_id)
     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);