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 16 matching lines @@
  public:
   // Delegate that is used to better handle external dependencies.
   // Default implementation is in .cc file, while tests provide an override.
   class Delegate {
    public:
     virtual ~Delegate() {}
     virtual bool GetTabId(content::WebContents* web_contents,
                           std::string* tab_id) const = 0;
   };
 
+  // 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.

[Mark P, 2016/06/29 19:18:27]

Please add something in here that because this is used for UMA reporting, these
values should not be changed or reused; new values should be ended immediately
before the MAX value.

Also, for ease in aligning this with histograms.xml, please do =0, =1, =2, etc.
on each of these (possibly except MAX, up to you).

[Mark P, 2016/06/29 19:18:27]

If "fringe errors are not included" yet "one of these outcomes will happen",
what do you emit for fringe errors?

[Dmitry Titov, 2016/06/29 20:27:42]

Done.
Moved and re-phrased the comment that was near REDIRECT_RESULT_MAX...

[Dmitry Titov, 2016/06/29 20:27:42]

Nothing is emitted for those. These are the cases that theoretically can happen
but won't in practice - for example, we pull the pointer to a KeyedService
(which should always be there) but the code still checks it for NULL and does an
early return. Those are likely never happen or at least they are not interesting
for evaluating redirect usage/effectiveness.

[Mark P, 2016/06/30 05:09:02]

Apologies, somehow I missed that comment.

+  // Public for testing.
+  enum class RedirectResult {

[Mark P, 2016/06/29 19:18:27]

nit: why a class?

[Dmitry Titov, 2016/06/29 20:27:42]

We seem to be getting a suggestion to use enum classes rather than an old enum
almost universally lately... For new code for sure. While coding style does not
mandate usage of one over another, they are more 'advanced' (no implicit
conversions, scoped names). Is there a reason not to use them?
Here is a link I use to look for allowed C++ features:
https://chromium-cpp.appspot.com/

[Mark P, 2016/06/30 05:09:02]

Thanks for the link and explanation.  class sgtm.

+    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,

[Mark P, 2016/06/29 19:18:27]

No PAGE_NOT_FOUND_ON_CONNECTED_NETWORK?

[Dmitry Titov, 2016/06/29 20:27:42]

Right, not applicable to connected network. "Page not found" in this context
always means offline page (a snapshot saved locally) and we don't do lookup for
it in connected case. This type of redirect happens when we were showing the
offline snapshot and then user reloads when network is connected - this redirect
always goes to online page, which might be also 'not found' but it's a different
'not found' that we are not interested in here...

[Mark P, 2016/06/30 05:09:02]

Acknowledged.

+    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,
+  };
+
   ~OfflinePageTabHelper() override;
 
   const OfflinePageItem* offline_page() { return offline_page_.get(); }
 
  private:
-  enum class RedirectReason {
-    DISCONNECTED_NETWORK,
-    FLAKY_NETWORK,
-    FLAKY_NETWORK_FORWARD_BACK
-  };
-
   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 is accumulated along the codepath to reflect the overall
+  // result of redirection - and be reported to UMA at the end.
+  void GetPagesForRedirectToOffline(RedirectResult result,
+                                    const GURL& online_url);
   void SelectBestPageForRedirectToOffline(
+      RedirectResult result,
       const GURL& online_url,
-      RedirectReason reason,
       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 if a given URL is in redirect chain already.
+  bool IsInRedirectLoop(const GURL& to_url) const;

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

nit: add empty line

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

Done.

+  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_