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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by Manish Jethani
Modified:
1 month, 3 weeks ago
Reviewers:
kzar1, kzar
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: 14
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 14 comments Download
M notification.js View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 7
Manish Jethani
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months ago (2017-08-21 13:59:25 UTC) #3
Thomas Greiner
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#newcode260 lib/notificationHelper.js:260: // Critical notifications should not go away in a ...
1 month, 4 weeks ago (2017-08-22 18:17:00 UTC) #4
Manish Jethani
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#newcode49 lib/notificationHelper.js:49: function prepareNotificationIconAndPopup(notification) On 2017/08/21 13:59:24, kzar wrote: > Since ...
1 month, 3 weeks ago (2017-08-24 09:54:12 UTC) #5
Manish Jethani
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#newcode49 lib/notificationHelper.js:49: function prepareNotificationIconAndPopup(notification) On 2017/08/21 13:59:24, kzar wrote: > Since ...
1 month, 3 weeks ago (2017-08-24 09:54:12 UTC) #6
kzar
1 month, 3 weeks ago (2017-08-29 12:16:30 UTC) #7
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/24 09:54:12, Manish Jethani wrote:
> 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've updated the issue now:
> 
> https://issues.adblockplus.org/ticket/5512?action=diff&version=5

Thanks
Sign in to reply to this message.

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