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

chrome/browser/offline_pages/background_loader_offliner.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/offline_pages/background_loader_offliner.h"
 
 #include "base/bind.h"
 #include "base/json/json_writer.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
 #include "base/threading/thread_task_runner_handle.h"
 #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"
 #include "components/offline_pages/core/client_namespace_constants.h"
 #include "components/offline_pages/core/offline_page_feature.h"
 #include "components/offline_pages/core/offline_page_model.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/mhtml_extra_parts.h"
 #include "content/public/browser/navigation_handle.h"
 #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 {
 
 namespace {
 const char kContentType[] = "text/plain";
 const char kContentTransferEncodingBinary[] =
     "Content-Transfer-Encoding: binary";
 const char kXHeaderForSignals[] = "X-Chrome-Loading-Metrics-Data: 1";
 
 class OfflinerData : public content::WebContentsUserData<OfflinerData> {
@@ skipping 29 matching lines @@
   return adjusted_histogram_name;
 }
 
 void RecordErrorCauseUMA(const ClientId& client_id, net::Error error_code) {
   UMA_HISTOGRAM_SPARSE_SLOWLY(
       AddHistogramSuffix(client_id,
                          "OfflinePages.Background.BackgroundLoadingFailedCode"),
       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) {
   completion_callback.Run(canceled_request,
                           Offliner::RequestStatus::FOREGROUND_CANCELED);
 }
 
 }  // namespace
 
 BackgroundLoaderOffliner::BackgroundLoaderOffliner(
@@ skipping 218 matching lines @@
     RecordErrorCauseUMA(pending_request_->client_id(),
                         navigation_handle->GetNetErrorCode());
     switch (navigation_handle->GetNetErrorCode()) {
       case net::ERR_INTERNET_DISCONNECTED:
         page_load_state_ = DELAY_RETRY;
         break;
       default:
         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(
     std::unique_ptr<SnapshotController> controller) {
   snapshot_controller_ = std::move(controller);
 }
 
 void BackgroundLoaderOffliner::OnNetworkBytesChanged(int64_t bytes) {
   if (pending_request_ && save_state_ != SAVING) {
     network_bytes_ += bytes;
@@ skipping 156 matching lines @@
   // Given the choice between int and double, we choose to implicitly convert to
   // a double since it maintains more precision (we can get a longer time in
   // milliseconds than we can with a 2 bit int, 53 bits vs 32).
   double delay = delay_so_far.InMilliseconds();
   signal_data_.SetDouble(signal_name, delay);
 }
 
 }  // namespace offline_pages
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::OfflinerData);