Issue 29370562: [adblockpluscore] Issue 4762 - Added "relentless" notification that shows up in intervals (Closed)
|
Created:
Dec. 30, 2016, 9:48 a.m. by wspee Modified:
Feb. 21, 2017, 11:14 a.m. CC:
Wladimir Palant Visibility:
Public. |
Description[adblockpluscore] Issue 4762 - Added "relentless" notification that shows up in intervals
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressed review comments #
Total comments: 2
Patch Set 3 : Changed not equal comparison #
Total comments: 2
Patch Set 4 : Added tests for relentless notification #
Total comments: 10
Patch Set 5 : Added another testcase and fixed review comments #
Total comments: 12
Patch Set 6 : Adressed review comments #MessagesTotal messages: 14
https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:212: let shown = undefined; Initializing a variable with undefined is equivalent to declaring a variable without assignment. So if this is what you want to do, you can just use: let shown; https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:213: if (Prefs.notificationdata.shown instanceof Object) Checking for the prototype seems unnecessary here. You rather want to check for the type, to just make sure that the property access below doesn't fail: if (typeof Prefs.notificationdata.shown == "object") ... https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:216: if (typeof shown !== "undefined") As per the Mozilla coding style guide (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...) which is referenced in our coding style guide (https://adblockplus.org/coding-style#javascript), we prefer != over !==. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:218: if (typeof notification.interval === "number") Same here, use == instead of ===. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:221: continue For consistency, please add the optional semicolon. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:224: continue For consistency, please add the optional semicolon. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:308: let newShown = {} For consistency, please add the optional semicolon. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:314: if (!(data.shown instanceof Object)) See above, you should probably use typeof here. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:317: data.shown[id] = Date.now(); I suppose, we should use the "now" variable here as well.
https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:212: let shown = undefined; On 2017/01/06 11:21:08, Sebastian Noack wrote: > Initializing a variable with undefined is equivalent to declaring a variable > without assignment. So if this is what you want to do, you can just use: > > let shown; Done. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:213: if (Prefs.notificationdata.shown instanceof Object) On 2017/01/06 11:21:08, Sebastian Noack wrote: > Checking for the prototype seems unnecessary here. You rather want to check for > the type, to just make sure that the property access below doesn't fail: > > if (typeof Prefs.notificationdata.shown == "object") > ... Done. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:216: if (typeof shown !== "undefined") On 2017/01/06 11:21:08, Sebastian Noack wrote: > As per the Mozilla coding style guide > (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...) > which is referenced in our coding style guide > (https://adblockplus.org/coding-style#javascript), we prefer != over !==. Done. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:218: if (typeof notification.interval === "number") On 2017/01/06 11:21:07, Sebastian Noack wrote: > Same here, use == instead of ===. Done. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:221: continue On 2017/01/06 11:21:08, Sebastian Noack wrote: > For consistency, please add the optional semicolon. Done. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:224: continue On 2017/01/06 11:21:07, Sebastian Noack wrote: > For consistency, please add the optional semicolon. Done. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:308: let newShown = {} On 2017/01/06 11:21:08, Sebastian Noack wrote: > For consistency, please add the optional semicolon. Done. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:314: if (!(data.shown instanceof Object)) On 2017/01/06 11:21:08, Sebastian Noack wrote: > See above, you should probably use typeof here. Done. https://codereview.adblockplus.org/29370562/diff/29370563/lib/notification.js... lib/notification.js:317: data.shown[id] = Date.now(); On 2017/01/06 11:21:08, Sebastian Noack wrote: > I suppose, we should use the "now" variable here as well. Done.
Added Felix and removed Wladimir as a reviewer as he will be afk for the next two weeks.
https://codereview.adblockplus.org/29370562/diff/29370799/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29370799/lib/notification.js... lib/notification.js:314: if (!(typeof data.shown == "object")) How about |typeof != "object"|?
https://codereview.adblockplus.org/29370562/diff/29370799/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29370799/lib/notification.js... lib/notification.js:314: if (!(typeof data.shown == "object")) On 2017/01/06 14:17:02, Sebastian Noack wrote: > How about |typeof != "object"|? Done.
LGTM
Looks good, just have one nit for the changes. But: I'm missing tests for the new logic, could you write some? See `test/notification.js`. https://codereview.adblockplus.org/29370562/diff/29370804/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29370804/lib/notification.js... lib/notification.js:309: for (let old_id of data.shown) Nit: Camel case rather than underscore please :)
Added tests to test relentless notification features: * cannot be disabled * shows up in intervals https://codereview.adblockplus.org/29370562/diff/29370804/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29370804/lib/notification.js... lib/notification.js:309: for (let old_id of data.shown) On 2017/01/12 16:39:49, Felix Dahlke wrote: > Nit: Camel case rather than underscore please :) Done.
Sorry for the delay, looks good! Few more comments. https://codereview.adblockplus.org/29370562/diff/29372039/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29372039/lib/notification.js... lib/notification.js:227: if (notification.type !== "relentless" && Prefs.notifications_ignoredcategories.indexOf("*") != -1) :) https://codereview.adblockplus.org/29370562/diff/29372039/test/notification.js File test/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29372039/test/notification.j... test/notification.js:374: exports.testInterval = function(test) { Nit: Opening brace should be on the next line https://codereview.adblockplus.org/29370562/diff/29372039/test/notification.j... test/notification.js:374: exports.testInterval = function(test) { How about writing a second test that combines a relentless notification with interval and url filter? That's the use case we want to go for in practice, so I think it would be good to have it covered. https://codereview.adblockplus.org/29370562/diff/29372039/test/notification.j... test/notification.js:384: test.deepEqual(showNotifications(), [relentless], "Relentless notifications are not shown initially"); Seems like the description here describes the situation when the test fails? How I see it, in the rest of the tests here the description describes the case when the test passes. IMO we should be consistent here. I maybe missing something though. https://codereview.adblockplus.org/29370562/diff/29372039/test/notification.j... test/notification.js:393: test.deepEqual(showNotifications(), [relentless], "Relentless notifications are not ignored"); Description nit: "Relentless notifications are shown after the interval"
https://codereview.adblockplus.org/29370562/diff/29372039/lib/notification.js File lib/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29372039/lib/notification.js... lib/notification.js:227: if (notification.type !== "relentless" && Prefs.notifications_ignoredcategories.indexOf("*") != -1) On 2017/01/19 17:16:14, Felix Dahlke wrote: > :) ;) https://codereview.adblockplus.org/29370562/diff/29372039/test/notification.js File test/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29372039/test/notification.j... test/notification.js:374: exports.testInterval = function(test) { On 2017/01/19 17:16:14, Felix Dahlke wrote: > Nit: Opening brace should be on the next line Done. https://codereview.adblockplus.org/29370562/diff/29372039/test/notification.j... test/notification.js:374: exports.testInterval = function(test) { On 2017/01/19 17:16:14, Felix Dahlke wrote: > How about writing a second test that combines a relentless notification with > interval and url filter? That's the use case we want to go for in practice, so I > think it would be good to have it covered. Done. https://codereview.adblockplus.org/29370562/diff/29372039/test/notification.j... test/notification.js:384: test.deepEqual(showNotifications(), [relentless], "Relentless notifications are not shown initially"); On 2017/01/19 17:16:14, Felix Dahlke wrote: > Seems like the description here describes the situation when the test fails? How > I see it, in the rest of the tests here the description describes the case when > the test passes. IMO we should be consistent here. I maybe missing something > though. Done. https://codereview.adblockplus.org/29370562/diff/29372039/test/notification.j... test/notification.js:393: test.deepEqual(showNotifications(), [relentless], "Relentless notifications are not ignored"); On 2017/01/19 17:16:14, Felix Dahlke wrote: > Description nit: "Relentless notifications are shown after the interval" Done.
LGTM - added a couple of test description nits you might want to address, but they're rather cosmetic. https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.js File test/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:410: test.deepEqual(showNotifications(), [], "Relentless notification is not shown without url"); Nit: "url" -> "URL"? https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:411: test.deepEqual(showNotifications("http://bar.com"), [], "Relentless notification is not shown to the wrong url"); Nit: "shown to the wrong url" -> "shown for a non-matching URL"? https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:412: test.deepEqual(showNotifications("http://foo.com"), [relentless], "Relentless notification is shown to correct url"); Nit: "shown to correct url" "shown for a matching URL"? https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:421: test.deepEqual(showNotifications(), [], "Relentless notifications are not shown after the interval without url"); Nit: "url" -> "URL"? https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:422: test.deepEqual(showNotifications("http://bar.com"), [], "Relentless notifications are not shown after the interval to the wrong url"); Nit: "to the wrong url" -> "for a non-matching URL"? https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:423: test.deepEqual(showNotifications("http://bar.foo.com"), [relentless], "Relentless notifications are shown after the interval to the correct url"); Nit: "to the correct url" -> "for a matching URL"?
https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.js File test/notification.js (right): https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:410: test.deepEqual(showNotifications(), [], "Relentless notification is not shown without url"); On 2017/01/20 09:51:37, Felix Dahlke wrote: > Nit: "url" -> "URL"? Done. https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:411: test.deepEqual(showNotifications("http://bar.com"), [], "Relentless notification is not shown to the wrong url"); On 2017/01/20 09:51:36, Felix Dahlke wrote: > Nit: "shown to the wrong url" -> "shown for a non-matching URL"? Done. https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:412: test.deepEqual(showNotifications("http://foo.com"), [relentless], "Relentless notification is shown to correct url"); On 2017/01/20 09:51:37, Felix Dahlke wrote: > Nit: "shown to correct url" "shown for a matching URL"? Done. https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:421: test.deepEqual(showNotifications(), [], "Relentless notifications are not shown after the interval without url"); On 2017/01/20 09:51:37, Felix Dahlke wrote: > Nit: "url" -> "URL"? Done. https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:422: test.deepEqual(showNotifications("http://bar.com"), [], "Relentless notifications are not shown after the interval to the wrong url"); On 2017/01/20 09:51:36, Felix Dahlke wrote: > Nit: "to the wrong url" -> "for a non-matching URL"? Done. https://codereview.adblockplus.org/29370562/diff/29372772/test/notification.j... test/notification.js:423: test.deepEqual(showNotifications("http://bar.foo.com"), [relentless], "Relentless notifications are shown after the interval to the correct url"); On 2017/01/20 09:51:37, Felix Dahlke wrote: > Nit: "to the correct url" -> "for a matching URL"? Done.
LGTM! 
|
