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.

[fgorski, 2016/06/29 17:01:15]

https://bugs.chromium.org/p/chromium/issues/detail?id=601217

It might be useful, but perhaps it is not.

[Dmitry Titov, 2016/06/29 18:25:43]

Thanks! Added reference to the tracking bug to link them.

   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(
+        navigated_url, RedirectResult::REDIRECTED_ON_DISCONNECTED_NETWORK);
     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);

[fgorski, 2016/06/29 17:01:16]

Cool find.

[Dmitry Titov, 2016/06/29 18:25:44]

Acknowledged.

+    return;
+  }
+
+  GetPagesForRedirectToOffline(
+      navigated_url, RedirectResult::REDIRECTED_ON_FLAKY_NETWORK);
 }
 
 void OfflinePageTabHelper::RedirectToOnline(
     const GURL& navigated_url,
     const OfflinePageItem* offline_page) {
   // Bails out if no redirection is needed.
   if (!offline_page)
     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) {
+                                                        RedirectResult result) {
   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(), online_url, result));
 }
 
 void OfflinePageTabHelper::SelectBestPageForRedirectToOffline(
     const GURL& online_url,
-    RedirectReason reason,
+    RedirectResult result,
     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) {
+    if (result == RedirectResult::REDIRECTED_ON_FLAKY_NETWORK)
+      result = RedirectResult::PAGE_NOT_FOUND_ON_FLAKY_NETWORK;

[fgorski, 2016/06/29 17:01:15]

nit: I am mildly bothered by overwriting input parameter, and I know you are
returning right after that... but on some level...

How about this?
ReportRedirectResultUMA(
    result == RedirectResult::REDIRECTED_ON_FLAKY_NETWORK ?
        RedirectResult::PAGE_NOT_FOUND_ON_FLAKY_NETWORK :
        RedirectResult::PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK)
return;

I know it looks like monstrosity, but you know... still bothers me less :)

[Dmitry Titov, 2016/06/29 18:25:44]

Done.

+    else
+      result = RedirectResult::PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK;
+    ReportRedirectResultUMA(result);  // 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)) {
+    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 {
+  // Avoids looping between online and offline redirections.

[fgorski, 2016/06/29 17:01:15]

Update comment: Detects looping...

[Dmitry Titov, 2016/06/29 18:25:43]

Done.

+  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