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

chrome/browser/android/offline_pages/offline_page_tab_helper.h

 // 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.
 
 #ifndef CHROME_BROWSER_ANDROID_OFFLINE_PAGES_OFFLINE_PAGE_TAB_HELPER_H_
 #define CHROME_BROWSER_ANDROID_OFFLINE_PAGES_OFFLINE_PAGE_TAB_HELPER_H_
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "components/offline_pages/offline_page_types.h"
@@ skipping 20 matching lines @@
    public:
     virtual ~Delegate() {}
     virtual bool GetTabId(content::WebContents* web_contents,
                           std::string* tab_id) const = 0;
   };
 
   ~OfflinePageTabHelper() override;
 
   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 class RedirectResult {

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

nit: Could you move this above the desctructor? It is a type definition and I
think should be at the beginning

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

Done.

+    REDIRECTED_ON_DISCONNECTED_NETWORK,
+    PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK,
+    REDIRECTED_ON_FLAKY_NETWORK,
+    PAGE_NOT_FOUND_ON_FLAKY_NETWORK,
+    IGNORED_FLAKY_NETWORK_FORWARD_BACK,
+    REDIRECTED_ON_CONNECTED_NETWORK,
+    NO_TAB_ID,
+    SHOW_NET_ERROR_PAGE,
+    REDIRECT_LOOP_OFFLINE,
+    REDIRECT_LOOP_ONLINE,
+    // 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,
                            NewNavigationCancelsPendingRedirects);
 
   explicit OfflinePageTabHelper(content::WebContents* web_contents);
 
   void SetDelegateForTesting(std::unique_ptr<Delegate> delegate);
 
   // Overridden from content::WebContentsObserver:
   void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
 
   void RedirectToOnline(const GURL& from_url,
                         const OfflinePageItem* offline_page);
 
   // 3 step redirection to the offline page. First getting all the pages, then
   // 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,

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

nit: please reorder parameters for consistency, if you don't mind.

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

Done.

                             const GURL& from_url,
                             const OfflinePageItem& offline_page);
 
   void Redirect(const GURL& from_url, const GURL& to_url);
 
+  // Returns true if a given URL is in redirect chain already.
+  bool IsInRedirectLoop(const GURL& to_url) const;
+  void ReportRedirectResultUMA(RedirectResult result);
+
   // 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.
   std::unique_ptr<OfflinePageItem> offline_page_;
   std::unique_ptr<Delegate> delegate_;
   base::WeakPtrFactory<OfflinePageTabHelper> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(OfflinePageTabHelper);
 };
 
 }  // namespace offline_pages
 
 #endif  // CHROME_BROWSER_ANDROID_OFFLINE_PAGES_OFFLINE_PAGE_TAB_HELPER_H_