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

chrome/browser/android/offline_pages/offline_page_tab_helper.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/android/offline_pages/offline_page_tab_helper.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram.h"
@@ skipping 68 matching lines @@
   weak_ptr_factory_.InvalidateWeakPtrs();
 
   // Since this is a new navigation, we will reset the cached offline page,
   // unless we are currently looking at an offline page.
   GURL navigated_url = navigation_handle->GetURL();
   if (offline_page_ && navigated_url != offline_page_->GetOfflineURL())
     offline_page_ = nullptr;
 
   // 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. Bug 624216.
   const content::NavigationController& controller =
       web_contents()->GetController();
   if (controller.GetEntryCount() > 0 &&
       controller.GetCurrentEntryIndex() != -1 &&
       controller.GetCurrentEntryIndex() < controller.GetEntryCount() - 1) {
     return;
   }
 
   content::BrowserContext* context = web_contents()->GetBrowserContext();
   if (net::NetworkChangeNotifier::IsOffline()) {
-    GetPagesForRedirectToOffline(navigated_url,
-                                 RedirectReason::DISCONNECTED_NETWORK);
+    GetPagesForRedirectToOffline(
+        RedirectResult::REDIRECTED_ON_DISCONNECTED_NETWORK, navigated_url);
     return;
   }
 
   OfflinePageModel* offline_page_model =
       OfflinePageModelFactory::GetForBrowserContext(context);
   if (!offline_page_model)
     return;
 
   offline_page_model->GetPageByOfflineURL(
       navigated_url, base::Bind(&OfflinePageTabHelper::RedirectToOnline,
@@ skipping 23 matching lines @@
   // When the navigation starts, we redirect immediately from online page to
   // offline version on the case that there is no network connection. If there
   // is still network connection but with no or poor network connectivity, the
   // navigation will eventually fail and we want to redirect to offline copy
   // in this case. If error code doesn't match this list, then we still show
   // the error page and not an offline page, so do nothing.
   if (error_code != net::ERR_INTERNET_DISCONNECTED &&
       error_code != net::ERR_NAME_NOT_RESOLVED &&
       error_code != net::ERR_ADDRESS_UNREACHABLE &&
       error_code != net::ERR_PROXY_CONNECTION_FAILED) {
+    ReportRedirectResultUMA(RedirectResult::SHOW_NET_ERROR_PAGE);
     return;
   }
 
-  // Otherwise, get the offline URL for this url, and attempt a redirect if
-  // necessary.
-  RedirectReason reason =
-      ui::PageTransitionTypeIncludingQualifiersIs(
+  // Don't actually want to redirect on a forward/back nav.
+  // TODO(dimich): Clarify and possibly redirect as well. Bug 624216.
+  if (ui::PageTransitionTypeIncludingQualifiersIs(
           navigation_handle->GetPageTransition(),
-          ui::PAGE_TRANSITION_FORWARD_BACK)
-          ? RedirectReason::FLAKY_NETWORK_FORWARD_BACK
-          : RedirectReason::FLAKY_NETWORK;
-  GetPagesForRedirectToOffline(navigated_url, reason);
+          ui::PAGE_TRANSITION_FORWARD_BACK)) {
+    ReportRedirectResultUMA(RedirectResult::IGNORED_FLAKY_NETWORK_FORWARD_BACK);
+    return;
+  }
+
+  GetPagesForRedirectToOffline(
+      RedirectResult::REDIRECTED_ON_FLAKY_NETWORK, navigated_url);
 }
 
 void OfflinePageTabHelper::RedirectToOnline(
     const GURL& navigated_url,
     const OfflinePageItem* offline_page) {
   // Bails out if no redirection is needed.
   if (!offline_page)

[jianli, 2016/06/29 20:20:16]

It seems that we can return PAGE_NOT_FOUND_ON_CONNECTED_NETWORK here.

[Dmitry Titov, 2016/06/29 20:38:03]

This case includes all regular navigations (when online). This case is not
interesting for the feature and will dwarf the useful results in histogram
because it is the much more frequent case.
Added comment.

     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)) {
+    ReportRedirectResultUMA(RedirectResult::REDIRECT_LOOP_ONLINE);
     return;
   }
 
   Redirect(navigated_url, redirect_url);
   // Clear the offline page since we are redirecting to online.
   offline_page_ = nullptr;
 
-  UMA_HISTOGRAM_COUNTS("OfflinePages.RedirectToOnlineCount", 1);
+  ReportRedirectResultUMA(RedirectResult::REDIRECTED_ON_CONNECTED_NETWORK);
 }
 
-void OfflinePageTabHelper::GetPagesForRedirectToOffline(const GURL& online_url,
-                                                        RedirectReason reason) {
+void OfflinePageTabHelper::GetPagesForRedirectToOffline(
+    RedirectResult result, const GURL& online_url) {

[jianli, 2016/06/29 20:20:16]

Passing RedirectResult at this time seems to be confusing since we indeed have
not concluded with the final result yet. Can we just pass a boolean flag to
indicate if the network is disconnected or flaky?

[Dmitry Titov, 2016/06/29 20:38:03]

I have a commentin .h file that explains that the result is accumulated along
the codepath. passing booleans and other enums to combine into some final result
at the end is possible but produces much more complex code. I considered this as
well.

   OfflinePageModel* offline_page_model =
       OfflinePageModelFactory::GetForBrowserContext(
           web_contents()->GetBrowserContext());
   if (!offline_page_model)
     return;
 
   offline_page_model->GetPagesByOnlineURL(
       online_url,
       base::Bind(&OfflinePageTabHelper::SelectBestPageForRedirectToOffline,
-                 weak_ptr_factory_.GetWeakPtr(), online_url, reason));
+                 weak_ptr_factory_.GetWeakPtr(), result, online_url));
 }
 
 void OfflinePageTabHelper::SelectBestPageForRedirectToOffline(
+    RedirectResult result,
     const GURL& online_url,
-    RedirectReason reason,
     const MultipleOfflinePageItemResult& pages) {
+  DCHECK(result == RedirectResult::REDIRECTED_ON_FLAKY_NETWORK ||
+         result == RedirectResult::REDIRECTED_ON_DISCONNECTED_NETWORK);
+
   // 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)) {
+    ReportRedirectResultUMA(RedirectResult::NO_TAB_ID);
     return;
+  }
 
   const OfflinePageItem* selected_page = nullptr;
   for (const auto& offline_page : pages) {
     if ((offline_page.client_id.name_space == kBookmarkNamespace) ||
         (offline_page.client_id.name_space == kLastNNamespace &&
          offline_page.client_id.id == tab_id)) {
       if (!selected_page ||
           offline_page.creation_time > selected_page->creation_time) {
         selected_page = &offline_page;
       }
     }
   }
 
-  if (!selected_page)
+  if (!selected_page) {
+    ReportRedirectResultUMA(
+        result == RedirectResult::REDIRECTED_ON_FLAKY_NETWORK ?
+        RedirectResult::PAGE_NOT_FOUND_ON_FLAKY_NETWORK :
+        RedirectResult::PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK);
     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)) {
+    ReportRedirectResultUMA(RedirectResult::REDIRECT_LOOP_OFFLINE);
+    return;
   }
 
   Redirect(from_url, redirect_url);
   offline_page_ = base::MakeUnique<OfflinePageItem>(offline_page);
-  UMA_HISTOGRAM_COUNTS("OfflinePages.RedirectToOfflineCount", 1);
+  ReportRedirectResultUMA(result);
 }
 
 void OfflinePageTabHelper::Redirect(const GURL& from_url, const GURL& to_url) {
   content::NavigationController::LoadURLParams load_params(to_url);
   load_params.transition_type = ui::PAGE_TRANSITION_CLIENT_REDIRECT;
   load_params.redirect_chain.push_back(from_url);
   web_contents()->GetController().LoadURLWithParams(load_params);
 }
 
+bool OfflinePageTabHelper::IsInRedirectLoop(const GURL& to_url) const {
+  // Detects 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::ReportRedirectResultUMA(RedirectResult result) {
+  UMA_HISTOGRAM_ENUMERATION("OfflinePages.RedirectResult",
+      static_cast<int>(result),
+      static_cast<int>(RedirectResult::REDIRECT_RESULT_MAX));
+}
 }  // namespace offline_pages