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

chrome/browser/android/offline_pages/offline_page_tab_helper.h

Index: chrome/browser/android/offline_pages/offline_page_tab_helper.h
diff --git a/chrome/browser/android/offline_pages/offline_page_tab_helper.h b/chrome/browser/android/offline_pages/offline_page_tab_helper.h
index af283e22b529c82a9280c3a39737383927bd13c7..1e26c36c7ee3bcfa00f990057f14b7084d1e0b68 100644
--- a/chrome/browser/android/offline_pages/offline_page_tab_helper.h
+++ b/chrome/browser/android/offline_pages/offline_page_tab_helper.h
@@ -38,13 +38,27 @@ class OfflinePageTabHelper :
 
   const OfflinePageItem* offline_page() { return offline_page_.get(); }
 
- private:
-  enum class RedirectReason {
-    DISCONNECTED_NETWORK,
-    FLAKY_NETWORK,
-    FLAKY_NETWORK_FORWARD_BACK
+  // This enum is used for UMA reporting. It contains all possible outcomes of
+  // redirect intent and result. One of these outcomes will happen. The fringe
+  // errors (like no OfflinePageModel etc) are not included.
+  // Public for testing.
+  enum RedirectResult {

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

nit: enum class

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

using enum class?

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

Done.

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

Done. Adds more static_cast<int>() in the test code but it's ok.

+    DISCONNECTED_NETWORK,            // Redirect to offline, success

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

Since now this enum is about redirect result, it is hard to get a sense of
success from DISCONNECTED_NETWORK value. How about renaming it to
SHOW_OFFLINE_ON_NO_NETWORK

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

Done.

+    DISCONNECTED_NETWORK_NOT_FOUND,  // Redirect failed, no snapshot

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

the wording of the NOT_FOUND enum values is a little confusing.  Something like
PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK reads a little more clearly to me.

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

Done.

+    FLAKY_NETWORK,                   // Redirect to offline, success
+    FLAKY_NETWORK_NOT_FOUND,         // Redirect to offline failed, no snapshot
+    FLAKY_NETWORK_FORWARD_BACK,      // Redirect to offline, success
+    FLAKY_NETWORK_FORWARD_BACK_NOT_FOUND, // Redirect failed, no snapshot
+    CONNECTED_NETWORK,               // Redirect to online, always success
+    NO_TAB_ID,                       // Android Tab ID was not found
+    SHOW_NET_ERROR_PAGE,             // Proceed to error page (unknown error)
+    // NOTE: always keep this entry at the end. Add new redirect results only
+    // immediately above this line. Make sure to update the histogram enum
+    // (OfflinePagesRedirectResult in histograms.xml) accordingly.
+    REDIRECT_RESULT_MAX,
   };
 
+ private:
   friend class content::WebContentsUserData<OfflinePageTabHelper>;
   friend class OfflinePageTabHelperTest;
   FRIEND_TEST_ALL_PREFIXES(OfflinePageTabHelperTest,
@@ -67,17 +81,22 @@ class OfflinePageTabHelper :
   // selecting appropriate page to redirect to and finally attempting to
   // redirect to that offline page, and caching metadata of that page locally.
   void GetPagesForRedirectToOffline(const GURL& online_url,
-                                    RedirectReason reason);
+                                    RedirectResult result);
   void SelectBestPageForRedirectToOffline(
       const GURL& online_url,
-      RedirectReason reason,
+      RedirectResult result,
       const MultipleOfflinePageItemResult& pages);
-  void TryRedirectToOffline(RedirectReason redirect_reason,
+  void TryRedirectToOffline(RedirectResult result,
                             const GURL& from_url,
                             const OfflinePageItem& offline_page);
 
   void Redirect(const GURL& from_url, const GURL& to_url);
 
+  // Returns true is a given URL is in redirect chain already.

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

update comment

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

Done.

+  bool IsInRedirectLoop(const GURL& to_url);

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

nit: can we add const modifier

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

Done.

+  void ReportUMARedirectResult(RedirectResult result,

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

nit: ReportRedirectResultUMA or simply ReportRedirectResult

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

Done.

+                               bool page_not_found = false);
+
   // Iff the tab we are associated with is redirected to an offline page,
   // |offline_page_| will be non-null.  This can be used to synchronously ask
   // about the offline state of the current web contents.