Track active notifications in Java

Track active notifications in Java

Store their notification ID and target URL, along with the
ContentSuggestion::ID of the article.

When dismissing notifications (singly or multiply, manually or
automatically), check if the notification is displayed (for the
automatic case) and generate an action for each notification displayed
(for the multiple case). In this way, we can ensure that one action is
generated for each impression.

Also, check for a existing notification before displaying one and
generating an impression--sometimes we get duplicates, for example at
startup.

On Android M+, we could use the getActiveNotifications() API, and access
this information through the PendingIntent of the StatusBarNotification.
However, to support L, we need to do this ourself.

BUG=675561
Review-Url: https://codereview.chromium.org/2639553002
Cr-Commit-Position: refs/heads/master@{#445097}
Committed: https://chromium.googlesource.com/chromium/src/+/024a0052dc1108dcba765040cc75a7180943994e

[sfiera, 2017-01-17 12:52:09]

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

[sfiera, 2017-01-17 12:52:09]

Honestly, as this CL stands right now, I can say I'm hugely confident in it.
I've played around with it and I don't *think* it generates duplicate actions
anymore. Verifying histograms by hand across restarts is tricky, though.

[Bernhard Bauer, 2017-01-17 15:56:56]

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java
(right):

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:197:
int mId;
Can you make these final?

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:211:
if (!notificationUri.getScheme().equals("chrome")) return null;
We shouldn't really get a malformed URL here, right? Would it be better to just
assert wellformedness?

Also, I think the parse methods below will throw exceptions on malformed input,
so we handle some kinds of malformedness but not others

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:243:
activeNotifications.add(notification.toUri().toString());
You are not allowed to modify the set returned by
SharedPreferences.html.getStringSet():
https://developer.android.com/reference/android/content/SharedPreferences.htm...

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:306:
public static String cachedMetricNameForAction(
Javadoc for public methods please.

[sfiera, 2017-01-17 17:32:08]

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java
(right):

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:197:
int mId;
On 2017/01/17 15:56:55, Bernhard Bauer wrote:
> Can you make these final?

Done.

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:211:
if (!notificationUri.getScheme().equals("chrome")) return null;
On 2017/01/17 15:56:55, Bernhard Bauer wrote:
> We shouldn't really get a malformed URL here, right? Would it be better to
just
> assert wellformedness?

Sure, done.

> Also, I think the parse methods below will throw exceptions on malformed
input,
> so we handle some kinds of malformedness but not others

Uri docs say: "This class is very forgiving--in the face of invalid input, it
will return garbage rather than throw an exception unless otherwise specified."
(none of the exception cases correspond to what we're doing here)

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:243:
activeNotifications.add(notification.toUri().toString());
On 2017/01/17 15:56:55, Bernhard Bauer wrote:
> You are not allowed to modify the set returned by
> SharedPreferences.html.getStringSet():
>
https://developer.android.com/reference/android/content/SharedPreferences.htm...

Done.

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:306:
public static String cachedMetricNameForAction(
On 2017/01/17 15:56:55, Bernhard Bauer wrote:
> Javadoc for public methods please.

Oops, shouldn't have been public.

[Bernhard Bauer, 2017-01-17 18:01:07]

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java
(right):

https://codereview.chromium.org/2639553002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:211:
if (!notificationUri.getScheme().equals("chrome")) return null;
On 2017/01/17 17:32:08, sfiera wrote:
> On 2017/01/17 15:56:55, Bernhard Bauer wrote:
> > We shouldn't really get a malformed URL here, right? Would it be better to
> just
> > assert wellformedness?
> 
> Sure, done.
> 
> > Also, I think the parse methods below will throw exceptions on malformed
> input,
> > so we handle some kinds of malformedness but not others
> 
> Uri docs say: "This class is very forgiving--in the face of invalid input, it
> will return garbage rather than throw an exception unless otherwise
specified."
> (none of the exception cases correspond to what we're doing here)

Integer.parseInt() might choke though, right? Anyway, I prefer assuming that the
values are wellformed anyway, so is fine for me.

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

https://codereview.chromium.org/2639553002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:256:
boolean result = activeNotifications.remove(notification.toUri().toString());
Same here.

It might be worth to extract a helper method that returns a mutable copy of a
preference?

https://codereview.chromium.org/2639553002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:264:
if (activeNotifications == null) return new HashSet<ActiveNotification>();
Can you return Collections.emptySet() here?

[sfiera, 2017-01-18 11:16:40]

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

https://codereview.chromium.org/2639553002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:256:
boolean result = activeNotifications.remove(notification.toUri().toString());
On 2017/01/17 18:01:06, Bernhard Bauer wrote:
> Same here.
> 
> It might be worth to extract a helper method that returns a mutable copy of a
> preference?

Done.

https://codereview.chromium.org/2639553002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:261:
private static Set<ActiveNotification> getActiveNotifications() {
Just realized that Set<> is maybe not valid here. I can obviously make it a
List<>. I could maybe make ActiveNotification hashable but I don't know my Java.

https://codereview.chromium.org/2639553002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:264:
if (activeNotifications == null) return new HashSet<ActiveNotification>();
On 2017/01/17 18:01:06, Bernhard Bauer wrote:
> Can you return Collections.emptySet() here?

Done.

[Bernhard Bauer, 2017-01-18 12:17:04]

LGTM

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

https://codereview.chromium.org/2639553002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:261:
private static Set<ActiveNotification> getActiveNotifications() {
On 2017/01/18 11:16:40, sfiera wrote:
> Just realized that Set<> is maybe not valid here. I can obviously make it a
> List<>. I could maybe make ActiveNotification hashable but I don't know my
Java.

Hm, good point. If you don't need or want deduplication, you could even return a
Collection<ActiveNotification>, which is even more general (or maybe even
Iterable?). Technically, ActiveNotification right now gets both equality and
hashing from Object, which uses reference equality, so a HashSet will work but
won't do any deduplication anyway.

https://codereview.chromium.org/2639553002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:264:
if (activeNotifications == null) return new HashSet<ActiveNotification>();
On 2017/01/18 11:16:40, sfiera wrote:
> On 2017/01/17 18:01:06, Bernhard Bauer wrote:
> > Can you return Collections.emptySet() here?
> 
> Done.

You got it the wrong way around :) Collections.emptySet() returns an immutable
empty set (which enforces that it's not modified, and which can be a shared
singleton because it's immutable, so you don't have to allocate a new object).
When you want a mutable set (like you do below), you have to create a new one
for yourself.

[sfiera, 2017-01-20 11:09:14]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2017-01-20 11:09:14]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2639553002/#ps60001
(title: "Review #3")

[commit-bot: I haz the power, 2017-01-20 11:09:25]

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-20 12:59:11]

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

[commit-bot: I haz the power, 2017-01-20 12:59:12]

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

[sfiera, 2017-01-20 13:06:18]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2017-01-20 13:06:19]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2639553002/#ps80001
(title: "FindBugs")

[commit-bot: I haz the power, 2017-01-20 13:06:31]

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-20 14:55:16]

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

[commit-bot: I haz the power, 2017-01-20 14:55:17]

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

[sfiera, 2017-01-20 17:27:23]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2017-01-20 17:27:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2639553002/#ps100001
(title: "g cl set_commit")

[commit-bot: I haz the power, 2017-01-20 17:27:46]

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-20 18:23:50]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484933243103770,
"parent_rev": "adf0f84df9f461875c97c51de5d1c739d5437c72", "commit_rev":
"024a0052dc1108dcba765040cc75a7180943994e"}

[commit-bot: I haz the power, 2017-01-20 18:24:20]

Description was changed from

==========
Track active notifications in Java

Store their notification ID and target URL, along with the
ContentSuggestion::ID of the article.

When dismissing notifications (singly or multiply, manually or
automatically), check if the notification is displayed (for the
automatic case) and generate an action for each notification displayed
(for the multiple case). In this way, we can ensure that one action is
generated for each impression.

Also, check for a existing notification before displaying one and
generating an impression--sometimes we get duplicates, for example at
startup.

On Android M+, we could use the getActiveNotifications() API, and access
this information through the PendingIntent of the StatusBarNotification.
However, to support L, we need to do this ourself.

BUG=675561
==========

to

==========
Track active notifications in Java

Store their notification ID and target URL, along with the
ContentSuggestion::ID of the article.

When dismissing notifications (singly or multiply, manually or
automatically), check if the notification is displayed (for the
automatic case) and generate an action for each notification displayed
(for the multiple case). In this way, we can ensure that one action is
generated for each impression.

Also, check for a existing notification before displaying one and
generating an impression--sometimes we get duplicates, for example at
startup.

On Android M+, we could use the getActiveNotifications() API, and access
this information through the PendingIntent of the StatusBarNotification.
However, to support L, we need to do this ourself.

BUG=675561

Review-Url: https://codereview.chromium.org/2639553002
Cr-Commit-Position: refs/heads/master@{#445097}
Committed:
https://chromium.googlesource.com/chromium/src/+/024a0052dc1108dcba765040cc75...
==========

[commit-bot: I haz the power, 2017-01-20 18:24:23]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/024a0052dc1108dcba765040cc75...