Issue 2193 - Added notification opt-out (Firefox)

Issue 2193 - Added notification opt-out (Firefox)

[Thomas Greiner, 2015-04-14 15:23:45]

Note that I've intentionally used a look that's more native to Firefox than what
you see in the mockup in the issue description to avoid unnecessary complexity
and since notifications are inherently platform-specific anyway.

[Wladimir Palant, 2015-06-26 13:13:09]

https://codereview.adblockplus.org/6526453483044864/diff/29321066/lib/ui.js
File lib/ui.js (right):

https://codereview.adblockplus.org/6526453483044864/diff/29321066/lib/ui.js#n...
lib/ui.js:133: this.value = (Prefs.notifications_ignoredcategories.indexOf("*")
== -1);
Nit: toggleIgnoreCategory() should really return the new value, not terribly
important however.

https://codereview.adblockplus.org/6526453483044864/diff/29321066/lib/ui.js#n...
lib/ui.js:607: Notification.toggleIgnoreCategory("*", true);
I'm not a huge fan of fallthrough in switch statements, it's very easy to miss.
At the very least we should adopt the suggestion from
https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style and have /* FALL
THROUGH */ instead of the missing break statement here.

https://codereview.adblockplus.org/6526453483044864/diff/29321066/lib/ui.js#n...
lib/ui.js:612: }, true);
Why true? We don't usually add capturing event listeners.

[Thomas Greiner, 2015-06-26 15:32:49]

https://codereview.adblockplus.org/6526453483044864/diff/29321066/lib/ui.js
File lib/ui.js (right):

https://codereview.adblockplus.org/6526453483044864/diff/29321066/lib/ui.js#n...
lib/ui.js:133: this.value = (Prefs.notifications_ignoredcategories.indexOf("*")
== -1);
On 2015/06/26 13:13:09, Wladimir Palant wrote:
> Nit: toggleIgnoreCategory() should really return the new value, not terribly
> important however.

Totally makes sense. I created #2727 for that now so that we can take care of it
later on.

https://codereview.adblockplus.org/6526453483044864/diff/29321066/lib/ui.js#n...
lib/ui.js:607: Notification.toggleIgnoreCategory("*", true);
On 2015/06/26 13:13:09, Wladimir Palant wrote:
> I'm not a huge fan of fallthrough in switch statements, it's very easy to
miss.
> At the very least we should adopt the suggestion from
> https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style and have /* FALL
> THROUGH */ instead of the missing break statement here.

Done. I guess it would make sense to add this to
https://adblockplus.org/en/coding-style#javascript then for future reference.

Also note Sebastian's comment on the Platform review:
https://codereview.adblockplus.org/29321084/diff/29321085/notification.js#new...
Therefore  I'd personally also add that "break" should always explicitly be set
in switch-statements.

https://codereview.adblockplus.org/6526453483044864/diff/29321066/lib/ui.js#n...
lib/ui.js:612: }, true);
On 2015/06/26 13:13:09, Wladimir Palant wrote:
> Why true? We don't usually add capturing event listeners.

Done.

[Wladimir Palant, 2015-06-27 06:52:33]

LGTM