Record user actions for long-press follow-on actions.

Record user actions for long-press follow-on actions.

This will be helpful for Contextual Search, and probably useful overall.
Now all user actions in the Action Bar that follow a select action should record a user action on Android.
This includes Share, Web Search, etc.  WebContents already records user actions for edits to text.

BUG=606091
Committed: https://crrev.com/9060f288389c9bfc8460a4f9c30ef70165199443
Cr-Commit-Position: refs/heads/master@{#393561}

[Donn Denman, 2016-05-11 00:01:11]

donnd@chromium.org changed reviewers:
+ pedrosimonetti@chromium.org

[Donn Denman, 2016-05-11 00:01:12]

Pedro, PTAL. Thanks!

[pedro (no code reviews), 2016-05-11 00:46:09]

https://codereview.chromium.org/1965013004/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1965013004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2018:
public void selectAll() {
Don't we want to log "select all" as well? So we can compare with the "web
search" numbers?

https://codereview.chromium.org/1965013004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2024:
mWebContents.cut();
Similarly, I'd think we would log all actions. Then it would be easier to reason
how significant a particular action in comparison with the total number of
actions performed.

https://codereview.chromium.org/1965013004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2039:
RecordUserAction.record("MobileShare");
"MobileShare" sounds a little vague. Wouldn't it be better using the convention
SelectionAction<UI-name>? In this case, it would be "SelectionActionShare".

IMO it would be much clearer what this metric is logging.

https://codereview.chromium.org/1965013004/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2058:
RecordUserAction.record("MobileProcessTextAssist");
In other actions, you are using the UI name for it (Web Search), but here you
are using both the internal name (process text) and the UI name (assist).

I think it would be better to stick with one naming convention, and I'd suggest
keeping with the UI name, so I'd call this "SelectionActionAssist".

[Donn Denman, 2016-05-11 17:26:44]

Pedro, PTAL.  Thanks!

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2018:
public void selectAll() {
On 2016/05/11 00:46:08, pedrosimonetti wrote:
> Don't we want to log "select all" as well? So we can compare with the "web
> search" numbers?

I added a comment above that you must have missed, PTAL.

Also added logging here for whether the SelectAll was done on editable text or
not so we can back up our push to move "Search" up.

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2024:
mWebContents.cut();
On 2016/05/11 00:46:08, pedrosimonetti wrote:
> Similarly, I'd think we would log all actions. Then it would be easier to
reason
> how significant a particular action in comparison with the total number of
> actions performed.

See above.

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2039:
RecordUserAction.record("MobileShare");
On 2016/05/11 00:46:08, pedrosimonetti wrote:
> "MobileShare" sounds a little vague. Wouldn't it be better using the
convention
> SelectionAction<UI-name>? In this case, it would be "SelectionActionShare".
> 
> IMO it would be much clearer what this metric is logging.

I think these are mobile-only actions, so they need to start with "Mobile". 
Looking at what's in actions.xml may be helpful with the naming.

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2058:
RecordUserAction.record("MobileProcessTextAssist");
On 2016/05/11 00:46:08, pedrosimonetti wrote:
> In other actions, you are using the UI name for it (Web Search), but here you
> are using both the internal name (process text) and the UI name (assist).
> 
> I think it would be better to stick with one naming convention, and I'd
suggest
> keeping with the UI name, so I'd call this "SelectionActionAssist".

This code can be triggered by more than one button: both Google Now and
Translate go through here.  Since we don't need to disambiguate them, I think
it's simplest to just record a generic action that does not directly reflect the
exact action.

Updated this to read MobileProcessTextIntent.  

What do you think?

[pedro (no code reviews), 2016-05-11 17:43:43]

lgtm

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2018:
public void selectAll() {
On 2016/05/11 17:26:44, Donn Denman wrote:
> On 2016/05/11 00:46:08, pedrosimonetti wrote:
> > Don't we want to log "select all" as well? So we can compare with the "web
> > search" numbers?
> 
> I added a comment above that you must have missed, PTAL.
> 
> Also added logging here for whether the SelectAll was done on editable text or
> not so we can back up our push to move "Search" up.

Sorry, didn't notice the comment.

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2039:
RecordUserAction.record("MobileShare");
On 2016/05/11 17:26:44, Donn Denman wrote:
> On 2016/05/11 00:46:08, pedrosimonetti wrote:
> > "MobileShare" sounds a little vague. Wouldn't it be better using the
> convention
> > SelectionAction<UI-name>? In this case, it would be "SelectionActionShare".
> > 
> > IMO it would be much clearer what this metric is logging.
> 
> I think these are mobile-only actions, so they need to start with "Mobile". 
> Looking at what's in actions.xml may be helpful with the naming.

My point was that "mobile share" is a little vague, given there are other ways
to "share" on a "mobile" device, other than using the selection's action bar.
You can share a whole page by clicking Preferences > Share, for example.

I'm okay with keeping this name, but I thought a more explicit name would be
better, like MobileSelectionShare, for example.

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2058:
RecordUserAction.record("MobileProcessTextAssist");
On 2016/05/11 17:26:44, Donn Denman wrote:
> On 2016/05/11 00:46:08, pedrosimonetti wrote:
> > In other actions, you are using the UI name for it (Web Search), but here
you
> > are using both the internal name (process text) and the UI name (assist).
> > 
> > I think it would be better to stick with one naming convention, and I'd
> suggest
> > keeping with the UI name, so I'd call this "SelectionActionAssist".
> 
> This code can be triggered by more than one button: both Google Now and
> Translate go through here.  Since we don't need to disambiguate them, I think
> it's simplest to just record a generic action that does not directly reflect
the
> exact action.
> 
> Updated this to read MobileProcessTextIntent.  
> 
> What do you think? 

Sounds good. Thanks for clarifying.

[Donn Denman, 2016-05-11 18:21:07]

Thanks Pedro!

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/1965013004/diff/1/content/public/andro...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2039:
RecordUserAction.record("MobileShare");
On 2016/05/11 17:43:43, pedrosimonetti wrote:
> On 2016/05/11 17:26:44, Donn Denman wrote:
> > On 2016/05/11 00:46:08, pedrosimonetti wrote:
> > > "MobileShare" sounds a little vague. Wouldn't it be better using the
> > convention
> > > SelectionAction<UI-name>? In this case, it would be
"SelectionActionShare".
> > > 
> > > IMO it would be much clearer what this metric is logging.
> > 
> > I think these are mobile-only actions, so they need to start with "Mobile". 
> > Looking at what's in actions.xml may be helpful with the naming.
> 
> My point was that "mobile share" is a little vague, given there are other ways
> to "share" on a "mobile" device, other than using the selection's action bar.
> You can share a whole page by clicking Preferences > Share, for example.
> 
> I'm okay with keeping this name, but I thought a more explicit name would be
> better, like MobileSelectionShare, for example.

That seems like a reasonable suggestion, but I'm thinking it might be best to
keep things simple since the Web Contents defined actions are super-short (copy,
paste, etc).  OTOH the other share that you mentioned I think it triggered by
Settings->Share and is recorded as MobileMenuShare so MobileActionBarShare or
MobileSelectionShare would be consistent with that.  

Let's leave it short for now and see what the other reviewers say.

[Donn Denman, 2016-05-11 18:23:22]

donnd@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Donn Denman, 2016-05-11 18:23:23]

Robert, PTAL.  Thanks!

[Donn Denman, 2016-05-11 18:27:23]

donnd@chromium.org changed reviewers:
- rkaplow@chromium.org

[Donn Denman, 2016-05-11 18:27:23]

Robert, sorry -- I left some junk in the most recent patch, will add you back as
a reviewer when ready.

[Donn Denman, 2016-05-11 18:37:08]

donnd@chromium.org changed reviewers:
+ mohsen@chromium.org

[Donn Denman, 2016-05-11 18:37:08]

mohsen@, PTAL at touch_selection_controller.cc.  Thanks!

[Donn Denman, 2016-05-11 23:36:53]

donnd@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Donn Denman, 2016-05-11 23:36:54]

Ted, PTAL at ContentViewCore.java.

Pedro, PTAL at ContextualSearchManager since I made a minor change there.

Thanks!

[Ted C, 2016-05-11 23:56:15]

lgtm w/ some sort of prefix added

https://chromiumcodereview.appspot.com/1965013004/diff/50001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/1965013004/diff/50001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2023:
RecordUserAction.record("MobileSelectAllWasEditable");
For all of these, it might be nice to have a general prefix.

Something like MobileActionMode.SelectAllWasEditable

MobileActionMode.Share
...

In particular, share is really generic as well as web search, so it would be
good to call out this as being triggered by the action mode explicitly.

[pedro (no code reviews), 2016-05-12 00:03:44]

lgtm

[Donn Denman, 2016-05-12 16:36:15]

Thanks Ted and Pedro!

Mohsen, if you're busy I'm happy to pass the review of
touch_selection_controller.cc to someone else.

https://chromiumcodereview.appspot.com/1965013004/diff/50001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/1965013004/diff/50001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2023:
RecordUserAction.record("MobileSelectAllWasEditable");
On 2016/05/11 23:56:15, Ted C wrote:
> For all of these, it might be nice to have a general prefix.
> 
> Something like MobileActionMode.SelectAllWasEditable
> 
> MobileActionMode.Share
> ...
> 
> In particular, share is really generic as well as web search, so it would be
> good to call out this as being triggered by the action mode explicitly.

Done: prefixed all of these with "MobileActionMode.", which allowed shortening
most action names beyond the dot.

[mohsen, 2016-05-12 17:27:44]

LGTM

https://chromiumcodereview.appspot.com/1965013004/diff/70001/ui/touch_selecti...
File ui/touch_selection/touch_selection_controller.cc (right):

https://chromiumcodereview.appspot.com/1965013004/diff/70001/ui/touch_selecti...
ui/touch_selection/touch_selection_controller.cc:390:
base::RecordAction(base::UserMetricsAction("MobileSelectionChanged"));
nit: By "Mobile" do you mean Android? Can we use a more general name that is
also meaningful on desktop?

nit2: Curly braces are not necessary here.

[Donn Denman, 2016-05-12 17:37:46]

Thanks Mohsen!

https://chromiumcodereview.appspot.com/1965013004/diff/70001/ui/touch_selecti...
File ui/touch_selection/touch_selection_controller.cc (right):

https://chromiumcodereview.appspot.com/1965013004/diff/70001/ui/touch_selecti...
ui/touch_selection/touch_selection_controller.cc:390:
base::RecordAction(base::UserMetricsAction("MobileSelectionChanged"));
On 2016/05/12 17:27:44, mohsen wrote:
> nit: By "Mobile" do you mean Android? Can we use a more general name that is
> also meaningful on desktop?
Good point -- changed to SelectionChanged.

> 
> nit2: Curly braces are not necessary here.

Done.

[Donn Denman, 2016-05-12 17:38:31]

donnd@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Donn Denman, 2016-05-12 17:38:31]

Robert, This is ready for a review of actions.xml, PTAL.

[rkaplow, 2016-05-12 22:01:52]

https://codereview.chromium.org/1965013004/diff/90001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1965013004/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:440:
RecordUserAction.record(isSingleWord ? "ContextualSearch.ManualRefineSingleWord"
would this make more sense as a histogram? Do you care about the timing
information (i.e. event X occured before event Y?)

[Donn Denman, 2016-05-12 22:12:59]

Thanks for taking a look Robert!

https://codereview.chromium.org/1965013004/diff/90001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1965013004/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:440:
RecordUserAction.record(isSingleWord ? "ContextualSearch.ManualRefineSingleWord"
On 2016/05/12 22:01:51, rkaplow wrote:
> would this make more sense as a histogram? Do you care about the timing
> information (i.e. event X occured before event Y?)

Yes, we do want the timing information because we're most interested in whether
we get single/multi-word proceeding a WebSearch action.

[rkaplow, 2016-05-13 14:09:12]

lgtm

[Donn Denman, 2016-05-13 16:26:18]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2016-05-13 16:26:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, pedrosimonetti@chromium.org, mohsen@chromium.org
Link to the patchset: https://codereview.chromium.org/1965013004/#ps90001
(title: "Changed MobileSelectionChanged to SelectionChanged.")

[commit-bot: I haz the power, 2016-05-13 16:26:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1965013004/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1965013004/90001

[commit-bot: I haz the power, 2016-05-13 17:17:48]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-05-13 17:18:50]

Description was changed from

==========
Record user actions for long-press follow-on actions.

This will be helpful for Contextual Search, and probably useful overall.
Now all user actions in the Action Bar that follow a select action should record
a user action on Android.
This includes Share, Web Search, etc.  WebContents already records user actions
for edits to text.

BUG=606091
==========

to

==========
Record user actions for long-press follow-on actions.

This will be helpful for Contextual Search, and probably useful overall.
Now all user actions in the Action Bar that follow a select action should record
a user action on Android.
This includes Share, Web Search, etc.  WebContents already records user actions
for edits to text.

BUG=606091

Committed: https://crrev.com/9060f288389c9bfc8460a4f9c30ef70165199443
Cr-Commit-Position: refs/heads/master@{#393561}
==========

[commit-bot: I haz the power, 2016-05-13 17:18:52]

Patchset 6 (id:??) landed as
https://crrev.com/9060f288389c9bfc8460a4f9c30ef70165199443
Cr-Commit-Position: refs/heads/master@{#393561}