[ios] Adds history user action metrics

ios/chrome/browser/ui/history/history_collection_view_controller.mm

Index: ios/chrome/browser/ui/history/history_collection_view_controller.mm
diff --git a/ios/chrome/browser/ui/history/history_collection_view_controller.mm b/ios/chrome/browser/ui/history/history_collection_view_controller.mm
index c2c010360832627a6b280f0861faedca1db39484..10fe97de52b30a743d763fd51e8462a2019d61e6 100644
--- a/ios/chrome/browser/ui/history/history_collection_view_controller.mm
+++ b/ios/chrome/browser/ui/history/history_collection_view_controller.mm
@@ -12,6 +12,8 @@
 #include "base/mac/foundation_util.h"
 #import "base/mac/objc_property_releaser.h"
 #include "base/mac/scoped_nsobject.h"
+#include "base/metrics/user_metrics.h"
+#include "base/metrics/user_metrics_action.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/browsing_data/core/history_notice_utils.h"
@@ -110,6 +112,8 @@ const CGFloat kSeparatorInset = 10;
 // Displays context menu on cell pressed with gestureRecognizer.
 - (void)displayContextMenuInvokedByGestureRecognizer:
     (UILongPressGestureRecognizer*)gestureRecognizer;
+// Helper method for opening URL's and recording metrics.
+- (void)openURLWithCompletion:(ProceduralBlock)completionHandler;
 // Opens URL in the current tab and dismisses the history view.
 - (void)openURL:(const GURL&)URL;
 // Opens URL in a new non-incognito tab and dismisses the history view.
@@ -145,6 +149,7 @@ const CGFloat kSeparatorInset = 10;
     _browserState = browserState;
     _delegate.reset(delegate);
     _URLLoader.reset(loader);
+    base::RecordAction(base::UserMetricsAction("HistoryPage_EntryLinkClick"));

[lpromero, 2016/12/20 23:37:30]

Feels weird to have it here. Idem than what Jackie said, this should go closer
to where the user action happens.

[sczs, 2016/12/21 04:24:26]

You're right, the "Click" part of the name makes it weird I think. I just wanted
to make sure that a history entry was going to be recorded regardless if there
are more entry points or if some are added in the future. 

But since this is a user action I will move it closer. I don't want to move it
up to main_controller.mm or BVC since there's already too much in there. 

[Jackie Quinn, 2016/12/21 17:18:19]

Also I think this is for when you tap on a history entry, not when you open
history. So I would throw this into
collectionViewController:didSelectItemAtIndexPath, something like 
self.isSearching ? record searchresultclick : record entrylinkclick

[sczs, 2016/12/21 19:41:51]

Ooooh, that makes everything different! Couldn't find a summary on the
actions.xml file :/ Will change both metrics to only record taps and won't
record anything through the contextual Menu.

Will follow up with Jason so we can add extra metrics if there are necessary. 

     [self loadModel];
     // Add initial info section as header.
     [self.collectionViewModel
@@ -739,42 +744,50 @@ const CGFloat kSeparatorInset = 10;
   [self.contextMenuCoordinator start];
 }
 
+- (void)openURLWithCompletion:(ProceduralBlock)completionHandler {

[Jackie Quinn, 2016/12/20 23:15:54]

Shouldn't this be closeHistoryWithCompletion?

[sczs, 2016/12/21 04:24:26]

You're right I think it would be clearer.

+  if ([self.currentQuery length]) {

[lpromero, 2016/12/20 23:37:30]

Is this always cleared when exiting search? Feels weird to check this side
effect to check the state of the controller.

[sczs, 2016/12/21 04:24:26]

Yes it should (from what I've seen) It was the best way I could find to figure
out that a search was being done. I guess I could also create a flag. 

[Jackie Quinn, 2016/12/21 17:18:19]

There's also self.isSearching.

+    base::RecordAction(
+        base::UserMetricsAction("HistoryPage_SearchResultClick"));

[Jackie Quinn, 2016/12/20 23:15:54]

Is this always going to be invoked from a click? I would put the metric closer
to where the selection occurs, e.g. line 472.

[sczs, 2016/12/21 04:24:26]

I was trying record this action everytime an item was selected after a search
was done. Either by tapping on the cell or using the contextual menu.

For that reason I thought it was better to do it here. If we just want to record
taps then 472 is better. Maybe I'm reading too much into the metric meaning.
WDYT? 

[Jackie Quinn, 2016/12/21 17:18:19]

I feel like it may be better to just have a separate metric for context menu
actions. We don't need to stick with the exact same metrics we had upstream -
for example, I could see having a user action for "present context menu" and for
performing each of the actions in the context menu (that could be action or
histogram, I could see either). 

+  }
+  [self.delegate historyCollectionViewController:self
+                       shouldCloseWithCompletion:completionHandler];
+}
+
 - (void)openURL:(const GURL&)URL {
   GURL copiedURL(URL);
-  [self.delegate historyCollectionViewController:self
-                       shouldCloseWithCompletion:^{
-                         [self.URLLoader
-                                       loadURL:copiedURL
-                                      referrer:web::Referrer()
-                                    transition:ui::PAGE_TRANSITION_AUTO_BOOKMARK
-                             rendererInitiated:NO];
-                       }];
+  ProceduralBlock completionHandler = ^{
+    [self.URLLoader loadURL:copiedURL
+                   referrer:web::Referrer()
+                 transition:ui::PAGE_TRANSITION_AUTO_BOOKMARK
+          rendererInitiated:NO];
+  };
+  [self openURLWithCompletion:completionHandler];
 }
 
 - (void)openURLInNewTab:(const GURL&)URL {
   GURL copiedURL(URL);
-  [self.delegate historyCollectionViewController:self
-                       shouldCloseWithCompletion:^{
-                         [self.URLLoader webPageOrderedOpen:copiedURL
-                                                   referrer:web::Referrer()
-                                                 windowName:nil
-                                                inIncognito:NO
-                                               inBackground:NO
-                                                   appendTo:kLastTab];
-                       }];
+  ProceduralBlock completionHandler = ^{
+    [self.URLLoader webPageOrderedOpen:copiedURL
+                              referrer:web::Referrer()
+                            windowName:nil
+                           inIncognito:NO
+                          inBackground:NO
+                              appendTo:kLastTab];
+  };
+  [self openURLWithCompletion:completionHandler];
 }
 
 - (void)openURLInNewIncognitoTab:(const GURL&)URL {
   GURL copiedURL(URL);
-  [self.delegate historyCollectionViewController:self
-                       shouldCloseWithCompletion:^{
-                         [self.URLLoader webPageOrderedOpen:copiedURL
-                                                   referrer:web::Referrer()
-                                                 windowName:nil
-                                                inIncognito:YES
-                                               inBackground:NO
-                                                   appendTo:kLastTab];
-                       }];
+  ProceduralBlock completionHandler = ^{
+    [self.URLLoader webPageOrderedOpen:copiedURL
+                              referrer:web::Referrer()
+                            windowName:nil
+                           inIncognito:YES
+                          inBackground:NO
+                              appendTo:kLastTab];
+  };
+  [self openURLWithCompletion:completionHandler];
 }
 
 - (void)copyURL:(const GURL&)URL {