Converging the two previews type enums

chrome/browser/previews/previews_infobar_delegate.cc

Index: chrome/browser/previews/previews_infobar_delegate.cc
diff --git a/chrome/browser/previews/previews_infobar_delegate.cc b/chrome/browser/previews/previews_infobar_delegate.cc
index d4eb931215f6f483cf28aac1e5eeea3a0b87b59c..ab3322524964ae7b3665104d09972b6cd4eab5d9 100644
--- a/chrome/browser/previews/previews_infobar_delegate.cc
+++ b/chrome/browser/previews/previews_infobar_delegate.cc
@@ -4,7 +4,7 @@
 
 #include "chrome/browser/previews/previews_infobar_delegate.h"
 
-#include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram.h"
 #include "base/optional.h"
 #include "chrome/browser/android/android_theme_resources.h"
 #include "chrome/browser/infobars/infobar_service.h"
@@ -26,29 +26,67 @@
 
 namespace {
 
-// Key of the UMA Previews.InfoBarAction.LoFi histogram.
-const char kUMAPreviewsInfoBarActionLoFi[] = "Previews.InfoBarAction.LoFi";
+void RecordPreviewsInfoBarAction(
+    previews::PreviewsType previews_type,
+    PreviewsInfoBarDelegate::PreviewsInfoBarAction action) {
+  int32_t max_limit =
+      static_cast<int32_t>(PreviewsInfoBarDelegate::INFOBAR_INDEX_BOUNDARY);

[megjablon, 2017/05/11 23:49:45]

Can we just use INFOBAR_INDEX_BOUNDARY without casting? I see other
FactoryGet()'s just using the enum max directly.

[RyanSturm, 2017/05/15 16:23:08]

I'd rather be explicit about casts from enum to int32_t as it's not a direct
mapping on all machines and not use an implicit c-style cast
(https://google.github.io/styleguide/cppguide.html#Casting). Although in the
case of enum, which is implicitly cast-able to all integral types especially for
a well-defined set of values, it's probably fine.

Let me now if it bothers you because the implicit cast should be safe.

+  base::LinearHistogram::FactoryGet(
+      base::StringPrintf("Previews.InfoBarAction.%s",
+                         GetStringNameForType(previews_type).c_str()),
+      1, max_limit, max_limit + 1,
+      base::HistogramBase::kUmaTargetedHistogramFlag)
+      ->Add(static_cast<int>(action));

[megjablon, 2017/05/11 23:49:45]

Does this need to be cast?

[RyanSturm, 2017/05/15 16:23:08]

Same as above comment. I am changing it to int32_t, since I copied it from
elsewhere that was wrong.

+}
 
-// Key of the UMA Previews.InfoBarAction.Offline histogram.
-const char kUMAPreviewsInfoBarActionOffline[] =
-    "Previews.InfoBarAction.Offline";
+// Sends opt out information to the pingback service based on a key value in the
+// infobar tab helper. Sending this information in the case of a navigation that
+// should not send a pingback (or is not a server preview) will not alter the
+// pingback.
+void ReportPingbackInformation(content::WebContents* web_contents) {
+  auto* data_reduction_proxy_settings =
+      DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
+          web_contents->GetBrowserContext());
+  PreviewsInfoBarTabHelper* infobar_tab_helper =
+      PreviewsInfoBarTabHelper::FromWebContents(web_contents);
+  if (infobar_tab_helper &&
+      infobar_tab_helper->committed_data_saver_navigation_id()) {
+    data_reduction_proxy_settings->data_reduction_proxy_service()
+        ->pingback_client()
+        ->AddOptOut(
+            infobar_tab_helper->committed_data_saver_navigation_id().value());
+  }
+}
 
-// Key of the UMA Previews.InfoBarAction.LitePage histogram.
-const char kUMAPreviewsInfoBarActionLitePage[] =
-    "Previews.InfoBarAction.LitePage";
+// Increments the prefs-based opt out information for data reduction proxy when
+// the user is not already transitioned to the PreviewsBlackList.
+void IncrementDataReductionProxyPrefs(content::WebContents* web_contents) {
+  if (data_reduction_proxy::params::IsBlackListEnabledForServerPreviews())
+    return;
+  auto* data_reduction_proxy_settings =
+      DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
+          web_contents->GetBrowserContext());
+  data_reduction_proxy_settings->IncrementLoFiUserRequestsForImages();
+}
 
-void RecordPreviewsInfoBarAction(
-    PreviewsInfoBarDelegate::PreviewsInfoBarType infobar_type,
-    PreviewsInfoBarDelegate::PreviewsInfoBarAction action) {
-  if (infobar_type == PreviewsInfoBarDelegate::LOFI) {
-    UMA_HISTOGRAM_ENUMERATION(kUMAPreviewsInfoBarActionLoFi, action,
-                              PreviewsInfoBarDelegate::INFOBAR_INDEX_BOUNDARY);
-  } else if (infobar_type == PreviewsInfoBarDelegate::LITE_PAGE) {
-    UMA_HISTOGRAM_ENUMERATION(kUMAPreviewsInfoBarActionLitePage, action,
-                              PreviewsInfoBarDelegate::INFOBAR_INDEX_BOUNDARY);
-  } else if (infobar_type == PreviewsInfoBarDelegate::OFFLINE) {
-    UMA_HISTOGRAM_ENUMERATION(kUMAPreviewsInfoBarActionOffline, action,
-                              PreviewsInfoBarDelegate::INFOBAR_INDEX_BOUNDARY);
+// Reloads the content of the page without previews.
+void ReloadWithoutPreviews(previews::PreviewsType previews_type,
+                           content::WebContents* web_contents) {
+  switch (previews_type) {
+    case previews::PreviewsType::LITE_PAGE:
+    case previews::PreviewsType::OFFLINE:
+      // Prevent LoFi and lite page modes from showing after reload.
+      // TODO(ryansturm): rename DISABLE_LOFI_MODE to DISABLE_PREVIEWS.
+      //  crbug.com/707272

[megjablon, 2017/05/11 23:49:45]

nit: extra space

[RyanSturm, 2017/05/15 16:23:08]

Done.

+      web_contents->GetController().Reload(
+          content::ReloadType::DISABLE_LOFI_MODE, true);
+      break;
+    case previews::PreviewsType::LOFI:
+      web_contents->ReloadLoFiImages();
+      break;
+    case previews::PreviewsType::NONE:
+    case previews::PreviewsType::LAST:
+      break;
   }
 }
 
@@ -58,13 +96,13 @@ PreviewsInfoBarDelegate::~PreviewsInfoBarDelegate() {
   if (!on_dismiss_callback_.is_null())
     on_dismiss_callback_.Run(false);
 
-  RecordPreviewsInfoBarAction(infobar_type_, infobar_dismissed_action_);
+  RecordPreviewsInfoBarAction(previews_type_, infobar_dismissed_action_);
 }
 
 // static
 void PreviewsInfoBarDelegate::Create(
     content::WebContents* web_contents,
-    PreviewsInfoBarType infobar_type,
+    previews::PreviewsType previews_type,
     bool is_data_saver_user,
     const OnDismissPreviewsInfobarCallback& on_dismiss_callback) {
   PreviewsInfoBarTabHelper* infobar_tab_helper =
@@ -80,7 +118,7 @@ void PreviewsInfoBarDelegate::Create(
     return;
 
   std::unique_ptr<PreviewsInfoBarDelegate> delegate(new PreviewsInfoBarDelegate(
-      web_contents, infobar_type, is_data_saver_user, on_dismiss_callback));
+      web_contents, previews_type, is_data_saver_user, on_dismiss_callback));
 
 #if defined(OS_ANDROID)
   std::unique_ptr<infobars::InfoBar> infobar_ptr(
@@ -93,24 +131,25 @@ void PreviewsInfoBarDelegate::Create(
   infobars::InfoBar* infobar =
       infobar_service->AddInfoBar(std::move(infobar_ptr));
 
-  if (infobar && (infobar_type == LITE_PAGE || infobar_type == LOFI)) {
+  if (infobar && (previews_type == previews::PreviewsType::LITE_PAGE ||
+                  previews_type == previews::PreviewsType::LOFI)) {
     auto* data_reduction_proxy_settings =
         DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
             web_contents->GetBrowserContext());
     data_reduction_proxy_settings->IncrementLoFiUIShown();
   }
 
-  RecordPreviewsInfoBarAction(infobar_type, INFOBAR_SHOWN);
+  RecordPreviewsInfoBarAction(previews_type, INFOBAR_SHOWN);
   infobar_tab_helper->set_displayed_preview_infobar(true);
 }
 
 PreviewsInfoBarDelegate::PreviewsInfoBarDelegate(
     content::WebContents* web_contents,
-    PreviewsInfoBarType infobar_type,
+    previews::PreviewsType previews_type,
     bool is_data_saver_user,
     const OnDismissPreviewsInfobarCallback& on_dismiss_callback)
     : ConfirmInfoBarDelegate(),
-      infobar_type_(infobar_type),
+      previews_type_(previews_type),
       infobar_dismissed_action_(INFOBAR_DISMISSED_BY_TAB_CLOSURE),
       message_text_(l10n_util::GetStringUTF16(
           is_data_saver_user ? IDS_PREVIEWS_INFOBAR_SAVED_DATA_TITLE
@@ -162,35 +201,16 @@ bool PreviewsInfoBarDelegate::LinkClicked(WindowOpenDisposition disposition) {
 
   content::WebContents* web_contents =
       InfoBarService::WebContentsFromInfoBar(infobar());
-  if (infobar_type_ == LITE_PAGE || infobar_type_ == LOFI) {
-    auto* data_reduction_proxy_settings =
-        DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
-            web_contents->GetBrowserContext());
-    if (!data_reduction_proxy::params::IsBlackListEnabledForServerPreviews())
-      data_reduction_proxy_settings->IncrementLoFiUserRequestsForImages();
-    PreviewsInfoBarTabHelper* infobar_tab_helper =
-        PreviewsInfoBarTabHelper::FromWebContents(web_contents);
-    if (infobar_tab_helper &&
-        infobar_tab_helper->committed_data_saver_navigation_id()) {
-      data_reduction_proxy_settings->data_reduction_proxy_service()
-          ->pingback_client()
-          ->AddOptOut(
-              infobar_tab_helper->committed_data_saver_navigation_id().value());
-    }
-
-    if (infobar_type_ == LITE_PAGE)
-      web_contents->GetController().Reload(
-          content::ReloadType::DISABLE_LOFI_MODE, true);
-    else if (infobar_type_ == LOFI)
-      web_contents->ReloadLoFiImages();
-  } else if (infobar_type_ == OFFLINE) {
-    // Prevent LoFi and lite page modes from showing after reload.
-    // TODO(ryansturm): rename DISABLE_LOFI_MODE to DISABLE_PREVIEWS.
-    //  crbug.com/707272
-    web_contents->GetController().Reload(content::ReloadType::DISABLE_LOFI_MODE,
-                                         true);
+
+  ReportPingbackInformation(web_contents);
+
+  if (previews_type_ == previews::PreviewsType::LITE_PAGE ||
+      previews_type_ == previews::PreviewsType::LOFI) {

[megjablon, 2017/05/11 23:49:45]

Don't you still want:

if (!data_reduction_proxy::params::IsBlackListEnabledForServerPreviews())	

[RyanSturm, 2017/05/15 16:23:08]

Done.

IncrementDataReductionProxyPrefs had an early return for that check, but I moved
it out since I don't have a better name for that helper method that takes into
account that it might or might not change prefs.

+    IncrementDataReductionProxyPrefs(web_contents);
   }
 
+  ReloadWithoutPreviews(previews_type_, web_contents);
+
   return true;
 }