[NoStatePrefetch] Track redirects in UMA

chrome/browser/prerender/prerender_histograms.cc

Index: chrome/browser/prerender/prerender_histograms.cc
diff --git a/chrome/browser/prerender/prerender_histograms.cc b/chrome/browser/prerender/prerender_histograms.cc
index fcc0501659996c99654585cf4ff13dc971455112..72bf52e80ef402a9d984d70e8739e6b0c5cc4ce6 100644
--- a/chrome/browser/prerender/prerender_histograms.cc
+++ b/chrome/browser/prerender/prerender_histograms.cc
@@ -22,14 +22,25 @@ namespace {
 // Hence, existing enumerated constants should never be deleted or reordered,
 // and new constants should only be appended at the end of the enumeration.
 enum NoStatePrefetchResourceType {
-  MAIN_RESOURCE_CACHEABLE = 0,
-  MAIN_RESOURCE_NO_STORE = 1,
-  SUB_RESOURCE_CACHEABLE = 2,
-  SUB_RESOURCE_NO_STORE = 3,
+  SUB_RESOURCE_CACHEABLE = 0,
+  SUB_RESOURCE_NO_STORE = 1,
+  SUB_RESOURCE_REDIRECT_CACHEABLE = 2,
+  SUB_RESOURCE_REDIRECT_NO_STORE = 3,
+  MAIN_RESOURCE_CACHEABLE = 4,
+  MAIN_RESOURCE_NO_STORE = 5,
+  MAIN_RESOURCE_REDIRECT_CACHEABLE = 6,
+  MAIN_RESOURCE_REDIRECT_NO_STORE = 7,
 
   NO_STATE_PREFETCH_RESOURCE_TYPE_COUNT  // Must be the last.
 };
 
+NoStatePrefetchResourceType GetResourceType(bool is_main_resource,
+                                            bool is_redirect,
+                                            bool is_no_store) {
+  return static_cast<NoStatePrefetchResourceType>(

[pasko, 2016/09/02 16:34:48]

This was confusing to me:
* casting explicitly to the enum that gets casted back when FactoryGet is called
* not using the values declared in the enum except one

I think the code will be more straightforward if we return int32_t (or perhaps
base::HistogramBase::Sample?)

This would be equivalent, but would better suggest that we are using bit fields:

sample = is_main_resource << 2 | is_redirect << 1 | is_no_store;
kPossibleSampleValues = 1 << 3;

[droger, 2016/09/05 12:38:47]

Done using a bitfield.

+      4 * is_main_resource + 2 * is_redirect + is_no_store);
+}
+
 // Time window for which we will record windowed PLTs from the last observed
 // link rel=prefetch tag. This is not intended to be the same as the prerender
 // ttl, it's just intended to be a window during which a prerender has likely
@@ -396,12 +407,11 @@ void PrerenderHistograms::RecordNetworkBytes(Origin origin,
 
 void PrerenderHistograms::RecordResourcePrefetch(Origin origin,
                                                  bool is_main_resource,
+                                                 bool is_redirect,
                                                  bool is_no_store) const {
   DCHECK(thread_checker_.CalledOnValidThread());
   NoStatePrefetchResourceType type =
-      is_main_resource
-          ? (is_no_store ? MAIN_RESOURCE_NO_STORE : MAIN_RESOURCE_CACHEABLE)
-          : (is_no_store ? SUB_RESOURCE_NO_STORE : SUB_RESOURCE_CACHEABLE);
+      GetResourceType(is_main_resource, is_redirect, is_no_store);
   DCHECK_LT(type, NO_STATE_PREFETCH_RESOURCE_TYPE_COUNT);
 
   std::string histogram_name =
@@ -416,6 +426,26 @@ void PrerenderHistograms::RecordResourcePrefetch(Origin origin,
   histogram_pointer->Add(type);
 }
 
+void PrerenderHistograms::RecordPrefetchRedirectCount(
+    Origin origin,
+    bool is_main_resource,
+    size_t redirect_count) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  std::string histogram_base_name = base::StringPrintf(
+      "NoStatePrefetch%sResourceRedirects", is_main_resource ? "Main" : "Sub");
+  std::string histogram_name =
+      GetHistogramName(origin, IsOriginWash(), histogram_base_name);
+
+  // Unrolls UMA_HISTOGRAM_CUSTOM_COUNTS, required to support dynamic histogram

[pasko, 2016/09/02 16:34:48]

can we avoid repeating ourselves twice here?

void PrerenderHistograms::RecordEnumHistogram(std::sting name,
base::HistogramBase::Sample sample, base::HistogramBase::Sample
possible_value_count) {
  DCHECK(thread_checker_.CalledOnValidThread());
  base::LinearHistogram::FactoryGet(name, 1, possible_value_count,
possible_value_count + 1,
base::HistogramBase::kUmaTargetedHistogramFlag)->Add(sample);
}

[droger, 2016/09/05 12:38:47]

Done.

+  // name.
+  const int kMaxRedirectCount = 10;
+  base::HistogramBase* histogram_pointer = base::LinearHistogram::FactoryGet(
+      histogram_name, 1, kMaxRedirectCount, kMaxRedirectCount + 1,
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram_pointer->Add(redirect_count);
+}
+
 bool PrerenderHistograms::IsOriginWash() const {
   if (!WithinWindow())
     return false;