service worker: Move PageTransition/RedirectChain metrics out of SWControlleeRequestHandler.

These metrics don't work well at the ServiceWorkerControlleeRequestHandler
layer for S13nServiceWorker, for the reasons in the bug. Actually they are
pretty suspect for non-S13nServiceWorker too because we are recording them on
every URL request, not just committed page load, so we are double-counting on
redirects.

This patch:
1) Moves the PageTransition metric to ServiceWorkerPageLoadMetricsObserver.
This is a better fit also because page transition info is expected to be
removed from ResourceRequest as per the TODO there.
2) Removes the RedirectChain metric. We haven't been using it. If needed
we can always add one to ServiceWorkerPageLoadMetricsObserver alongside
PageTransition.

R=bmcquade, kinuko

Bug: 805805
Change-Id: I58029612f8c7697ec280bcc80bec97f6f64d9dd9
Reviewed-on: https://chromium-review.googlesource.com/886102Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533042}

chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc

@@ -13,6 +13,9 @@
 
 namespace internal {
 
+const char kHistogramServiceWorkerPageTransition[] =
+    "PageLoad.Clients.ServiceWorker.PageTransition";
+
 const char kHistogramServiceWorkerParseStart[] =
     "PageLoad.Clients.ServiceWorker.ParseTiming.NavigationToParseStart";
 const char kHistogramServiceWorkerParseStartForwardBack[] =
@@ -322,6 +325,17 @@ void ServiceWorkerPageLoadMetricsObserver::OnParseStart(
     const page_load_metrics::PageLoadExtraInfo& info) {
   if (!IsServiceWorkerControlled(info))
     return;
+
+  // TODO(falken): It may be cleaner to record page transition in OnCommit() but
+  // at that point we don't yet know if the page is controlled by a service
+  // worker. It should be possible to plumb the information there since the
+  // browser process already sends the controller service worker in the
+  // navigation commit IPC.
+  UMA_HISTOGRAM_ENUMERATION(
+      internal::kHistogramServiceWorkerPageTransition,
+      static_cast<int>(ui::PageTransitionStripQualifier(transition_)),
+      static_cast<int>(ui::PAGE_TRANSITION_LAST_CORE) + 1);
+
   if (WasStartedInForegroundOptionalEventInForeground(
           timing.parse_timing->parse_start, info)) {
     PAGE_LOAD_HISTOGRAM(internal::kHistogramServiceWorkerParseStart,

content/browser/service_worker/service_worker_controllee_request_handler.cc

@@ -400,8 +400,7 @@ void ServiceWorkerControlleeRequestHandler::
   DCHECK_NE(active_version->fetch_handler_existence(),
             ServiceWorkerVersion::FetchHandlerExistence::UNKNOWN);
   ServiceWorkerMetrics::CountControlledPageLoad(
-      active_version->site_for_uma(), stripped_url_, is_main_frame_load_,
-      url_job_->GetPageTransition(), url_job_->GetURLChainSize());
+      active_version->site_for_uma(), stripped_url_, is_main_frame_load_);
 
   bool is_forwarded =
       MaybeForwardToServiceWorker(url_job_.get(), active_version.get());
@@ -433,8 +432,7 @@ void ServiceWorkerControlleeRequestHandler::OnVersionStatusChanged(
   DCHECK_NE(version->fetch_handler_existence(),
             ServiceWorkerVersion::FetchHandlerExistence::UNKNOWN);
   ServiceWorkerMetrics::CountControlledPageLoad(
-      version->site_for_uma(), stripped_url_, is_main_frame_load_,
-      url_job_->GetPageTransition(), url_job_->GetURLChainSize());
+      version->site_for_uma(), stripped_url_, is_main_frame_load_);
 
   provider_host_->AssociateRegistration(registration,
                                         false /* notify_controllerchange */);

content/browser/service_worker/service_worker_metrics.cc

@@ -453,12 +453,9 @@ void ServiceWorkerMetrics::RecordDeleteAndStartOverResult(
                             result, NUM_DELETE_AND_START_OVER_RESULT_TYPES);
 }
 
-void ServiceWorkerMetrics::CountControlledPageLoad(
-    Site site,
-    const GURL& url,
-    bool is_main_frame_load,
-    ui::PageTransition page_transition,
-    size_t redirect_chain_length) {
+void ServiceWorkerMetrics::CountControlledPageLoad(Site site,
+                                                   const GURL& url,
+                                                   bool is_main_frame_load) {
   DCHECK_NE(site, Site::OTHER);
   UMA_HISTOGRAM_ENUMERATION("ServiceWorker.PageLoad", static_cast<int>(site),
                             static_cast<int>(Site::NUM_TYPES));
@@ -470,19 +467,6 @@ void ServiceWorkerMetrics::CountControlledPageLoad(
   if (ShouldExcludeSiteFromHistogram(site))
     return;
 
-  if (is_main_frame_load) {
-    UMA_HISTOGRAM_ENUMERATION(
-        "ServiceWorker.MainFramePageLoad.CoreTransition",
-        static_cast<int>(ui::PageTransitionStripQualifier(page_transition)),
-        static_cast<int>(ui::PAGE_TRANSITION_LAST_CORE) + 1);
-    // Currently the max number of HTTP redirects is 20 as defined in
-    // net::URLRequest::kMaxRedirects in
-    // net/url_request/url_request.h. So the max number of the chain
-    // length is 21.
-    UMA_HISTOGRAM_EXACT_LINEAR(
-        "ServiceWorker.MainFramePageLoad.RedirectChainLength",
-        redirect_chain_length, net::URLRequest::kMaxRedirects + 1);
-  }
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::BindOnce(&RecordURLMetricOnUI, "ServiceWorker.ControlledPageUrl",

content/browser/service_worker/service_worker_metrics.h

@@ -262,9 +262,7 @@ class ServiceWorkerMetrics {
   // Counts the number of page loads controlled by a Service Worker.
   static void CountControlledPageLoad(Site site,
                                       const GURL& url,
-                                      bool is_main_frame_load,
-                                      ui::PageTransition page_transition,
-                                      size_t redirect_chain_length);
+                                      bool is_main_frame_load);
 
   // Records the result of trying to start a worker. |is_installed| indicates
   // whether the version has been installed.

content/browser/service_worker/service_worker_url_job_wrapper.cc

@@ -61,27 +61,6 @@ bool ServiceWorkerURLJobWrapper::ShouldFallbackToNetwork() {
   }
 }
 
-ui::PageTransition ServiceWorkerURLJobWrapper::GetPageTransition() {
-  if (url_loader_job_) {
-    return url_loader_job_->GetPageTransition();
-  } else {
-    const ResourceRequestInfo* info =
-        ResourceRequestInfo::ForRequest(url_request_job_->request());
-    // ResourceRequestInfo may not be set in some tests.
-    if (!info)
-      return ui::PAGE_TRANSITION_LINK;
-    return info->GetPageTransition();
-  }
-}
-
-size_t ServiceWorkerURLJobWrapper::GetURLChainSize() const {
-  if (url_loader_job_) {
-    return url_loader_job_->GetURLChainSize();
-  } else {
-    return url_request_job_->request()->url_chain().size();
-  }
-}
-
 void ServiceWorkerURLJobWrapper::FailDueToLostController() {
   if (url_loader_job_) {
     url_loader_job_->FailDueToLostController();

content/browser/service_worker/service_worker_url_job_wrapper.h

@@ -76,14 +76,6 @@ class ServiceWorkerURLJobWrapper {
   // if needed later.
   void FailDueToLostController();
 
-  // Determines from the ResourceRequestInfo (or similar) the type of page
-  // transition used (for metrics purposes).
-  ui::PageTransition GetPageTransition();
-
-  // Determines the number of redirects used to handle the job (for metrics
-  // purposes).
-  size_t GetURLChainSize() const;
-
   // Returns true if the underlying job has been canceled or destroyed.
   bool WasCanceled() const;
 

content/browser/service_worker/service_worker_url_loader_job.cc

@@ -119,16 +119,6 @@ bool ServiceWorkerURLLoaderJob::ShouldFallbackToNetwork() {
   return response_type_ == ResponseType::FALLBACK_TO_NETWORK;
 }
 
-ui::PageTransition ServiceWorkerURLLoaderJob::GetPageTransition() {
-  NOTIMPLEMENTED();
-  return ui::PAGE_TRANSITION_LINK;
-}
-
-size_t ServiceWorkerURLLoaderJob::GetURLChainSize() const {
-  NOTIMPLEMENTED();
-  return 0;
-}
-
 void ServiceWorkerURLLoaderJob::FailDueToLostController() {
   NOTIMPLEMENTED();
 }

content/browser/service_worker/service_worker_url_loader_job.h

@@ -79,8 +79,6 @@ class CONTENT_EXPORT ServiceWorkerURLLoaderJob
   void ForwardToServiceWorker();
   bool ShouldFallbackToNetwork();
   void FailDueToLostController();
-  ui::PageTransition GetPageTransition();
-  size_t GetURLChainSize() const;
   void Cancel();
   bool WasCanceled() const;
 

tools/metrics/histograms/histograms.xml

@@ -58460,6 +58460,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="PageLoad.Clients.ServiceWorker.PageTransition"
+    enum="CorePageTransition">
+  <owner>falken@chromium.org</owner>
+  <summary>
+    The core transition type for main frame page loads controlled by a service
+    worker.
+  </summary>
+</histogram>
+
 <histogram name="PageLoad.Clients.SubresourceFilter.ActivationDecision"
     enum="SubresourceFilterActivationDecision">
   <owner>bmcquade@chromium.org</owner>
@@ -79621,6 +79630,10 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 
 <histogram name="ServiceWorker.MainFramePageLoad.CoreTransition"
     enum="CorePageTransition">
+  <obsolete>
+    Deprecated 2018-01 in favor of
+    PageLoad.Clients.ServiceWorker.PageTransition.
+  </obsolete>
   <owner>horo@chromium.org</owner>
   <summary>
     The transition type for main frame page loads controlled by a service
@@ -79630,6 +79643,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 
 <histogram name="ServiceWorker.MainFramePageLoad.RedirectChainLength"
     units="urls">
+  <obsolete>
+    Deprecated 2018-01.
+  </obsolete>
   <owner>horo@chromium.org</owner>
   <summary>
     Total length of the server redirects during main frame page loads controlled