Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(243)

Issue 29513555: Issue 5512 - Manage multiple notifications displayed at the same time

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 3 days ago by Manish Jethani
Modified:
31 minutes ago
Reviewers:
kzar1
CC:
Sebastian Noack, Thomas Greiner
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Issue 5512 - Manage multiple notifications displayed at the same time

Patch Set 1 #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+122 lines, -46 lines) Patch
M lib/notificationHelper.js View 2 chunks +121 lines, -45 lines 10 comments Download
M notification.js View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 4
Manish Jethani
1 week, 3 days ago (2017-08-12 16:49:13 UTC) #1
Manish Jethani
Patch Set 1 Comments inline. https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHelper.js File lib/notificationHelper.js (left): https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHelper.js#oldcode139 lib/notificationHelper.js:139: activeNotification.onClicked(); Calling onClicked (now ...
1 week, 3 days ago (2017-08-12 16:57:48 UTC) #2
kzar
(Adding Thomas to Cc.) https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHelper.js#newcode29 lib/notificationHelper.js:29: // "Open" notifications are any ...
1 day, 4 hours ago (2017-08-21 13:59:25 UTC) #3
Thomas Greiner
31 minutes ago (2017-08-22 18:17:00 UTC) #4
https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel...
lib/notificationHelper.js:260: // Critical notifications should not go away in a
few seconds, as
On 2017/08/21 13:59:24, kzar wrote:
> On 2017/08/12 16:57:48, Manish Jethani wrote:
> > I took the liberty to add this here. If it's a critical notification, maybe
it
> > shouldn't just go away in five seconds the way it does on macOS? macOS's
> > security update notifications, for example, persist until the user closes
them
> > explicitly.
> 
> Sounds sensible to me but you've not mentioned this in the issue or (I assume)
> discussed this with Thomas. We need to check with him first before making this
> change.

I can confirm that critical notifications are not meant to disappear as you can
also read from the comment above: "We use the highest priority to prevent the
notification from closing automatically."

Note that at the time when we first implemented them, the "requireInteraction"
option didn't exist yet (see
https://developer.chrome.com/apps/notifications#type-NotificationOptions) which
is why we had to resort to using "priority" instead to keep it open as long as
possible.

It's news to me that the notification would only go away on some platforms
though because I was under the impression that this was the case across all
platforms.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5