Issue 2420 - Move notification show logic to core

Issue 2420 - Move notification show logic to core

This also solves issue 2367 - didn't make too much sense to tackle these in
isolation.

[Wladimir Palant, 2015-06-08 11:33:15]

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
File lib/notification.js (right):

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:89: });
Mind changing this into arrow function as well for consistency?

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:96:
notificationTimer.initWithCallback(Notification.showNext,
Notification.showNext.bind(this)? The next person changing something here might
spend lots of time wondering why their code doesn't work.

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:185: showListeners.splice(showListeners.indexOf(listener));
For reference:

[1,2,3].splice(1) => [1]
[1,2,3].splice(-1) => [1,2]

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:191: removeAllShowListeners: function()
Why do we need that function? Seems to be a footgun. Normally, code should only
remove listeners it added itself.

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:382: for (let listener of questionListeners[id])
Please keep using a temporary variable here - otherwise questionListeners[id]
will be evaluated for each loop iteration in all browsers but Firefox.

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/ui.js:422: Notification.addShowListener(function(notification)
How about:

Notification.addShowListener(notification => 
{
  ...
});

You don't need to bind this then.

[Felix Dahlke, 2015-06-08 19:34:34]

New patch set is up.

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
File lib/notification.js (right):

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:89: });
On 2015/06/08 11:33:15, Wladimir Palant wrote:
> Mind changing this into arrow function as well for consistency?

Done.

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:96:
notificationTimer.initWithCallback(Notification.showNext,
On 2015/06/08 11:33:15, Wladimir Palant wrote:
> Notification.showNext.bind(this)? The next person changing something here
might
> spend lots of time wondering why their code doesn't work.

Done.

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:185: showListeners.splice(showListeners.indexOf(listener));
On 2015/06/08 11:33:15, Wladimir Palant wrote:
> For reference:
> 
> [1,2,3].splice(1) => [1]
> [1,2,3].splice(-1) => [1,2]

Done.

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:191: removeAllShowListeners: function()
On 2015/06/08 11:33:15, Wladimir Palant wrote:
> Why do we need that function? Seems to be a footgun. Normally, code should
only
> remove listeners it added itself.

Done.

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:382: for (let listener of questionListeners[id])
On 2015/06/08 11:33:15, Wladimir Palant wrote:
> Please keep using a temporary variable here - otherwise questionListeners[id]
> will be evaluated for each loop iteration in all browsers but Firefox.

Done.

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/ui.js:422: Notification.addShowListener(function(notification)
On 2015/06/08 11:33:15, Wladimir Palant wrote:
> How about:
> 
> Notification.addShowListener(notification => 
> {
>   ...
> });
> 
> You don't need to bind this then.

Done.

http://codereview.adblockplus.org/5256408131960832/diff/5685265389584384/lib/...
File lib/notification.js (right):

http://codereview.adblockplus.org/5256408131960832/diff/5685265389584384/lib/...
lib/notification.js:30: let INITIAL_DELAY = 12 * MILLIS_IN_MINUTE;
Nevermind this, just based this commit on an earlier one and this is the only
change reverted by that.

[Wladimir Palant, 2015-06-08 20:45:14]

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
File lib/notification.js (right):

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:191: removeAllShowListeners: function()
On 2015/06/08 19:34:34, Felix H. Dahlke wrote:
> On 2015/06/08 11:33:15, Wladimir Palant wrote:
> > Why do we need that function? Seems to be a footgun. Normally, code should
> only
> > remove listeners it added itself.
> 
> Done.

Why am I still seeing it? :)

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:382: for (let listener of questionListeners[id])
On 2015/06/08 19:34:34, Felix H. Dahlke wrote:
> On 2015/06/08 11:33:15, Wladimir Palant wrote:
> > Please keep using a temporary variable here - otherwise
questionListeners[id]
> > will be evaluated for each loop iteration in all browsers but Firefox.
> 
> Done.

I would have called that variable "listeners" or something equally short.
Doesn't matter much however...

[Felix Dahlke, 2015-06-08 22:01:53]

Another patch set

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
File lib/notification.js (right):

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:191: removeAllShowListeners: function()
On 2015/06/08 20:45:15, Wladimir Palant wrote:
> On 2015/06/08 19:34:34, Felix H. Dahlke wrote:
> > On 2015/06/08 11:33:15, Wladimir Palant wrote:
> > > Why do we need that function? Seems to be a footgun. Normally, code should
> > only
> > > remove listeners it added itself.
> > 
> > Done.
> 
> Why am I still seeing it? :)

Good question, done for real now.

http://codereview.adblockplus.org/5256408131960832/diff/5629499534213120/lib/...
lib/notification.js:382: for (let listener of questionListeners[id])
On 2015/06/08 20:45:15, Wladimir Palant wrote:
> On 2015/06/08 19:34:34, Felix H. Dahlke wrote:
> > On 2015/06/08 11:33:15, Wladimir Palant wrote:
> > > Please keep using a temporary variable here - otherwise
> questionListeners[id]
> > > will be evaluated for each loop iteration in all browsers but Firefox.
> > 
> > Done.
> 
> I would have called that variable "listeners" or something equally short.
> Doesn't matter much however...

True, wasn't a brilliant name. Done, since I was uploading another patch set
anyway.

[Wladimir Palant, 2015-06-08 22:16:59]

LGTM