Hide notifications after their deadline

Hide notifications after their deadline

Right now, this code will hide the notification at the end of the
timeout, even we switched to showing a different article in the
notification in the meantime. Fixing that will mean switching to
different tags for different articles.

Notifications should also implicitly be hidden if the articles
themselves expire, since the notifier service will watch for their
removal.

BUG=675561
Review-Url: https://codereview.chromium.org/2616893003
Cr-Commit-Position: refs/heads/master@{#442010}
Committed: https://chromium.googlesource.com/chromium/src/+/5a568c2ca3041b4b2b1975508c75d7e95df2a131

[sfiera, 2017-01-05 16:28:55]

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

[commit-bot: I haz the power, 2017-01-05 16:29:19]

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

[sfiera, 2017-01-05 16:30:02]

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

[sfiera, 2017-01-05 16:30:02]

Would it be OK to use the URL of the article as the notification tag, or is that
not an appropriate use of the tag? What about url.hash() as the ID?

[commit-bot: I haz the power, 2017-01-05 16:31:10]

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

[commit-bot: I haz the power, 2017-01-05 16:31:11]

Dry run: Try jobs failed on following builders:
  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...)

[Bernhard Bauer, 2017-01-05 17:14:54]

On 2017/01/05 16:30:02, sfiera wrote:
> Would it be OK to use the URL of the article as the notification tag, or is
that
> not an appropriate use of the tag? What about url.hash() as the ID?

Yeah, using tags seems a bit weird, and hashes might collide (in particular the
ones for data structures, which make absolutely no guarantees).

One thing you could try though to uniquify IDs is to call
NotificationManager.getActiveNotifications(), iterate over the IDs and then use
the maximum ID + 1? If the same ID is reused for a second notification (where
the first one has been canceled before the timeout), we will reschedule the
timeout alarm, so there shouldn't be a second unexpected timeout.

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

https://codereview.chromium.org/2616893003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:34:
private static final String TIMEOUT_ACTION =
Do you use this for anything? Your BroadcastReceiver doesn't have an intent
filter, so it will receive any explicit (broadcast to its name) intent.

[sfiera, 2017-01-05 20:01:02]

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

[commit-bot: I haz the power, 2017-01-05 20:01:32]

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-01-05 20:38:05]

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

[commit-bot: I haz the power, 2017-01-05 20:38:06]

Dry run: This issue passed the CQ dry run.

[sfiera, 2017-01-05 22:55:04]

On 2017/01/05 17:14:54, Bernhard Bauer wrote:
> On 2017/01/05 16:30:02, sfiera wrote:
> > Would it be OK to use the URL of the article as the notification tag, or is
> that
> > not an appropriate use of the tag? What about url.hash() as the ID?
> 
> Yeah, using tags seems a bit weird, and hashes might collide (in particular
the
> ones for data structures, which make absolutely no guarantees).
> 
> One thing you could try though to uniquify IDs is to call
> NotificationManager.getActiveNotifications(), iterate over the IDs and then
use
> the maximum ID + 1? If the same ID is reused for a second notification (where
> the first one has been canceled before the timeout), we will reschedule the
> timeout alarm, so there shouldn't be a second unexpected timeout.

I implemented this, but getActiveNotification() is M+ so the bug still exists on
L. I guess we will have to keep our own list of IDs in prefs to do this
properly.

If it's alright, I'd like to go ahead with this as it is, so as to have
something testable by FF.

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

https://codereview.chromium.org/2616893003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:34:
private static final String TIMEOUT_ACTION =
On 2017/01/05 17:14:54, Bernhard Bauer wrote:
> Do you use this for anything? Your BroadcastReceiver doesn't have an intent
> filter, so it will receive any explicit (broadcast to its name) intent.

I don't think so. Is it OK to omit the .setAction() in this case? (I've now done
so)

[Bernhard Bauer, 2017-01-06 13:32:56]

LGTM with lint warnings fixed:

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

https://codereview.chromium.org/2616893003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:34:
private static final String TIMEOUT_ACTION =
On 2017/01/05 22:55:04, sfiera wrote:
> On 2017/01/05 17:14:54, Bernhard Bauer wrote:
> > Do you use this for anything? Your BroadcastReceiver doesn't have an intent
> > filter, so it will receive any explicit (broadcast to its name) intent.
> 
> I don't think so. Is it OK to omit the .setAction() in this case? (I've now
done
> so)

I think it's okay. At the very least, it would be weird to require an action to
be set that is never used.

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

https://codereview.chromium.org/2616893003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:74:
for (StatusBarNotification activeNotification :
manager.getActiveNotifications()) {
This will generate a lint warning that you're using a method that might not be
available. To tell the linter that this is guarded by a check, annotate the
method with @TargetApi(Build.VERSION_CODES.M).

[sfiera, 2017-01-06 17:44:03]

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

[sfiera, 2017-01-06 17:44:20]

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

https://codereview.chromium.org/2616893003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:74:
for (StatusBarNotification activeNotification :
manager.getActiveNotifications()) {
On 2017/01/06 13:32:56, Bernhard Bauer wrote:
> This will generate a lint warning that you're using a method that might not be
> available. To tell the linter that this is guarded by a check, annotate the
> method with @TargetApi(Build.VERSION_CODES.M).

Done.

[commit-bot: I haz the power, 2017-01-06 17:44:23]

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

[sfiera, 2017-01-06 17:57:16]

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

[commit-bot: I haz the power, 2017-01-06 17:57:36]

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-01-06 18:33:30]

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

[commit-bot: I haz the power, 2017-01-06 18:33:31]

Dry run: This issue passed the CQ dry run.

[sfiera, 2017-01-06 19:30:45]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2017-01-06 19:30:46]

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/2616893003/#ps120001
(title: "import annotation")

[commit-bot: I haz the power, 2017-01-06 19:31:28]

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 19:39:49]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483731045674930,
"parent_rev": "80dc1ec706e972f489543a202f9e910e1b8c67b4", "commit_rev":
"5a568c2ca3041b4b2b1975508c75d7e95df2a131"}

[commit-bot: I haz the power, 2017-01-06 19:40:19]

Description was changed from

==========
Hide notifications after their deadline

Right now, this code will hide the notification at the end of the
timeout, even we switched to showing a different article in the
notification in the meantime. Fixing that will mean switching to
different tags for different articles.

Notifications should also implicitly be hidden if the articles
themselves expire, since the notifier service will watch for their
removal.

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

to

==========
Hide notifications after their deadline

Right now, this code will hide the notification at the end of the
timeout, even we switched to showing a different article in the
notification in the meantime. Fixing that will mean switching to
different tags for different articles.

Notifications should also implicitly be hidden if the articles
themselves expire, since the notifier service will watch for their
removal.

BUG=675561

Review-Url: https://codereview.chromium.org/2616893003
Cr-Commit-Position: refs/heads/master@{#442010}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a568c2ca3041b4b2b1975508c75...
==========

[commit-bot: I haz the power, 2017-01-06 19:40:20]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/5a568c2ca3041b4b2b1975508c75...