Various cleaups for AMP page load metrics.

chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
index 427b68ed5e27f3149b8a5978af2d3a5e574b2d11..d1a8253c9e6e3e5cfa4d0068b15d382d94efc406 100644
--- a/chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
@@ -17,35 +17,49 @@
 
 namespace {
 
+using AMPViewType = AMPPageLoadMetricsObserver::AMPViewType;
+
+#define RECORD_HISTOGRAM_FOR_TYPE(name, amp_view_type, value)                  \
+  do {                                                                         \
+    PAGE_LOAD_HISTOGRAM(name, value);                                          \
+    switch (amp_view_type) {                                                   \
+      case AMPViewType::AMP_CACHE:                                             \
+        PAGE_LOAD_HISTOGRAM(std::string(name).append(".AmpCache"), value);     \
+        break;                                                                 \
+      case AMPViewType::GOOGLE_SEARCH_AMP_VIEWER:                              \
+        PAGE_LOAD_HISTOGRAM(std::string(name).append(".GoogleSearch"), value); \
+        break;                                                                 \
+      case AMPViewType::GOOGLE_NEWS_AMP_VIEWER:                                \
+        PAGE_LOAD_HISTOGRAM(std::string(name).append(".GoogleNews"), value);   \
+        break;                                                                 \
+      case AMPViewType::NONE:                                                  \
+        NOTREACHED();                                                          \
+        PAGE_LOAD_HISTOGRAM(std::string(name).append(".None"), value);         \

[RyanSturm, 2017/05/15 20:42:06]

Remove the PAGE_LOAD_HISTOGRAM from this case.

[Bryan McQuade, 2017/05/16 03:59:14]

Done

+        break;                                                                 \
+    }                                                                          \
+  } while (false)
+
 const char kHistogramAMPDOMContentLoadedEventFired[] =
-    "PageLoad.Clients.AMPCache2.DocumentTiming."
+    "PageLoad.Clients.AMP.DocumentTiming."
     "NavigationToDOMContentLoadedEventFired";
 const char kHistogramAMPFirstLayout[] =
-    "PageLoad.Clients.AMPCache2.DocumentTiming.NavigationToFirstLayout";
+    "PageLoad.Clients.AMP.DocumentTiming.NavigationToFirstLayout";
 const char kHistogramAMPLoadEventFired[] =
-    "PageLoad.Clients.AMPCache2.DocumentTiming.NavigationToLoadEventFired";
+    "PageLoad.Clients.AMP.DocumentTiming.NavigationToLoadEventFired";
 const char kHistogramAMPFirstContentfulPaint[] =
-    "PageLoad.Clients.AMPCache2.PaintTiming."
+    "PageLoad.Clients.AMP.PaintTiming."
     "NavigationToFirstContentfulPaint";
 const char kHistogramAMPParseStart[] =
-    "PageLoad.Clients.AMPCache2.ParseTiming.NavigationToParseStart";
+    "PageLoad.Clients.AMP.ParseTiming.NavigationToParseStart";
 
 // Host pattern for AMP Cache URLs.
 // See https://developers.google.com/amp/cache/overview#amp-cache-url-format
 // for a definition of the format of AMP Cache URLs.
-const char kAmpCacheHost[] = "cdn.ampproject.org";
+const char kAmpCacheHostSuffix[] = "cdn.ampproject.org";
 
 // Pattern for the path of Google AMP Viewer URLs.
-const char kGoogleAmpViewerPathPattern[] = "/amp/";
-
-bool IsAMPCacheURL(const GURL& url) {
-  return url.host() == kAmpCacheHost ||
-         (google_util::IsGoogleDomainUrl(
-              url, google_util::DISALLOW_SUBDOMAIN,
-              google_util::DISALLOW_NON_STANDARD_PORTS) &&
-          base::StartsWith(url.path(), kGoogleAmpViewerPathPattern,
-                           base::CompareCase::SENSITIVE));
-}
+const char kGoogleSearchAmpViewerPathPrefix[] = "/amp/";
+const char kGoogleNewsAmpViewerPath[] = "/news/amp";
 
 }  // namespace
 
@@ -56,8 +70,9 @@ AMPPageLoadMetricsObserver::~AMPPageLoadMetricsObserver() {}
 page_load_metrics::PageLoadMetricsObserver::ObservePolicy
 AMPPageLoadMetricsObserver::OnCommit(
     content::NavigationHandle* navigation_handle) {
-  return IsAMPCacheURL(navigation_handle->GetURL()) ? CONTINUE_OBSERVING
-                                                    : STOP_OBSERVING;
+  view_type_ = GetAMPViewType(navigation_handle->GetURL());
+  return (view_type_ != AMPViewType::NONE) ? CONTINUE_OBSERVING
+                                           : STOP_OBSERVING;
 }
 
 void AMPPageLoadMetricsObserver::OnDomContentLoadedEventStart(
@@ -67,8 +82,8 @@ void AMPPageLoadMetricsObserver::OnDomContentLoadedEventStart(
           timing.document_timing.dom_content_loaded_event_start, info)) {
     return;
   }
-  PAGE_LOAD_HISTOGRAM(
-      kHistogramAMPDOMContentLoadedEventFired,
+  RECORD_HISTOGRAM_FOR_TYPE(
+      kHistogramAMPDOMContentLoadedEventFired, view_type_,
       timing.document_timing.dom_content_loaded_event_start.value());
 }
 
@@ -79,8 +94,8 @@ void AMPPageLoadMetricsObserver::OnLoadEventStart(
           timing.document_timing.load_event_start, info)) {
     return;
   }
-  PAGE_LOAD_HISTOGRAM(kHistogramAMPLoadEventFired,
-                      timing.document_timing.load_event_start.value());
+  RECORD_HISTOGRAM_FOR_TYPE(kHistogramAMPLoadEventFired, view_type_,
+                            timing.document_timing.load_event_start.value());
 }
 
 void AMPPageLoadMetricsObserver::OnFirstLayout(
@@ -90,8 +105,8 @@ void AMPPageLoadMetricsObserver::OnFirstLayout(
           timing.document_timing.first_layout, info)) {
     return;
   }
-  PAGE_LOAD_HISTOGRAM(kHistogramAMPFirstLayout,
-                      timing.document_timing.first_layout.value());
+  RECORD_HISTOGRAM_FOR_TYPE(kHistogramAMPFirstLayout, view_type_,
+                            timing.document_timing.first_layout.value());
 }
 
 void AMPPageLoadMetricsObserver::OnFirstContentfulPaintInPage(
@@ -101,8 +116,8 @@ void AMPPageLoadMetricsObserver::OnFirstContentfulPaintInPage(
           timing.paint_timing.first_contentful_paint, info)) {
     return;
   }
-  PAGE_LOAD_HISTOGRAM(kHistogramAMPFirstContentfulPaint,
-                      timing.paint_timing.first_contentful_paint.value());
+  RECORD_HISTOGRAM_FOR_TYPE(kHistogramAMPFirstContentfulPaint, view_type_,
+                            timing.paint_timing.first_contentful_paint.value());
 }
 
 void AMPPageLoadMetricsObserver::OnParseStart(
@@ -112,6 +127,33 @@ void AMPPageLoadMetricsObserver::OnParseStart(
           timing.parse_timing.parse_start, info)) {
     return;
   }
-  PAGE_LOAD_HISTOGRAM(kHistogramAMPParseStart,
-                      timing.parse_timing.parse_start.value());
+  RECORD_HISTOGRAM_FOR_TYPE(kHistogramAMPParseStart, view_type_,
+                            timing.parse_timing.parse_start.value());
+}
+
+// static
+AMPPageLoadMetricsObserver::AMPViewType
+AMPPageLoadMetricsObserver::GetAMPViewType(const GURL& url) {
+  if (base::EndsWith(url.host(), kAmpCacheHostSuffix,
+                     base::CompareCase::INSENSITIVE_ASCII)) {
+    return AMPViewType::AMP_CACHE;
+  }
+
+  base::StringPiece google_hostname_prefix;
+  if (!page_load_metrics::IsGoogleHostnameAndGetPrefix(
+          url, &google_hostname_prefix)) {
+    return AMPViewType::NONE;
+  }
+
+  if (google_hostname_prefix == "www" &&

[RyanSturm, 2017/05/15 20:42:06]

As a note: it's a little strange that "www" is inline and
kGoogleSearchAmpViewerPathPrefix is defined above. Let's make them both inline
constants? Same with "news" & kGoogleNewsAmpViewerPath below

[Bryan McQuade, 2017/05/16 03:59:15]

Done

+      base::StartsWith(url.path_piece(), kGoogleSearchAmpViewerPathPrefix,
+                       base::CompareCase::SENSITIVE)) {

[RyanSturm, 2017/05/15 20:42:06]

Not sure whether this should be case sensitive or not.

[Bryan McQuade, 2017/05/16 03:59:14]

My understanding is that hostnames are not case sensitive while paths are by
default case sensitive. I tested a capitalized version of this string and got a
404.

+    return AMPViewType::GOOGLE_SEARCH_AMP_VIEWER;
+  }
+
+  if (google_hostname_prefix == "news" &&
+      url.path_piece() == kGoogleNewsAmpViewerPath) {
+    return AMPViewType::GOOGLE_NEWS_AMP_VIEWER;
+  }
+  return AMPViewType::NONE;
 }