Issue 3024 - Added opt-out to Chrome notifications

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

[Thomas Greiner, 2015-09-10 18:55:16]

Admittingly, I'm not too proud of the code in options.js but instead of
reengineering the "old" options page I decided to work around the limitation of
jQuery tabs. That code needs to be rewritten for the new options page anyway.

[Thomas Greiner, 2015-09-11 09:41:40]

Just noticed that the opt-out button is not supposed to be shown for critical
notifications. So for now please ignore that this hasn't been considered yet.
I'll add it and update the review as soon as the change is done and tested.

[Sebastian Noack, 2015-09-11 10:58:26]

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

https://codereview.adblockplus.org/29327244/diff/29327245/lib/notificationHel...
lib/notificationHelper.js:66: activeNotification.links.forEach(function(link)
I wonder why we did it that complicated in the first place. Do you remember?
From what I can tell the new code should be sufficient.

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

https://codereview.adblockplus.org/29327244/diff/29327245/lib/notificationHel...
lib/notificationHelper.js:177: let match = /<a>(.*?)<\/a>/.exec(texts.message ||
"");
Nit: .exec() simply returns null if you pass null or undefined. Therefore the
<|| ""> is redundant.

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 + "'");
It seems you forgot the closing ']' in the selector.

[Thomas Greiner, 2015-09-11 12:51:48]

To avoid unnecessary complexity and code duplication in the
`notificationButtonClick` function I decided to store the buttons separately in
an `activeButtons` variable.

I added the expected button placement to the ticket at
https://issues.adblockplus.org/ticket/3024#Whattotest

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

https://codereview.adblockplus.org/29327244/diff/29327245/lib/notificationHel...
lib/notificationHelper.js:66: activeNotification.links.forEach(function(link)
On 2015/09/11 10:58:26, Sebastian Noack wrote:
> I wonder why we did it that complicated in the first place. Do you remember?
> From what I can tell the new code should be sufficient.

From what I've seen `ext.pages.open` wasn't available when we implemented this
(see https://hg.adblockplus.org/adblockpluschrome/rev/3e05efe28a75).

It was added later on but not used in this part of the code from what it looks
like (see https://hg.adblockplus.org/adblockpluschrome/rev/063be19c6314).

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

https://codereview.adblockplus.org/29327244/diff/29327245/lib/notificationHel...
lib/notificationHelper.js:177: let match = /<a>(.*?)<\/a>/.exec(texts.message ||
"");
On 2015/09/11 10:58:26, Sebastian Noack wrote:
> Nit: .exec() simply returns null if you pass null or undefined. Therefore the
> <|| ""> is redundant.

Done.

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 2015/09/11 10:58:26, Sebastian Noack wrote:
> It seems you forgot the closing ']' in the selector.

Done.

Apparently, both `document.querySelector("[data-section='notifications'")` and
even `document.querySelector("[data-section='notifications")` return the correct
result and don't throw an error. That explains why I didn't spot this earlier.

Not sure why it works though but it's the same behavior in both Chrome and
Firefox and it doesn't appear to be standard-compliant from what I've read. I'll
check whether this is intended behavior and report it to the respective browsers
if necessary.

[Sebastian Noack, 2015-09-11 13:09:55]

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 2015/09/11 12:51:48, Thomas Greiner wrote:
> Apparently, both `document.querySelector("[data-section='notifications'")` and
> even `document.querySelector("[data-section='notifications")` return the
correct
> result and don't throw an error. That explains why I didn't spot this earlier.
> 
> Not sure why it works though but it's the same behavior in both Chrome and
> Firefox and it doesn't appear to be standard-compliant from what I've read.
I'll
> check whether this is intended behavior and report it to the respective
browsers
> if necessary.

My guess would be, that some popular browser didn't mind throwing an exception,
that some websites started to rely on that behavior, that other browser vendors
adopted that behavior for compatibility.

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

https://codereview.adblockplus.org/29327244/diff/29327313/lib/notificationHel...
lib/notificationHelper.js:157: let hasLinks = activeNotification.links &&
activeNotification.links.length > 0;
I actually preferred the way you did it before using <|| []>. This isn't a
hotspot where you have to care about creating unnecessary objects.

https://codereview.adblockplus.org/29327244/diff/29327313/lib/notificationHel...
lib/notificationHelper.js:161: activeButtons = [];
This function is becoming quite long. How about moving the logic to a separate
function getNotificationButtons()? I don't have a strong opinion here though.

https://codereview.adblockplus.org/29327244/diff/29327313/lib/notificationHel...
lib/notificationHelper.js:215: opts.buttons = activeButtons.map(function(button)
Why not a for-of loop?

[Thomas Greiner, 2015-09-11 14:03:45]

I also added an extra patch for the requested change from
https://codereview.adblockplus.org/29326181/#msg9. Hope you don't mind that I
included it in here.

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

https://codereview.adblockplus.org/29327244/diff/29327313/lib/notificationHel...
lib/notificationHelper.js:157: let hasLinks = activeNotification.links &&
activeNotification.links.length > 0;
On 2015/09/11 13:09:55, Sebastian Noack wrote:
> I actually preferred the way you did it before using <|| []>. This isn't a
> hotspot where you have to care about creating unnecessary objects.

Done. Note that I merely reverted the changes from my previous patch here so
this is what's currently in the extension.

https://codereview.adblockplus.org/29327244/diff/29327313/lib/notificationHel...
lib/notificationHelper.js:161: activeButtons = [];
On 2015/09/11 13:09:55, Sebastian Noack wrote:
> This function is becoming quite long. How about moving the logic to a separate
> function getNotificationButtons()? I don't have a strong opinion here though.

Done. I agree, should make it easier to comprehend.

https://codereview.adblockplus.org/29327244/diff/29327313/lib/notificationHel...
lib/notificationHelper.js:215: opts.buttons = activeButtons.map(function(button)
On 2015/09/11 13:09:55, Sebastian Noack wrote:
> Why not a for-of loop?

No particular reason.

The way I use it now in the new patch, however, helps us avoid the creation of a
temporary variable.

[Sebastian Noack, 2015-09-11 14:15:41]

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

https://codereview.adblockplus.org/29327244/diff/29327313/lib/notificationHel...
lib/notificationHelper.js:215: opts.buttons = activeButtons.map(function(button)
On 2015/09/11 14:03:44, Thomas Greiner wrote:
> On 2015/09/11 13:09:55, Sebastian Noack wrote:
> > Why not a for-of loop?
> 
> No particular reason.
> 
> The way I use it now in the new patch, however, helps us avoid the creation of
a
> temporary variable.

I see. However, much better with an arrow function now.

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

https://codereview.adblockplus.org/29327244/diff/29327431/lib/notificationHel...
lib/notificationHelper.js:65: activeButtons = [];
I find it kinda unexpected that a function that is supposed to "get" something,
changes global state. Also the function would be easier to debug, if you could
call it from the console, without these unexpcted side effects. So why don't you
set the global variable in the calling code?

[Thomas Greiner, 2015-09-11 14:43:58]

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

https://codereview.adblockplus.org/29327244/diff/29327431/lib/notificationHel...
lib/notificationHelper.js:65: activeButtons = [];
On 2015/09/11 14:15:39, Sebastian Noack wrote:
> I find it kinda unexpected that a function that is supposed to "get"
something,
> changes global state. Also the function would be easier to debug, if you could
> call it from the console, without these unexpcted side effects. So why don't
you
> set the global variable in the calling code?

Done. The idea was to ensure that `activeButtons` is always kept in sync but I
do see your point.

[Sebastian Noack, 2015-09-11 14:51:00]

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.