Add UMA for the content suggestions settings

Add UMA for the content suggestions settings

Adds tracking for
- User actions:
  + ContentSuggestions.RemoteSuggestionsPreferenceOn
  + ContentSuggestions.RemoteSuggestionsPreferenceOff
  + ContentSuggestions.NotificationsPreferenceOn
  + ContentSuggestions.NotificationsPreferenceOff
- Histograms:
  + NewTabPage.ContentSuggestions.Preferences.RemoteSuggestions (Boolean)
  + NewTabPage.ContentSuggestions.Notifications.Actions (added one enum value)

BUG=706384
Review-Url: https://codereview.chromium.org/2790183002
Cr-Commit-Position: refs/heads/master@{#463466}
Committed: https://chromium.googlesource.com/chromium/src/+/65d6cd8be033362397ccd6d444cefc8a4be2fd1c

[dgn, 2017-04-04 13:40:15]

dgn@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[dgn, 2017-04-04 13:40:16]

PTAL. I will add xml changes once the general tracking is approved.

[Michael van Ouwerkerk, 2017-04-04 13:57:18]

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

[commit-bot: I haz the power, 2017-04-04 13:57:36]

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

[Michael van Ouwerkerk, 2017-04-04 14:09:48]

looks good so far :-)

https://codereview.chromium.org/2790183002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsUma.java
(right):

https://codereview.chromium.org/2790183002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsUma.java:21:
private static final String REMOTE_SUGGESTIONS_PREF_TOGGLE_HISTOGRAM_NAME = "";
No name?

https://codereview.chromium.org/2790183002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsUma.java:33:
public static void remoteSuggestionsPreferencesUIAction(@PreferenceToggle int
action) {
maybe rename to something like recordPreferencesChange or
recordPreferencesToggle so that:
* Avoid UI acronym and capitalization discussions
* Indicate this records something
* Do not imply this records an action, it's a histogram

[commit-bot: I haz the power, 2017-04-04 14:23:13]

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

[commit-bot: I haz the power, 2017-04-04 14:23:14]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dgn, 2017-04-04 18:56:48]

Description was changed from

==========
Add UMA for the content suggestions settings

BUG=706384
==========

to

==========
Add UMA for the content suggestions settings

Adds tracking for
- User actions:
  + ContentSuggestions.NotificationsAction.Settings
  + ContentSuggestions.RemoteSuggestionsPreferenceOn
  + ContentSuggestions.RemoteSuggestionsPreferenceOff
  + ContentSuggestions.NotificationsPreferenceOn
  + ContentSuggestions.NotificationsPreferenceOff
- Histograms:
  + ContentSuggestions.Preferences.Notifications (Boolean)
  + ContentSuggestions.Preferences.RemoteSuggestions (Boolean)


BUG=706384
==========

[dgn, 2017-04-04 19:02:54]

PTAL. Do you have better suggestions for naming/scoping?

I'd like to avoid having NTP in the name, and not sure if it should be scoped by
notifications, preferences or others. 


Possibly relevant existing histograms:

NewTabPage.ContentSuggestions.Notifications.Impressions
NewTabPage.ContentSuggestions.Notifications.Actions

from 
https://cs.chromium.org/chromium/src/chrome/browser/ntp_snippets/ntp_snippets...

And others from
https://docs.google.com/spreadsheets/d/1I5-p_WCQJxVv-OK3BXyGQ9McjGsUOvr7Er454...

https://codereview.chromium.org/2790183002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsUma.java
(right):

https://codereview.chromium.org/2790183002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsUma.java:21:
private static final String REMOTE_SUGGESTIONS_PREF_TOGGLE_HISTOGRAM_NAME = "";
On 2017/04/04 14:09:47, Michael van Ouwerkerk wrote:
> No name?

Done.

https://codereview.chromium.org/2790183002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsUma.java:33:
public static void remoteSuggestionsPreferencesUIAction(@PreferenceToggle int
action) {
On 2017/04/04 14:09:47, Michael van Ouwerkerk wrote:
> maybe rename to something like recordPreferencesChange or
> recordPreferencesToggle so that:
> * Avoid UI acronym and capitalization discussions
> * Indicate this records something
> * Do not imply this records an action, it's a histogram

Ack

[dgn, 2017-04-04 19:03:02]

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

[commit-bot: I haz the power, 2017-04-04 19:04:27]

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, 2017-04-04 19:53:12]

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

[commit-bot: I haz the power, 2017-04-04 19:53:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgn, 2017-04-05 10:21:49]

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

[commit-bot: I haz the power, 2017-04-05 10:22:01]

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

[Michael van Ouwerkerk, 2017-04-05 10:27:42]

https://codereview.chromium.org/2790183002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java
(right):

https://codereview.chromium.org/2790183002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:53:
public static Intent getIntent(Context context, @LaunchSource int source) {
This creates a new intent using createIntentForSettingsPage so let's rename this
to createIntent for consistency and clarity.

https://codereview.chromium.org/2790183002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:53:
public static Intent getIntent(Context context, @LaunchSource int source) {
Please add JavaDoc for this public method, probably a bit similar as for
createIntentForSettingsPage.

https://codereview.chromium.org/2790183002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:76:
RecordUserAction.record("ContentSuggestions.NotificationsAction.Settings");
Should we also record an action for LAUNCH_SOURCE_SETTINGS?

https://codereview.chromium.org/2790183002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:163:
ContentSuggestionsPreferences.this.updatePreferences(isEnabled);
Do you need to specify "ContentSuggestionsPreferences.this." or can you just
call updatePreferences directly?

https://codereview.chromium.org/2790183002/diff/20001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2790183002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:3351: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
Please list the metric's owners :-)

https://codereview.chromium.org/2790183002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:3352: <description>Please enter the
description of the metric.</description>
Please enter the description of the metric :-)

https://codereview.chromium.org/2790183002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:9834: <action
name="MobileMenuDataSaverOpened">
Wait, what? Is this a free rebase gift?

[commit-bot: I haz the power, 2017-04-05 11:08:57]

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

[commit-bot: I haz the power, 2017-04-05 11:08:58]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[dgn, 2017-04-05 13:54:06]

dgn@chromium.org changed reviewers:
+ sfiera@chromium.org

[dgn, 2017-04-05 13:54:07]

PTAL

https://codereview.chromium.org/2790183002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java
(right):

https://codereview.chromium.org/2790183002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:53:
public static Intent getIntent(Context context, @LaunchSource int source) {
On 2017/04/05 10:27:41, Michael van Ouwerkerk wrote:
> This creates a new intent using createIntentForSettingsPage so let's rename
this
> to createIntent for consistency and clarity.

Done.

https://codereview.chromium.org/2790183002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:53:
public static Intent getIntent(Context context, @LaunchSource int source) {
On 2017/04/05 10:27:42, Michael van Ouwerkerk wrote:
> Please add JavaDoc for this public method, probably a bit similar as for
> createIntentForSettingsPage.

Done.

https://codereview.chromium.org/2790183002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:76:
RecordUserAction.record("ContentSuggestions.NotificationsAction.Settings");
On 2017/04/05 10:27:41, Michael van Ouwerkerk wrote:
> Should we also record an action for LAUNCH_SOURCE_SETTINGS?

In that case I would prefer if it was in the context of the chrome settings in
general. I'll email tedchoc@ about this and add in a separate CL.

https://codereview.chromium.org/2790183002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:163:
ContentSuggestionsPreferences.this.updatePreferences(isEnabled);
On 2017/04/05 10:27:42, Michael van Ouwerkerk wrote:
> Do you need to specify "ContentSuggestionsPreferences.this." or can you just
> call updatePreferences directly?

Done.

https://codereview.chromium.org/2790183002/diff/20001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2790183002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:3351: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
On 2017/04/05 10:27:42, Michael van Ouwerkerk wrote:
> Please list the metric's owners :-)

Removed this one entirely to use an existing histogram

https://codereview.chromium.org/2790183002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:3352: <description>Please enter the
description of the metric.</description>
On 2017/04/05 10:27:42, Michael van Ouwerkerk wrote:
> Please enter the description of the metric :-)

Obsolete.

[sfiera, 2017-04-05 14:02:47]

Drive-by: there's already a metric waiting called
NewTabPage.ContentSuggestions.Notifications.OptOuts[EXPLICIT]. If we have
something different, we should change
NewTabPage.ContentSuggestions.Notifications.OptOuts[IMPLICIT] to match.

https://codereview.chromium.org/2790183002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2790183002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:8686: +<histogram
name="ContentSuggestions.Preferences.Notifications"
All Notifications histograms are currently under NewTabPage.ContentSuggestions.

[dgn, 2017-04-05 14:09:08]

Description was changed from

==========
Add UMA for the content suggestions settings

Adds tracking for
- User actions:
  + ContentSuggestions.NotificationsAction.Settings
  + ContentSuggestions.RemoteSuggestionsPreferenceOn
  + ContentSuggestions.RemoteSuggestionsPreferenceOff
  + ContentSuggestions.NotificationsPreferenceOn
  + ContentSuggestions.NotificationsPreferenceOff
- Histograms:
  + ContentSuggestions.Preferences.Notifications (Boolean)
  + ContentSuggestions.Preferences.RemoteSuggestions (Boolean)


BUG=706384
==========

to

==========
Add UMA for the content suggestions settings

Adds tracking for
- User actions:
  + ContentSuggestions.RemoteSuggestionsPreferenceOn
  + ContentSuggestions.RemoteSuggestionsPreferenceOff
  + ContentSuggestions.NotificationsPreferenceOn
  + ContentSuggestions.NotificationsPreferenceOff
- Histograms:
  + ContentSuggestions.Preferences.Notifications (Boolean)
  + ContentSuggestions.Preferences.RemoteSuggestions (Boolean)
  + NewTabPage.ContentSuggestions.Notifications.Actions (added one enum value)

BUG=706384
==========

[dgn, 2017-04-05 14:25:47]

PTAL.

@sfiera: I removed the new Notification histogram since I also update the one
you mentioned.

https://codereview.chromium.org/2790183002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2790183002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:8686: +<histogram
name="ContentSuggestions.Preferences.Notifications"
On 2017/04/05 14:02:47, sfiera wrote:
> All Notifications histograms are currently under
NewTabPage.ContentSuggestions.

Done... I can see the argument for consistency having priority here, even though
the name is not accurate.

[dgn, 2017-04-05 14:27:57]

Description was changed from

==========
Add UMA for the content suggestions settings

Adds tracking for
- User actions:
  + ContentSuggestions.RemoteSuggestionsPreferenceOn
  + ContentSuggestions.RemoteSuggestionsPreferenceOff
  + ContentSuggestions.NotificationsPreferenceOn
  + ContentSuggestions.NotificationsPreferenceOff
- Histograms:
  + ContentSuggestions.Preferences.Notifications (Boolean)
  + ContentSuggestions.Preferences.RemoteSuggestions (Boolean)
  + NewTabPage.ContentSuggestions.Notifications.Actions (added one enum value)

BUG=706384
==========

to

==========
Add UMA for the content suggestions settings

Adds tracking for
- User actions:
  + ContentSuggestions.RemoteSuggestionsPreferenceOn
  + ContentSuggestions.RemoteSuggestionsPreferenceOff
  + ContentSuggestions.NotificationsPreferenceOn
  + ContentSuggestions.NotificationsPreferenceOff
- Histograms:
  + NewTabPage.ContentSuggestions.Preferences.RemoteSuggestions (Boolean)
  + NewTabPage.ContentSuggestions.Notifications.Actions (added one enum value)

BUG=706384
==========

[dgn, 2017-04-05 14:28:15]

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

[commit-bot: I haz the power, 2017-04-05 14:28:45]

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, 2017-04-05 15:27:23]

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

[commit-bot: I haz the power, 2017-04-05 15:27:23]

Dry run: This issue passed the CQ dry run.

[Michael van Ouwerkerk, 2017-04-05 15:47:31]

lgtm with nits

https://codereview.chromium.org/2790183002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java
(right):

https://codereview.chromium.org/2790183002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:81:
public static void recordNotificationAction(
nit: empty line between methods, and docs for public methods

https://codereview.chromium.org/2790183002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java
(right):

https://codereview.chromium.org/2790183002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:175:
RecordUserAction.record("ContentSuggestions.RemoteSuggestionsPreferenceOn");
Should this be NewTabPage.ContentSuggestions everywhere for consistency? I
personally would prefer not saying NewTabPage as it's not correct from a Chrome
Home perspective.

[dgn, 2017-04-05 16:53:31]

dgn@chromium.org changed reviewers:
+ asvitkine@chromium.org

[dgn, 2017-04-05 16:53:32]

asvitkine@: PTAL at histograms, actions

https://codereview.chromium.org/2790183002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java
(right):

https://codereview.chromium.org/2790183002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:175:
RecordUserAction.record("ContentSuggestions.RemoteSuggestionsPreferenceOn");
On 2017/04/05 15:47:31, Michael van Ouwerkerk wrote:
> Should this be NewTabPage.ContentSuggestions everywhere for consistency? I
> personally would prefer not saying NewTabPage as it's not correct from a
Chrome
> Home perspective.

we don't have NewTabPage. user actions, the related ones are things like
MobileBookmarkManagerEntryOpened, so consistency is less of an issue here.

[dgn, 2017-04-05 16:53:54]

https://codereview.chromium.org/2790183002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java
(right):

https://codereview.chromium.org/2790183002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:81:
public static void recordNotificationAction(
On 2017/04/05 15:47:31, Michael van Ouwerkerk wrote:
> nit: empty line between methods, and docs for public methods

Done.

[dgn, 2017-04-05 16:55:30]

dgn@chromium.org changed reviewers:
+ treib@chromium.org

[dgn, 2017-04-05 16:55:31]

PTAL at //components/ntp_snippets

[Alexei Svitkine (slow), 2017-04-05 17:10:33]

lgtm

[dgn, 2017-04-05 17:13:22]

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

[commit-bot: I haz the power, 2017-04-05 17:13:45]

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, 2017-04-05 18:39:31]

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

[commit-bot: I haz the power, 2017-04-05 18:39:32]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-04-06 15:39:30]

Some nits, otherwise lg

https://codereview.chromium.org/2790183002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2790183002/diff/140001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.cc:56:
IsRemoteSuggestionsServiceEnabled());
So far, all the metrics recording code lives in the separate metrics.h/cc files.
Can we stay with that?

https://codereview.chromium.org/2790183002/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2790183002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:41260: +    checked at startup. Note:
This histogram is not specific to the New Tab
s/checked/recorded/ ?

Changing the pref at runtime should be supported just fine; I guess we don't
record the histogram in that case?

[dgn, 2017-04-06 17:28:12]

PTAL

https://codereview.chromium.org/2790183002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2790183002/diff/140001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.cc:56:
IsRemoteSuggestionsServiceEnabled());
On 2017/04/06 15:39:30, Marc Treib wrote:
> So far, all the metrics recording code lives in the separate metrics.h/cc
files.
> Can we stay with that?

Done.

https://codereview.chromium.org/2790183002/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2790183002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:41260: +    checked at startup. Note:
This histogram is not specific to the New Tab
On 2017/04/06 15:39:30, Marc Treib wrote:
> s/checked/recorded/ ?

Done.

> Changing the pref at runtime should be supported just fine; I guess we don't
> record the histogram in that case?

Exactly, we record UserActions for punctual changes, but histograms for whether
the user has the service enabled or not are recorded when the suggestions
service is initialised.

[dgn, 2017-04-06 17:28:26]

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

[commit-bot: I haz the power, 2017-04-06 17:29:26]

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, 2017-04-06 17:34:16]

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

[commit-bot: I haz the power, 2017-04-06 17:34:18]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgn, 2017-04-06 18:15:15]

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

[commit-bot: I haz the power, 2017-04-06 18:16:07]

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, 2017-04-06 19:34:04]

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

[commit-bot: I haz the power, 2017-04-06 19:34:05]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-04-07 08:44:33]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-04-07 08:44:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2790183002/#ps180001
(title: "rebase")

[dgn, 2017-04-07 08:44:35]

The CQ bit was unchecked by dgn@chromium.org

[Marc Treib, 2017-04-07 09:00:34]

Sorry for the delay! C++ LGTM with one naming suggestion.

https://codereview.chromium.org/2790183002/diff/180001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_metrics.h (right):

https://codereview.chromium.org/2790183002/diff/180001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_metrics.h:61: void
RecordRemoteSuggestionsServiceState(bool enabled);
nit: Should this be RemoteSuggestionsProvider? It's not actually a service.

[dgn, 2017-04-07 10:24:55]

Thanks!

https://codereview.chromium.org/2790183002/diff/180001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_metrics.h (right):

https://codereview.chromium.org/2790183002/diff/180001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_metrics.h:61: void
RecordRemoteSuggestionsServiceState(bool enabled);
On 2017/04/07 09:00:34, Marc Treib wrote:
> nit: Should this be RemoteSuggestionsProvider? It's not actually a service.

Done.

[dgn, 2017-04-07 10:25:03]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-04-07 10:25:03]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
asvitkine@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2790183002/#ps200001
(title: "rename methods to remove service mention")

[commit-bot: I haz the power, 2017-04-07 10:25:20]

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-04-07 10:34:45]

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

[commit-bot: I haz the power, 2017-04-07 10:34:47]

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

[dgn, 2017-04-10 04:41:15]

dgn@chromium.org changed reviewers:
+ bauerb@chromium.org

[dgn, 2017-04-10 04:41:16]

bauerb@chromium.org: c/a/.../preferences

[dgn, 2017-04-10 04:41:44]

On 2017/04/10 04:41:16, dgn wrote:
> mailto:bauerb@chromium.org: c/a/.../preferences

I'm missing LGTMs for a couple files, PTAL

[dgn, 2017-04-10 04:41:56]

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

[commit-bot: I haz the power, 2017-04-10 04:42:02]

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, 2017-04-10 05:48:54]

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

[commit-bot: I haz the power, 2017-04-10 05:48:56]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-04-10 23:16:03]

LGTM

https://codereview.chromium.org/2790183002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java
(right):

https://codereview.chromium.org/2790183002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:57:
* Creates an intent for launching content suggestions settings page.
Nit: "the content suggestions settings page".

https://codereview.chromium.org/2790183002/diff/200001/chrome/browser/ntp_sni...
File chrome/browser/ntp_snippets/ntp_snippets_metrics.h (right):

https://codereview.chromium.org/2790183002/diff/200001/chrome/browser/ntp_sni...
chrome/browser/ntp_snippets/ntp_snippets_metrics.h:16: //
GENERATED_JAVA_PREFIX_TO_STRIP: CONTENT_SUGGESTIONS_
That makes me wonder whether it would make sense to use enum classes here, so
you could remove the prefix already from the C++ enum.

[dgn, 2017-04-10 23:31:29]

Thanks!

https://codereview.chromium.org/2790183002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java
(right):

https://codereview.chromium.org/2790183002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/ContentSuggestionsPreferences.java:57:
* Creates an intent for launching content suggestions settings page.
On 2017/04/10 23:16:03, Bernhard Bauer wrote:
> Nit: "the content suggestions settings page".

Done.

https://codereview.chromium.org/2790183002/diff/200001/chrome/browser/ntp_sni...
File chrome/browser/ntp_snippets/ntp_snippets_metrics.h (right):

https://codereview.chromium.org/2790183002/diff/200001/chrome/browser/ntp_sni...
chrome/browser/ntp_snippets/ntp_snippets_metrics.h:16: //
GENERATED_JAVA_PREFIX_TO_STRIP: CONTENT_SUGGESTIONS_
On 2017/04/10 23:16:03, Bernhard Bauer wrote:
> That makes me wonder whether it would make sense to use enum classes here, so
> you could remove the prefix already from the C++ enum.

Definitely, I was thinking about that. But I'd do it in a separate CL as it
would result in lots of churn on the C++ side. Filed
https://bugs.chromium.org/p/chromium/issues/detail?id=710254

[dgn, 2017-04-10 23:32:36]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-04-10 23:32:36]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org, asvitkine@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2790183002/#ps220001
(title: "address comments and rebase")

[commit-bot: I haz the power, 2017-04-10 23:33: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-04-11 00:36:42]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1491867156428390,
"parent_rev": "f65d65d3fc6c0413d6f940980bd006654fd02bf8", "commit_rev":
"65d6cd8be033362397ccd6d444cefc8a4be2fd1c"}

[commit-bot: I haz the power, 2017-04-11 00:37:42]

Description was changed from

==========
Add UMA for the content suggestions settings

Adds tracking for
- User actions:
  + ContentSuggestions.RemoteSuggestionsPreferenceOn
  + ContentSuggestions.RemoteSuggestionsPreferenceOff
  + ContentSuggestions.NotificationsPreferenceOn
  + ContentSuggestions.NotificationsPreferenceOff
- Histograms:
  + NewTabPage.ContentSuggestions.Preferences.RemoteSuggestions (Boolean)
  + NewTabPage.ContentSuggestions.Notifications.Actions (added one enum value)

BUG=706384
==========

to

==========
Add UMA for the content suggestions settings

Adds tracking for
- User actions:
  + ContentSuggestions.RemoteSuggestionsPreferenceOn
  + ContentSuggestions.RemoteSuggestionsPreferenceOff
  + ContentSuggestions.NotificationsPreferenceOn
  + ContentSuggestions.NotificationsPreferenceOff
- Histograms:
  + NewTabPage.ContentSuggestions.Preferences.RemoteSuggestions (Boolean)
  + NewTabPage.ContentSuggestions.Notifications.Actions (added one enum value)

BUG=706384

Review-Url: https://codereview.chromium.org/2790183002
Cr-Commit-Position: refs/heads/master@{#463466}
Committed:
https://chromium.googlesource.com/chromium/src/+/65d6cd8be033362397ccd6d444ce...
==========

[commit-bot: I haz the power, 2017-04-11 00:37:44]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/65d6cd8be033362397ccd6d444ce...