Add UMA to determine how often we offline a page with previews.

chrome/browser/offline_pages/background_loader_offliner.cc

Index: chrome/browser/offline_pages/background_loader_offliner.cc
diff --git a/chrome/browser/offline_pages/background_loader_offliner.cc b/chrome/browser/offline_pages/background_loader_offliner.cc
index 7860f5ead47813c7344b9f438ac634b6232ec01b..132f912d428459290173137428ab7ec6bafea131 100644
--- a/chrome/browser/offline_pages/background_loader_offliner.cc
+++ b/chrome/browser/offline_pages/background_loader_offliner.cc
@@ -12,6 +12,7 @@
 #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/loader/chrome_navigation_data.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"
@@ -24,6 +25,7 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_user_data.h"
+#include "content/public/common/previews_state.h"
 
 namespace offline_pages {
 
@@ -73,6 +75,18 @@ void RecordErrorCauseUMA(const ClientId& client_id, net::Error error_code) {
       std::abs(error_code));
 }
 
+void RecordOffliningPreviewsUMA(const ClientId& client_id,
+                                content::PreviewsState previews_state) {
+  int previews_enabling = 0;

[chili, 2017/06/02 01:38:07]

alternate names: preview_state_numeration? preview_state? is_previews_enabled?

[Pete Williamson, 2017/06/02 21:40:32]

picked "is_previews_enabled"

+  if (previews_state != content::PreviewsTypes::PREVIEWS_OFF)
+    previews_enabling = 1;
+
+  UMA_HISTOGRAM_ENUMERATION(
+      AddHistogramSuffix(client_id,
+                         "OfflinePages.Background.OffliningPreviewStatus"),

[RyanSturm, 2017/06/02 20:50:33]

As a note, if you care about offline previews (which I don't think you do, since
you don't need to re-offline the page), you would need to add extra handling.
Even if you do think it's a case you might care about, a follow-up CL might be
better for it.

[Pete Williamson, 2017/06/02 21:40:32]

Noted.  I don't think we care about it.

+      previews_enabling, 2);
+}
+
 void HandleLoadTerminationCancel(
     const Offliner::CompletionCallback& completion_callback,
     const SavePageRequest& canceled_request) {
@@ -311,6 +325,21 @@ void BackgroundLoaderOffliner::DidFinishNavigation(
         page_load_state_ = RETRIABLE;
     }
   }
+
+  // Record UMA if we are offlining a previvew instead of an unmodified page.
+  // TODO(petewil): Today ChromeNavigationData is the only subclass of
+  // NavigationData.  What can I do to make sure new subclasses don't break
+  // this cast someday?  Move previews_state() to the NavigationData interface?
+  // Use some form of RTTI to know what I have before I cast it?

[Pete Williamson, 2017/06/02 00:47:37]

Reviewers:  What is your preference here?  I'm thinking of adding the
previews_state function to NavigationData, but I'm open to other options.

[chili, 2017/06/02 01:38:07]

I would make a function called IsAnyPreviewsEnabled() in NavigationData (perhaps
ryan can suggest better location)

[RyanSturm, 2017/06/02 20:50:33]

So the basic idea is that Chrome will only ever implement one type of subclass,
but other projects that use content/ may have their own implementation.

Specifically, there are comments in navigation_handle.h that state that this
NavgiationData will be the same as the one returned from GetNavigationData.

It's an ugly layering contract, but you can add the same comment as from here
(https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...)
if you like.

[RyanSturm, 2017/06/02 20:56:29]

Also, if you like, I would not be opposed to adding a static method to
ChromeNavigationData that takes a NavigationData. Something like
"ChromeNavigationData* ChromeNavigationdata(NavigationData* navigation_data)"
that moves the cast into the ChromeNavigationData code.

Also, I think static_cast would be better than reinterpret_cast here.

[RyanSturm, 2017/06/02 20:57:56]

Ooops: typo above:

s/"ChromeNavigationData* ChromeNavigationdata(NavigationData*
navigation_data)"/"ChromeNavigationData*
ChromeNavigationdata::FromNavigationData(NavigationData* navigation_data)"

[Pete Williamson, 2017/06/02 21:40:32]

Changed to static cast.  Decided not to move the code into ChromeNavigationData
for now, but I can if you prefer it.

+  ChromeNavigationData* navigation_data =
+      reinterpret_cast<ChromeNavigationData*>(
+          navigation_handle->GetNavigationData());
+
+  content::PreviewsState previews_state = content::PreviewsTypes::PREVIEWS_OFF;
+  if (navigation_data)
+    previews_state = navigation_data->previews_state();
+
+  RecordOffliningPreviewsUMA(pending_request_->client_id(), previews_state);
 }
 
 void BackgroundLoaderOffliner::SetSnapshotControllerForTest(