Add enum-based UMA reporting to automatic redirect logic of Offline Pages.

chrome/browser/android/offline_pages/offline_page_tab_helper.cc

Index: chrome/browser/android/offline_pages/offline_page_tab_helper.cc
diff --git a/chrome/browser/android/offline_pages/offline_page_tab_helper.cc b/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
index cb542e74330ee5109395327c58824bc56bd25c2d..49dad6a7f100a1c489f37a607d1f7a5c06104d48 100644
--- a/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
+++ b/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
@@ -86,6 +86,7 @@ void OfflinePageTabHelper::DidStartNavigation(
 
   // Ignore navigations that are forward or back transitions in the nav stack
   // which are not at the head of the stack.
+  // TODO(dimich): Not sure this is needed. Clarify and remove.

[dewittj, 2016/06/29 00:16:31]

maybe add a tracking bug reference here?

[Dmitry Titov, 2016/06/29 01:51:31]

Done.

   const content::NavigationController& controller =
       web_contents()->GetController();
   if (controller.GetEntryCount() > 0 &&
@@ -97,7 +98,7 @@ void OfflinePageTabHelper::DidStartNavigation(
   content::BrowserContext* context = web_contents()->GetBrowserContext();
   if (net::NetworkChangeNotifier::IsOffline()) {
     GetPagesForRedirectToOffline(navigated_url,
-                                 RedirectReason::DISCONNECTED_NETWORK);
+                                 RedirectResult::DISCONNECTED_NETWORK);

[dewittj, 2016/06/29 00:16:31]

It feels like there are actually two enums here: one representing the conditions
under which the redirect was generated, and one representing the result of the
redirect.  Then you wouldn't have the error-prone switch statement when
|page_not_found == true| in ReportUMARedirectResult.

[Dmitry Titov, 2016/06/29 01:51:31]

I removed that switch but still want to have a denormalized flat enum. Even
though some outcomes are clearly a combination of things, it provides for much
more understandable and useful UMA histogram at the end. For example, if it was
normalized into 2 histograms (if this is what you mean) the connection between
results would be lost and it would be impossible to tell in which case pages are
not being found etc. Also less Dremel to get useful numbers.

I've changed wording and removed one unused value so how it is clearer now.

[dewittj, 2016/06/29 17:36:06]

I was actually requesting two enums in code: one for program flow control here,
and a separate one for use with the histogram; the point being that we don't
have a logical "result" yet, we have a state machine state which will be used to
modify execution paths further down the line.

     return;
   }
 
@@ -141,18 +142,26 @@ void OfflinePageTabHelper::DidFinishNavigation(
       error_code != net::ERR_NAME_NOT_RESOLVED &&
       error_code != net::ERR_ADDRESS_UNREACHABLE &&
       error_code != net::ERR_PROXY_CONNECTION_FAILED) {
+    ReportUMARedirectResult(RedirectResult::SHOW_NET_ERROR_PAGE);
     return;
   }
-
   // Otherwise, get the offline URL for this url, and attempt a redirect if
   // necessary.
-  RedirectReason reason =
+  RedirectResult result =
       ui::PageTransitionTypeIncludingQualifiersIs(
           navigation_handle->GetPageTransition(),
           ui::PAGE_TRANSITION_FORWARD_BACK)
-          ? RedirectReason::FLAKY_NETWORK_FORWARD_BACK
-          : RedirectReason::FLAKY_NETWORK;
-  GetPagesForRedirectToOffline(navigated_url, reason);
+          ? RedirectResult::FLAKY_NETWORK_FORWARD_BACK
+          : RedirectResult::FLAKY_NETWORK;
+
+  // Don't actually want to redirect on a forward/back nav.
+  // TODO(dimich): why is this? Clarify and possibly redirect as well.

[jianli, 2016/06/29 00:19:30]

This is because doing redirect here does not work for going back/forward.

[Dmitry Titov, 2016/06/29 01:51:30]

I don't understand *why* it doesn't work so I'll leave the TODO for now - it is
mostly a TODO to understand this :)

+  if (result == RedirectResult::FLAKY_NETWORK_FORWARD_BACK) {

[jianli, 2016/06/29 00:19:29]

Can we rewrite line 148-164 to the following:
  if (ui::PageTransitionTypeIncludingQualifiersIs(...)) {
    // Don't actually want to redirect on a forward/back nav since it
    // doesn't work.
    ReportUMARedirectResult(RedirectResult::FLAKY_NETWORK_FORWARD_BACK);
    return;
  }

  GetPagesForRedirectToOffline(navigated_url, RedirectResult::FLAKY_NETWORK);

[Dmitry Titov, 2016/06/29 01:51:31]

Done.

+    ReportUMARedirectResult(RedirectResult::FLAKY_NETWORK_FORWARD_BACK);
+    return;
+  }
+
+  GetPagesForRedirectToOffline(navigated_url, result);
 }
 
 void OfflinePageTabHelper::RedirectToOnline(
@@ -163,26 +172,18 @@ void OfflinePageTabHelper::RedirectToOnline(
     return;
 
   GURL redirect_url = offline_page->url;
-
-  const content::NavigationController& controller =
-      web_contents()->GetController();
-
-  // Avoids looping between online and offline redirections.
-  content::NavigationEntry* entry = controller.GetPendingEntry();
-  if (entry && !entry->GetRedirectChain().empty() &&
-      entry->GetRedirectChain().back() == redirect_url) {
+  if (IsInRedirectLoop(redirect_url))
     return;

[dewittj, 2016/06/29 00:16:31]

Do we want to UMA the redirect loop?

[Dmitry Titov, 2016/06/29 01:51:31]

Done. I think it might be a fringe condition, but with FLAKY_NETWORKS it can be
more...

-  }
 
   Redirect(navigated_url, redirect_url);
   // Clear the offline page since we are redirecting to online.
   offline_page_ = nullptr;
 
-  UMA_HISTOGRAM_COUNTS("OfflinePages.RedirectToOnlineCount", 1);
+  ReportUMARedirectResult(RedirectResult::CONNECTED_NETWORK);
 }
 
 void OfflinePageTabHelper::GetPagesForRedirectToOffline(const GURL& online_url,
-                                                        RedirectReason reason) {
+                                                        RedirectResult result) {
   OfflinePageModel* offline_page_model =
       OfflinePageModelFactory::GetForBrowserContext(
           web_contents()->GetBrowserContext());
@@ -192,18 +193,20 @@ void OfflinePageTabHelper::GetPagesForRedirectToOffline(const GURL& online_url,
   offline_page_model->GetPagesByOnlineURL(
       online_url,
       base::Bind(&OfflinePageTabHelper::SelectBestPageForRedirectToOffline,
-                 weak_ptr_factory_.GetWeakPtr(), online_url, reason));
+                 weak_ptr_factory_.GetWeakPtr(), online_url, result));
 }
 
 void OfflinePageTabHelper::SelectBestPageForRedirectToOffline(
     const GURL& online_url,
-    RedirectReason reason,
+    RedirectResult result,
     const MultipleOfflinePageItemResult& pages) {
   // When there is no valid tab android there is nowhere to show the offline
   // page, so we can leave.
   std::string tab_id;
-  if (!delegate_->GetTabId(web_contents(), &tab_id))
+  if (!delegate_->GetTabId(web_contents(), &tab_id)) {
+    ReportUMARedirectResult(RedirectResult::NO_TAB_ID);
     return;
+  }
 
   const OfflinePageItem* selected_page = nullptr;
   for (const auto& offline_page : pages) {
@@ -217,42 +220,28 @@ void OfflinePageTabHelper::SelectBestPageForRedirectToOffline(
     }
   }
 
-  if (!selected_page)
+  if (!selected_page) {
+    ReportUMARedirectResult(result, true);  // Report offline page not found.
     return;
+  }
 
-  TryRedirectToOffline(reason, online_url, *selected_page);
+  TryRedirectToOffline(result, online_url, *selected_page);
 }
 
 void OfflinePageTabHelper::TryRedirectToOffline(
-    RedirectReason redirect_reason,
+    RedirectResult result,
     const GURL& from_url,
     const OfflinePageItem& offline_page) {
   GURL redirect_url = offline_page.GetOfflineURL();
   if (!redirect_url.is_valid())
     return;
 
-  if (redirect_reason == RedirectReason::FLAKY_NETWORK ||
-      redirect_reason == RedirectReason::FLAKY_NETWORK_FORWARD_BACK) {
-    UMA_HISTOGRAM_BOOLEAN("OfflinePages.ShowOfflinePageOnBadNetwork",
-                          redirect_reason == RedirectReason::FLAKY_NETWORK);
-    // Don't actually want to redirect on a forward/back nav.
-    if (redirect_reason == RedirectReason::FLAKY_NETWORK_FORWARD_BACK)
-      return;
-  } else {
-    const content::NavigationController& controller =
-        web_contents()->GetController();
-
-    // Avoids looping between online and offline redirections.
-    content::NavigationEntry* entry = controller.GetPendingEntry();
-    if (entry && !entry->GetRedirectChain().empty() &&
-        entry->GetRedirectChain().back() == redirect_url) {
-      return;
-    }
-  }
+  if (IsInRedirectLoop(redirect_url))
+    return;
 
   Redirect(from_url, redirect_url);
   offline_page_ = base::MakeUnique<OfflinePageItem>(offline_page);
-  UMA_HISTOGRAM_COUNTS("OfflinePages.RedirectToOfflineCount", 1);
+  ReportUMARedirectResult(result);
 }
 
 void OfflinePageTabHelper::Redirect(const GURL& from_url, const GURL& to_url) {
@@ -262,4 +251,35 @@ void OfflinePageTabHelper::Redirect(const GURL& from_url, const GURL& to_url) {
   web_contents()->GetController().LoadURLWithParams(load_params);
 }
 
+bool OfflinePageTabHelper::IsInRedirectLoop(const GURL& to_url) {
+  // Avoids looping between online and offline redirections.
+  const content::NavigationController& controller =
+      web_contents()->GetController();
+  content::NavigationEntry* entry = controller.GetPendingEntry();
+  return entry &&
+         !entry->GetRedirectChain().empty() &&
+         entry->GetRedirectChain().back() == to_url;
+}
+
+void OfflinePageTabHelper::ReportUMARedirectResult(
+    RedirectResult result, bool page_not_found) {
+  if (page_not_found) {
+    switch (result) {

[dewittj, 2016/06/29 00:16:31]

This logic is a bit hard to read, it feels like the results should be
manipulated in SelectBestPageForRedirectToOffline, rather than here.

[Dmitry Titov, 2016/06/29 01:51:30]

Done.

+      case RedirectResult::DISCONNECTED_NETWORK:
+        result = RedirectResult::DISCONNECTED_NETWORK_NOT_FOUND;
+        break;
+      case RedirectResult::FLAKY_NETWORK:
+        result = RedirectResult::FLAKY_NETWORK_NOT_FOUND;
+        break;
+      case RedirectResult::FLAKY_NETWORK_FORWARD_BACK:
+        result = RedirectResult::FLAKY_NETWORK_FORWARD_BACK_NOT_FOUND;
+        break;
+      default:
+        NOTREACHED();
+    }
+  }
+  UMA_HISTOGRAM_ENUMERATION("OfflinePages.RedirectResult",
+      static_cast<int>(result),
+      static_cast<int>(RedirectResult::REDIRECT_RESULT_MAX));
+}
 }  // namespace offline_pages