[ios] Adds history user action metrics

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

 // 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.
 
 #include "ios/chrome/browser/ui/history/history_collection_view_controller.h"
 
 #import <MobileCoreServices/MobileCoreServices.h>
 
 #include <memory>
 
 #import "base/ios/weak_nsobject.h"
 #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"
 #include "components/strings/grit/components_strings.h"
 #include "components/url_formatter/url_formatter.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/chrome_url_constants.h"
 #import "ios/chrome/browser/signin/authentication_service.h"
 #include "ios/chrome/browser/signin/authentication_service_factory.h"
 #import "ios/chrome/browser/ui/collection_view/cells/MDCCollectionViewCell+Chrome.h"
@@ skipping 78 matching lines @@
 // displayed history entries.
 - (void)updateEntriesStatusMessage;
 // Removes selected items from the visible collection, but does not delete them
 // from browser history.
 - (void)removeSelectedItemsFromCollection;
 // Removes all items in the collection that are not included in entries.
 - (void)filterForHistoryEntries:(NSArray*)entries;
 // 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.
 - (void)openURLInNewTab:(const GURL&)URL;
 // Opens URL in a new incognito tab and dismisses the history view.
 - (void)openURLInNewIncognitoTab:(const GURL&)URL;
 // Copies URL to the clipboard.
 - (void)copyURL:(const GURL&)URL;
 @end
 
@@ skipping 15 matching lines @@
                       delegate:(id<HistoryCollectionViewControllerDelegate>)
                                    delegate {
   self = [super initWithStyle:CollectionViewControllerStyleDefault];
   if (self) {
     _propertyReleaser_HistoryCollectionViewController.Init(
         self, [HistoryCollectionViewController class]);
     _historyServiceFacade.reset(new HistoryServiceFacade(browserState, self));
     _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
         addSectionWithIdentifier:kEntriesStatusSectionIdentifier];
     _entryInserter =
         [[HistoryEntryInserter alloc] initWithModel:self.collectionViewModel];
     _entryInserter.delegate = self;
     _entriesType = NO_ENTRIES;
     [self showHistoryMatchingQuery:nil];
   }
@@ skipping 574 matching lines @@
       l10n_util::GetNSStringWithFixup(IDS_IOS_CONTENT_CONTEXT_COPY);
   ProceduralBlock copyURLAction = ^{
     [weakSelf copyURL:entry.URL];
   };
   [self.contextMenuCoordinator addItemWithTitle:copyURLTitle
                                          action:copyURLAction];
   [self.parentViewController.view endEditing:YES];
   [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 {
   DCHECK(URL.is_valid());
   NSData* plainText = [base::SysUTF8ToNSString(URL.spec())
       dataUsingEncoding:NSUTF8StringEncoding];
   NSDictionary* copiedItem = @{
     (NSString*)kUTTypeURL : net::NSURLWithGURL(URL),
     (NSString*)kUTTypeUTF8PlainText : plainText,
   };
   [[UIPasteboard generalPasteboard] setItems:@[ copiedItem ]];
 }
 
 @end