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

ios/web/web_state/ui/crw_wk_web_view_web_controller.mm

Index: ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
diff --git a/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm b/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
index 5cedc12de5b5f2fe4733f156fbe920bd7f2dc233..4b1ec3bb47f77010baaa5ad6cdeedff74ed6dd22 100644
--- a/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
+++ b/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
@@ -89,6 +89,23 @@ NSString* GetRefererFromNavigationAction(WKNavigationAction* action) {
 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
@@ -472,6 +489,35 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
   _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