Issue 5354 - Adds handling for notifications on Opera

Issue 5354 - Adds handling for notifications on Opera

[Jon Sonesen, 2017-10-19 21:59:17]

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

https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHel...
lib/notificationHelper.js:211: delete options.buttons;
perhaps it is better to move this down or put all the logic in one control
statement?

[Sebastian Noack, 2017-10-21 21:04:28]

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

https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHel...
lib/notificationHelper.js:209: Adding buttons to notifications is not
supported.`)
I would skip the check for the error message. If it failed for some reason, we
will just keep getting the same error again once we try without buttons.

But I would add a comment here, that on Opera buttons are not supported, causing
an error here.

https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHel...
lib/notificationHelper.js:211: delete options.buttons;
On 2017/10/19 21:59:17, Jon Sonesen wrote:
> perhaps it is better to move this down or put all the logic in one control
> statement?

Yeah, I guess this operation makes more sense in the if-branch below.

[Jon Sonesen, 2017-10-30 22:41:41]

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

https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHel...
lib/notificationHelper.js:209: Adding buttons to notifications is not
supported.`)
On 2017/10/21 21:04:28, Sebastian Noack wrote:
> I would skip the check for the error message. If it failed for some reason, we
> will just keep getting the same error again once we try without buttons.
> 
> But I would add a comment here, that on Opera buttons are not supported,
causing
> an error here.

Done.

https://codereview.adblockplus.org/29583620/diff/29583621/lib/notificationHel...
lib/notificationHelper.js:211: delete options.buttons;
On 2017/10/21 21:04:28, Sebastian Noack wrote:
> On 2017/10/19 21:59:17, Jon Sonesen wrote:
> > perhaps it is better to move this down or put all the logic in one control
> > statement?
> 
> Yeah, I guess this operation makes more sense in the if-branch below.

Done.

[Sebastian Noack, 2017-10-30 22:46:43]

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

https://codereview.adblockplus.org/29583620/diff/29592648/lib/notificationHel...
lib/notificationHelper.js:215: if (activeNotification.type != "question")
You can just check both conditions in the same if-statement.

[Jon Sonesen, 2017-11-01 18:21:44]

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

https://codereview.adblockplus.org/29583620/diff/29592648/lib/notificationHel...
lib/notificationHelper.js:215: if (activeNotification.type != "question")
On 2017/10/30 22:46:43, Sebastian Noack wrote:
> You can just check both conditions in the same if-statement.

Done.

[Sebastian Noack, 2017-11-01 18:45:03]

LGTM except for one typo. Feel free to go ahead pushing the change once you
fixed the typo. There is no need for me to review it again (if you don't change
anything else).

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

https://codereview.adblockplus.org/29583620/diff/29594666/lib/notificationHel...
lib/notificationHelper.js:211: // Opera does not support the addtition of
buttons to notifications.
Typo: addtition