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.

[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 &&
       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);
+                                 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;
   }
 
   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) {
+    ReportUMARedirectResult(RedirectResult::SHOW_NET_ERROR_PAGE);
+    return;
+  }
+  // Otherwise, get the offline URL for this url, and attempt a redirect if
+  // necessary.
+  RedirectResult result =
+      ui::PageTransitionTypeIncludingQualifiersIs(
+          navigation_handle->GetPageTransition(),
+          ui::PAGE_TRANSITION_FORWARD_BACK)
+          ? 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;
   }
 
-  // Otherwise, get the offline URL for this url, and attempt a redirect if
-  // necessary.
-  RedirectReason reason =
-      ui::PageTransitionTypeIncludingQualifiersIs(
-          navigation_handle->GetPageTransition(),
-          ui::PAGE_TRANSITION_FORWARD_BACK)
-          ? RedirectReason::FLAKY_NETWORK_FORWARD_BACK
-          : RedirectReason::FLAKY_NETWORK;
-  GetPagesForRedirectToOffline(navigated_url, reason);
+  GetPagesForRedirectToOffline(navigated_url, result);
 }
 
 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))
     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());
   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) {
   // 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) {
     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) {
+    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) {
   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) {
+  // 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