Refine tracking of snippet dismissals.

Refine tracking of snippet dismissals.

Track snippets that were read before they were dismissed differently than those
that were dismissed without being read.

BUG=603920
Committed: https://crrev.com/e756dacf157a829d0b6c6839a4bc2e348b2c55ee
Cr-Commit-Position: refs/heads/master@{#389446}

[PEConn, 2016-04-21 15:26:49]

peconn@chromium.org changed reviewers:
+ bauerb@chromium.org, markusheintz@chromium.org, maybelle@chromium.org

[PEConn, 2016-04-21 15:26:50]

PTAL

[PEConn, 2016-04-21 15:50:41]

Description was changed from

==========
Refine tracking of snippet dismissals.

Track snippets that were read before they were dismissed differently than those
that were dismissed without being read.

Additionally, remove four unused functions from HistoryService that are unused
and I believe to be non-functional.

BUG=603920
==========

to

==========
Refine tracking of snippet dismissals.

Track snippets that were read before they were dismissed differently than those
that were dismissed without being read.

BUG=603920
==========

[Bernhard Bauer, 2016-04-21 16:19:27]

https://codereview.chromium.org/1905013002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/1905013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:75:
public static final int SNIPPETS_ACTION_DISMISSED_VISITED = 3;
Try not to change the semantics of existing UMA actions. Just add new ones
instead and remove the old ones if they're not necessary anymore -- that's the
reason why we specify explicit values.

https://codereview.chromium.org/1905013002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1905013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:79:
public void snippedVisited(SnippetArticle snippet, Callback<Boolean> callback) {
This should probably be named getSnippetVisited or something.

https://codereview.chromium.org/1905013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/1905013002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:24: using
base::android::ConvertUTF8ToJavaString;
This doesn't seem to be necessary.

https://codereview.chromium.org/1905013002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:81: void
SnippetVisitedHistoryRequestCallback(
Move this into an anonymous namespace?

[PEConn, 2016-04-21 16:35:40]

https://codereview.chromium.org/1905013002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/1905013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:75:
public static final int SNIPPETS_ACTION_DISMISSED_VISITED = 3;
On 2016/04/21 16:19:27, Bernhard Bauer wrote:
> Try not to change the semantics of existing UMA actions. Just add new ones
> instead and remove the old ones if they're not necessary anymore -- that's the
> reason why we specify explicit values.

Done.

https://codereview.chromium.org/1905013002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1905013002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:79:
public void snippedVisited(SnippetArticle snippet, Callback<Boolean> callback) {
On 2016/04/21 16:19:27, Bernhard Bauer wrote:
> This should probably be named getSnippetVisited or something.

Done.

https://codereview.chromium.org/1905013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/1905013002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:24: using
base::android::ConvertUTF8ToJavaString;
On 2016/04/21 16:19:27, Bernhard Bauer wrote:
> This doesn't seem to be necessary.

Done.

https://codereview.chromium.org/1905013002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:81: void
SnippetVisitedHistoryRequestCallback(
On 2016/04/21 16:19:27, Bernhard Bauer wrote:
> Move this into an anonymous namespace?

Done.

[Bernhard Bauer, 2016-04-22 15:08:57]

LGTM w/ a nit:

https://codereview.chromium.org/1905013002/diff/40001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/1905013002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:28: namespace {
Add empty lines inside of the namespace.

https://codereview.chromium.org/1905013002/diff/40001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:40: }
// namespace

[PEConn, 2016-04-22 15:29:13]

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

[commit-bot: I haz the power, 2016-04-22 15:29:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1905013002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1905013002/60001

[PEConn, 2016-04-22 15:29:45]

peconn@chromium.org changed reviewers:
+ mpearson@chromium.org

[PEConn, 2016-04-22 15:29:46]

mpearson, PTAL at histograms.xml changes.

[commit-bot: I haz the power, 2016-04-22 18:44:05]

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

[commit-bot: I haz the power, 2016-04-22 18:44:06]

Dry run: This issue passed the CQ dry run.

[Mark P, 2016-04-22 20:16:57]

histograms.xml lgtm with one minor comment

https://codereview.chromium.org/1905013002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/1905013002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:73:
public static final int SNIPPETS_ACTION_CLICKED = 2;
Please leave the =3 entry in this list, possibly renaming it as _OBSOLETE and
commenting it as such.  It should not be reused.

Please somewhere around here that all these numbers (excent NUM_ACTIONS) are
recorded to logs and should not be changed.  New numbers can only be added
immediately before NUM_ACTIONS.

[PEConn, 2016-04-25 09:50:00]

https://codereview.chromium.org/1905013002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/1905013002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:73:
public static final int SNIPPETS_ACTION_CLICKED = 2;
On 2016/04/22 20:16:56, Mark P wrote:
> Please leave the =3 entry in this list, possibly renaming it as _OBSOLETE and
> commenting it as such.  It should not be reused.
> 
> Please somewhere around here that all these numbers (excent NUM_ACTIONS) are
> recorded to logs and should not be changed.  New numbers can only be added
> immediately before NUM_ACTIONS.

Done.

[PEConn, 2016-04-25 09:50:08]

The CQ bit was checked by peconn@chromium.org

[PEConn, 2016-04-25 09:50:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1905013002/#ps120001
(title: "Formatting")

[commit-bot: I haz the power, 2016-04-25 09:50:27]

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

[commit-bot: I haz the power, 2016-04-25 10:59:25]

Description was changed from

==========
Refine tracking of snippet dismissals.

Track snippets that were read before they were dismissed differently than those
that were dismissed without being read.

BUG=603920
==========

to

==========
Refine tracking of snippet dismissals.

Track snippets that were read before they were dismissed differently than those
that were dismissed without being read.

BUG=603920
==========

[commit-bot: I haz the power, 2016-04-25 10:59:26]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-25 11:00:57]

Description was changed from

==========
Refine tracking of snippet dismissals.

Track snippets that were read before they were dismissed differently than those
that were dismissed without being read.

BUG=603920
==========

to

==========
Refine tracking of snippet dismissals.

Track snippets that were read before they were dismissed differently than those
that were dismissed without being read.

BUG=603920

Committed: https://crrev.com/e756dacf157a829d0b6c6839a4bc2e348b2c55ee
Cr-Commit-Position: refs/heads/master@{#389446}
==========

[commit-bot: I haz the power, 2016-04-25 11:00:58]

Patchset 7 (id:??) landed as
https://crrev.com/e756dacf157a829d0b6c6839a4bc2e348b2c55ee
Cr-Commit-Position: refs/heads/master@{#389446}