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

Issue 29327244: Issue 3024 - Added opt-out to Chrome notifications (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 6 months ago by Thomas Greiner
Modified:
4 years, 6 months ago
Reviewers:
Sebastian Noack
CC:
Felix Dahlke
Visibility:
Public.

Description

This review also fixes code that uses the removed `ext.windows` namespace and thereby fixes notification links which are currently broken.

Patch Set 1 #

Total comments: 7

Patch Set 2 : Centralized button management #

Total comments: 7

Patch Set 3 : Extracted button creation into own function #

Patch Set 4 : Fixed documentation of shouldDisplay function #

Total comments: 2

Patch Set 5 : Made `getNotificationButtons` independent of external variables #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+113 lines, -39 lines) Patch
M _locales/en_US/messages.json View 1 chunk +6 lines, -0 lines 0 comments Download
M lib/notificationHelper.js View 1 2 3 4 6 chunks +83 lines, -38 lines 2 comments Download
M options.html View 2 chunks +6 lines, -1 line 0 comments Download
M options.js View 1 1 chunk +18 lines, -0 lines 0 comments Download

Messages

Total messages: 9
Thomas Greiner
Admittingly, I'm not too proud of the code in options.js but instead of reengineering the ...
4 years, 6 months ago (2015-09-10 18:55:16 UTC) #1
Thomas Greiner
Just noticed that the opt-out button is not supposed to be shown for critical notifications. ...
4 years, 6 months ago (2015-09-11 09:41:40 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29327244/diff/29327245/lib/notificationHelper.js File lib/notificationHelper.js (left): https://codereview.adblockplus.org/29327244/diff/29327245/lib/notificationHelper.js#oldcode66 lib/notificationHelper.js:66: activeNotification.links.forEach(function(link) I wonder why we did it that complicated ...
4 years, 6 months ago (2015-09-11 10:58:26 UTC) #3
Thomas Greiner
To avoid unnecessary complexity and code duplication in the `notificationButtonClick` function I decided to store ...
4 years, 6 months ago (2015-09-11 12:51:48 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29327244/diff/29327245/options.js File options.js (right): https://codereview.adblockplus.org/29327244/diff/29327245/options.js#newcode116 options.js:116: var found = tabs[i].querySelector("[data-section='" + msg.section + "'"); On ...
4 years, 6 months ago (2015-09-11 13:09:55 UTC) #5
Thomas Greiner
I also added an extra patch for the requested change from https://codereview.adblockplus.org/29326181/#msg9. Hope you don't ...
4 years, 6 months ago (2015-09-11 14:03:45 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29327244/diff/29327313/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29327244/diff/29327313/lib/notificationHelper.js#newcode215 lib/notificationHelper.js:215: opts.buttons = activeButtons.map(function(button) On 2015/09/11 14:03:44, Thomas Greiner wrote: ...
4 years, 6 months ago (2015-09-11 14:15:41 UTC) #7
Thomas Greiner
https://codereview.adblockplus.org/29327244/diff/29327431/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29327244/diff/29327431/lib/notificationHelper.js#newcode65 lib/notificationHelper.js:65: activeButtons = []; On 2015/09/11 14:15:39, Sebastian Noack wrote: ...
4 years, 6 months ago (2015-09-11 14:43:58 UTC) #8
Sebastian Noack
4 years, 6 months ago (2015-09-11 14:51:00 UTC) #9
LGMT. But please commit the unrelated changes pointed out below separately as
"Noissue - " with a link to this review in the second line. Thanks.

https://codereview.adblockplus.org/29327244/diff/29327455/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29327244/diff/29327455/lib/notificationHel...
lib/notificationHelper.js:117: for (let link of activeNotification.links)
This one is unrelated.

https://codereview.adblockplus.org/29327244/diff/29327455/lib/notificationHel...
lib/notificationHelper.js:293: let shouldDisplay =
This one is unrelated.
Sign in to reply to this message.

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