[ios] Adds history user action metrics

Adds the following user action metrics:

HistoryPage_EntryLinkClick
HistoryPage_InitClearBrowsingData
HistoryPage_RemoveSelected
HistoryPage_Search
HistoryPage_SearchResultClick


BUG=676121
Review-Url: https://codereview.chromium.org/2592843002
Cr-Commit-Position: refs/heads/master@{#441968}
Committed: https://chromium.googlesource.com/chromium/src/+/50305e58b695fabcf4338c6912dafeaf6b3ad65e

[sczs, 2016-12-20 22:12:02]

Description was changed from

==========
[ios] Adds history user action metrics

BUG=676121
==========

to

==========
Adds the following user action metrics:

HistoryPage_EntryLinkClick
HistoryPage_InitClearBrowsingData
HistoryPage_RemoveSelected
HistoryPage_Search
HistoryPage_SearchResultClick


BUG=676121
==========

[sczs, 2016-12-20 22:12:10]

sczs@chromium.org changed reviewers:
+ jyquinn@chromium.org, lpromero@chromium.org, sczs@chromium.org

[sczs, 2016-12-20 22:12:11]

PTAL

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

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:747: -
(void)openURLWithCompletion:(ProceduralBlock)completionHandler {
Shouldn't this be closeHistoryWithCompletion?

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:750:
base::UserMetricsAction("HistoryPage_SearchResultClick"));
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.

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

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:152:
base::RecordAction(base::UserMetricsAction("HistoryPage_EntryLinkClick"));
Feels weird to have it here. Idem than what Jackie said, this should go closer
to where the user action happens.

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:748: if
([self.currentQuery length]) {
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]

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:152:
base::RecordAction(base::UserMetricsAction("HistoryPage_EntryLinkClick"));
On 2016/12/20 23:37:30, lpromero wrote:
> Feels weird to have it here. Idem than what Jackie said, this should go closer
> to where the user action happens.

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.

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:747: -
(void)openURLWithCompletion:(ProceduralBlock)completionHandler {
On 2016/12/20 23:15:54, Jackie Quinn wrote:
> Shouldn't this be closeHistoryWithCompletion?
You're right I think it would be clearer.

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:748: if
([self.currentQuery length]) {
On 2016/12/20 23:37:30, lpromero wrote:
> Is this always cleared when exiting search? Feels weird to check this side
> effect to check the state of the controller.

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.

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:750:
base::UserMetricsAction("HistoryPage_SearchResultClick"));
On 2016/12/20 23:15:54, Jackie Quinn wrote:
> 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.

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:20]

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:152:
base::RecordAction(base::UserMetricsAction("HistoryPage_EntryLinkClick"));
On 2016/12/21 04:24:26, sczs wrote:
> On 2016/12/20 23:37:30, lpromero wrote:
> > Feels weird to have it here. Idem than what Jackie said, this should go
closer
> > to where the user action happens.
> 
> 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. 

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

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:748: if
([self.currentQuery length]) {
On 2016/12/21 04:24:26, sczs wrote:
> On 2016/12/20 23:37:30, lpromero wrote:
> > Is this always cleared when exiting search? Feels weird to check this side
> > effect to check the state of the controller.
> 
> 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. 

There's also self.isSearching.

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:750:
base::UserMetricsAction("HistoryPage_SearchResultClick"));
On 2016/12/21 04:24:26, sczs wrote:
> On 2016/12/20 23:15:54, Jackie Quinn wrote:
> > 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.
> 
> 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? 

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).

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

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2592843002/diff/1/ios/chrome/browser/ui/histo...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:152:
base::RecordAction(base::UserMetricsAction("HistoryPage_EntryLinkClick"));
On 2016/12/21 17:18:19, Jackie Quinn wrote:
> On 2016/12/21 04:24:26, sczs wrote:
> > On 2016/12/20 23:37:30, lpromero wrote:
> > > Feels weird to have it here. Idem than what Jackie said, this should go
> closer
> > > to where the user action happens.
> > 
> > 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. 
> 
> 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

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.

[Jackie Quinn, 2016-12-22 01:42:10]

Thanks! lgtm

[sczs, 2016-12-22 16:12:45]

The CQ bit was checked by sczs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 16:12:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 16:46:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 16:46:21]

Dry run: This issue passed the CQ dry run.

[sczs, 2016-12-22 16:47:43]

The CQ bit was checked by sczs@chromium.org

[commit-bot: I haz the power, 2016-12-22 16:47:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 16:54:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 16:54:10]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sczs, 2017-01-05 17:08:04]

Louis, does this lgty? :)

[lpromero, 2017-01-06 10:13:55]

The CQ bit was checked by lpromero@chromium.org

[lpromero, 2017-01-06 10:13:56]

lgtm

[commit-bot: I haz the power, 2017-01-06 10:14:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 10:23:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 10:23:46]

Failed to apply patch for
ios/chrome/browser/ui/history/history_panel_view_controller.mm:
While running git apply --index -p1;
  error: patch failed:
ios/chrome/browser/ui/history/history_panel_view_controller.mm:7
  error: ios/chrome/browser/ui/history/history_panel_view_controller.mm: patch
does not apply

Patch:       ios/chrome/browser/ui/history/history_panel_view_controller.mm
Index: ios/chrome/browser/ui/history/history_panel_view_controller.mm
diff --git a/ios/chrome/browser/ui/history/history_panel_view_controller.mm
b/ios/chrome/browser/ui/history/history_panel_view_controller.mm
index
41be2faa97542d813293e05832e1e87c99454cf5..3010ba729d80ff0bc0255b4e611f487a932d14ba
100644
--- a/ios/chrome/browser/ui/history/history_panel_view_controller.mm
+++ b/ios/chrome/browser/ui/history/history_panel_view_controller.mm
@@ -7,6 +7,8 @@
 #include "base/ios/block_types.h"
 #include "base/ios/ios_util.h"
 #include "base/mac/scoped_nsobject.h"
+#include "base/metrics/user_metrics.h"
+#include "base/metrics/user_metrics_action.h"
 #include "components/strings/grit/components_strings.h"
 #import "ios/chrome/browser/ui/history/clear_browsing_bar.h"
 #import "ios/chrome/browser/ui/history/history_collection_view_controller.h"
@@ -299,6 +301,8 @@ CGFloat kShadowOpacity = 0.2f;
 
 - (void)openPrivacySettings {
   [self exitSearchMode];
+  base::RecordAction(
+      base::UserMetricsAction("HistoryPage_InitClearBrowsingData"));
   ShowClearBrowsingData();
 }
 
@@ -324,6 +328,7 @@ CGFloat kShadowOpacity = 0.2f;
 
 - (void)deleteSelectedItems {
   [_historyCollectionController deleteSelectedItemsFromHistory];
+  base::RecordAction(base::UserMetricsAction("HistoryPage_RemoveSelected"));
   [self exitEditingMode];
 }
 - (void)enterSearchMode {
@@ -337,6 +342,7 @@ CGFloat kShadowOpacity = 0.2f;
   [self.view addSubview:searchBarView];
   _historyCollectionController.get().searching = YES;
   [_searchViewController didMoveToParentViewController:self];
+  base::RecordAction(base::UserMetricsAction("HistoryPage_Search"));
 
   // Constraints to make search bar cover header.
   [searchBarView setTranslatesAutoresizingMaskIntoConstraints:NO];

[sczs, 2017-01-06 17:35:15]

The CQ bit was checked by sczs@chromium.org

[sczs, 2017-01-06 17:35:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyquinn@chromium.org, lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2592843002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-06 17:35:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 17:46:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483724115904460,
"parent_rev": "e1e1e253bf95fad3c1311da89a2927327fb6b17a", "commit_rev":
"50305e58b695fabcf4338c6912dafeaf6b3ad65e"}

[commit-bot: I haz the power, 2017-01-06 17:47:04]

Description was changed from

==========
Adds the following user action metrics:

HistoryPage_EntryLinkClick
HistoryPage_InitClearBrowsingData
HistoryPage_RemoveSelected
HistoryPage_Search
HistoryPage_SearchResultClick


BUG=676121
==========

to

==========
Adds the following user action metrics:

HistoryPage_EntryLinkClick
HistoryPage_InitClearBrowsingData
HistoryPage_RemoveSelected
HistoryPage_Search
HistoryPage_SearchResultClick


BUG=676121

Review-Url: https://codereview.chromium.org/2592843002
Cr-Commit-Position: refs/heads/master@{#441968}
Committed:
https://chromium.googlesource.com/chromium/src/+/50305e58b695fabcf4338c6912da...
==========

[commit-bot: I haz the power, 2017-01-06 17:47:05]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/50305e58b695fabcf4338c6912da...