Adds UMA for slow and fast back/forward WKWebController navigations.

ios/web/web_state/ui/crw_wk_web_view_web_controller.mm

 // Copyright 2014 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.
 
 #import "ios/web/web_state/ui/crw_wk_web_view_web_controller.h"
 
 #import <WebKit/WebKit.h>
 #include <stddef.h>
 
 #include <utility>
@@ skipping 71 matching lines @@
 const CertVerificationErrorsCacheType::size_type kMaxCertErrorsCount = 100;
 
 // Extracts Referer value from WKNavigationAction request header.
 NSString* GetRefererFromNavigationAction(WKNavigationAction* action) {
   return [action.request valueForHTTPHeaderField:@"Referer"];
 }
 
 NSString* const kScriptMessageName = @"crwebinvoke";
 NSString* const kScriptImmediateName = @"crwebinvokeimmediate";
 
+// Key of the UMA Navigation.IOSWKWebViewSlowFastBackForward histogram.
+const char kUMAWKWebViewSlowFastBackForwardNavigationKey[] =
+    "Navigation.IOSWKWebViewSlowFastBackForward";
+
+// Values for the histogram that counts slow/fast back/forward navigations.
+enum BackForwardNavigationType {
+  // Fast back navigation through WKWebView back-forward list.
+  FAST_BACK = 0,
+  // Slow back navigation when back-forward list navigation is not possible.
+  SLOW_BACK,
+  // Fast forward navigation through WKWebView back-forward list.
+  FAST_FORWARD,
+  // Slow forward navigation when back-forward list navigation is not possible.
+  SLOW_FORWARD,
+  BACK_FORWARD_NAVIGATION_TYPE_COUNT
+};
+
 // Utility functions for storing the source of NSErrors received by WKWebViews:
 // - Errors received by |-webView:didFailProvisionalNavigation:withError:| are
 //   recorded using WKWebViewErrorSource::PROVISIONAL_LOAD.  These should be
 //   aborted.
 // - Errors received by |-webView:didFailNavigation:withError:| are recorded
 //   using WKWebViewsource::NAVIGATION.  These errors should not be aborted, as
 //   the WKWebView will automatically retry the load.
 NSString* const kWKWebViewErrorSourceKey = @"ErrorSource";
 typedef enum { NONE = 0, PROVISIONAL_LOAD, NAVIGATION } WKWebViewErrorSource;
 NSError* WKWebViewErrorWithSource(NSError* error, WKWebViewErrorSource source) {
@@ skipping 363 matching lines @@
 
 - (void)close {
   [_certVerificationController shutDown];
   [super close];
 }
 
 - (void)stopLoading {
   _stoppedWKNavigation.reset(_latestWKNavigation);
 }
 
+- (void)goDelta:(int)delta {
+  [super goDelta:delta];
+  if (delta != 0) {

[Eugene But (OOO till 7-30), 2016/01/21 15:03:49]

NIT: Please use early return to avoid indentations.

[stkhapugin, 2016/02/05 15:40:15]

Done.

+    // Report slow/fast navigation metric.
+    web::WKBackForwardListItemHolder* holder =
+        web::WKBackForwardListItemHolder::FromNavigationItem(
+            self.currentSessionEntry.navigationItemImpl);
+    BOOL isFast =
+        (holder->back_forward_list_item() &&
+         [self isBackForwardListItemValid:holder->back_forward_list_item()]);

[stuartmorgan, 2016/01/28 23:25:44]

The fact that the logic for measuring fast back/forward and actually deciding
whether to do fast back-forward is in totally different places makes me nervous.
I'm not sure we'd ever know if this became wrong.

At the very least I think this conditional should be extracted to a helper
rather than duplicated, but there's other logic too (e.g., "is it a post?") so
I'm wondering if we could find a way to actually log it where the decision is
really made. I know there was some investigation of that, but how many options
were considered? For instance, could we propagate a fast/slow value out of the
implementing function, and check its return value? Or piggy back some extra
state somewhere?

[stkhapugin, 2016/02/05 15:40:15]

Keeping the reported metric as is for now. Please if you feel like this is not
correct, explain in detail what metric do you want to see instead. The way I
understand the reasoning behind the introduction of this metric, what is
reported in this patch is sufficient.

I factored out the logic here and in the method. As was discussed here, in
emails, on previous CL and in the bug, I don't see a 100% clean way of
implementing this because I'm not super familiar with this part of the code. I
don't think I can come up with anything significantly better (propagating delta
or navigation type through the series of calls) than current implementation in
reasonable time, so if you find current solution fundamentally bad, let's find
someone else to find a better way. Personally I think that this is good enough
for reporting one UMA metric.

[stuartmorgan, 2016/02/08 17:12:52]

It's not a question of the divisions in the metric, it's whether the reported
values actually match reality.
I don't think it matters that it's one metric; if we're going to report a
metric, we should report it correctly. Having a metric that is likely to give
increasingly inaccurate data over time seems like a bad idea, since arguably
having an incorrect metric is worse than having no metric.

Having the metric and the actual back/forward code in different places, with
partial copies of the decision tree, is a recipe for things getting out of sync.
It's in fact *already* not actually correct in all cases as I noted in the last
comment (POST is not handled here). There's an argument to be made that that's
an edge case that may not matter in the metric, but the fact that it exists is a
demonstration of the structural problem here. I think if the actual back/forward
logic were to change, there's a good chance this piece of code would be
forgotten and we'd end up reporting something completely unrelated to the actual
behavior of the app.

I'll leave the final call to Eugene as to whether he thinks the risk here
warrants more effort.

+
+    BOOL isBack = (delta < 0);
+
+    BackForwardNavigationType navigationType =
+        BackForwardNavigationType::FAST_BACK;
+    if (isBack) {
+      navigationType = isFast ? BackForwardNavigationType::FAST_BACK
+                              : BackForwardNavigationType::SLOW_BACK;
+    } else {
+      navigationType = isFast ? BackForwardNavigationType::FAST_FORWARD
+                              : BackForwardNavigationType::SLOW_FORWARD;
+    }
+
+    UMA_HISTOGRAM_ENUMERATION(
+        kUMAWKWebViewSlowFastBackForwardNavigationKey, navigationType,
+        BackForwardNavigationType::BACK_FORWARD_NAVIGATION_TYPE_COUNT);
+  }
+}
+
 #pragma mark -
 #pragma mark Testing-Only Methods
 
 - (void)injectWebViewContentView:(CRWWebViewContentView*)webViewContentView {
   [super injectWebViewContentView:webViewContentView];
   [self setWebView:static_cast<WKWebView*>(webViewContentView.webView)];
 }
 
 #pragma mark - Protected property implementations
 
@@ skipping 1564 matching lines @@
         runJavaScriptTextInputPanelWithPrompt:prompt
                                   defaultText:defaultText
                                    requestURL:requestURL
                             completionHandler:completionHandler];
   } else if (completionHandler) {
     completionHandler(nil);
   }
 }
 
 @end