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..fbc41f2f55a8773d4ecea0376741ff493d114379 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,50 @@
 
 namespace {
 
+using AMPViewType = AMPPageLoadMetricsObserver::AMPViewType;
+
+#define RECORD_HISTOGRAM_FOR_TYPE(name, amp_view_type, value)                 \

[RyanSturm, 2017/05/16 16:12:22]

Slight nit:

This macro is defined before kHistogramPrefix is declared/defined, but it
references kHistogramPrefix in it. That works because it's a macro and it is
only used below the definition of kHistogramPrefix, but it feels slightly
confusing from a readability stand point.

[Bryan McQuade, 2017/05/16 17:29:20]

Done

+  do {                                                                        \
+    PAGE_LOAD_HISTOGRAM(std::string(kHistogramPrefix).append(name), value);   \
+    switch (amp_view_type) {                                                  \
+      case AMPViewType::AMP_CACHE:                                            \
+        PAGE_LOAD_HISTOGRAM(                                                  \
+            std::string(kHistogramPrefix).append("AmpCache.").append(name),   \
+            value);                                                           \
+        break;                                                                \
+      case AMPViewType::GOOGLE_SEARCH_AMP_VIEWER:                             \
+        PAGE_LOAD_HISTOGRAM(std::string(kHistogramPrefix)                     \
+                                .append("GoogleSearch.")                      \
+                                .append(name),                                \
+                            value);                                           \
+        break;                                                                \
+      case AMPViewType::GOOGLE_NEWS_AMP_VIEWER:                               \
+        PAGE_LOAD_HISTOGRAM(                                                  \
+            std::string(kHistogramPrefix).append("GoogleNews.").append(name), \
+            value);                                                           \
+        break;                                                                \
+      case AMPViewType::NONE:                                                 \
+        NOTREACHED();                                                         \
+        break;                                                                \
+    }                                                                         \
+  } while (false)
+
+const char kHistogramPrefix[] = "PageLoad.Clients.AMP.";
+
 const char kHistogramAMPDOMContentLoadedEventFired[] =
-    "PageLoad.Clients.AMPCache2.DocumentTiming."
-    "NavigationToDOMContentLoadedEventFired";
+    "DocumentTiming.NavigationToDOMContentLoadedEventFired";
 const char kHistogramAMPFirstLayout[] =
-    "PageLoad.Clients.AMPCache2.DocumentTiming.NavigationToFirstLayout";
+    "DocumentTiming.NavigationToFirstLayout";
 const char kHistogramAMPLoadEventFired[] =
-    "PageLoad.Clients.AMPCache2.DocumentTiming.NavigationToLoadEventFired";
+    "DocumentTiming.NavigationToLoadEventFired";
 const char kHistogramAMPFirstContentfulPaint[] =
-    "PageLoad.Clients.AMPCache2.PaintTiming."
-    "NavigationToFirstContentfulPaint";
-const char kHistogramAMPParseStart[] =
-    "PageLoad.Clients.AMPCache2.ParseTiming.NavigationToParseStart";
+    "PaintTiming.NavigationToFirstContentfulPaint";
+const char kHistogramAMPParseStart[] = "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";
-
-// 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 kAmpCacheHostSuffix[] = "cdn.ampproject.org";
 
 }  // namespace
 
@@ -56,8 +71,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 +83,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 +95,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 +106,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 +117,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 +128,32 @@ 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::Optional<std::string> google_hostname_prefix =
+      page_load_metrics::GetGoogleHostnamePrefix(url);
+  if (!google_hostname_prefix.has_value())
+    return AMPViewType::NONE;
+
+  if (google_hostname_prefix.value() == "www" &&
+      base::StartsWith(url.path_piece(), "/amp/",
+                       base::CompareCase::SENSITIVE)) {
+    return AMPViewType::GOOGLE_SEARCH_AMP_VIEWER;
+  }
+
+  if (google_hostname_prefix.value() == "news" &&
+      url.path_piece() == "/news/amp") {
+    return AMPViewType::GOOGLE_NEWS_AMP_VIEWER;
+  }
+  return AMPViewType::NONE;
 }