Issue 3826 - Filter preference change events

Issue 3826 - Filter preference change events

[Sebastian Noack, 2016-03-17 16:34:17]

https://codereview.adblockplus.org/29338534/diff/29338535/qunit/common.js
File qunit/common.js (left):

https://codereview.adblockplus.org/29338534/diff/29338535/qunit/common.js#old...
qunit/common.js:49: if (!(value instanceof ext._EventTarget))
Note that this logic has already been outdated since we turned untilLoaded
(formerly isLoaded) into a promise. However, there is no harm done if we just
backup those properties which aren't preferences themselves as well.

[kzar, 2016-03-18 15:49:59]

https://codereview.adblockplus.org/29338534/diff/29338535/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29338534/diff/29338535/lib/events.js#newco...
lib/events.js:56: if (callbacks)
I guess if the last callback for a name is removed we should also remove the
array?

https://codereview.adblockplus.org/29338534/diff/29338535/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29338534/diff/29338535/lib/prefs.js#newcod...
lib/prefs.js:186: * value of the specified preference changed.
Nit: Should be "a specified"

[Sebastian Noack, 2016-03-18 18:15:50]

https://codereview.adblockplus.org/29338534/diff/29338535/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29338534/diff/29338535/lib/events.js#newco...
lib/events.js:56: if (callbacks)
On 2016/03/18 15:49:59, kzar wrote:
> I guess if the last callback for a name is removed we should also remove the
> array?

I had a similar discussion with Wladimir about the implementation of
messaging.Port for Firefox. And he pointed out that we currently never remove
messages listeners (that is true for preferences here as well), and if in the
future we use temporary listeners we don't have to create a new array at least
when they are added again.

https://codereview.adblockplus.org/29338534/diff/29338535/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29338534/diff/29338535/lib/prefs.js#newcod...
lib/prefs.js:186: * value of the specified preference changed.
On 2016/03/18 15:49:59, kzar wrote:
> Nit: Should be "a specified"

Done.

[kzar, 2016-03-18 18:44:14]

LGTM

https://codereview.adblockplus.org/29338534/diff/29338535/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29338534/diff/29338535/lib/events.js#newco...
lib/events.js:56: if (callbacks)
On 2016/03/18 18:15:49, Sebastian Noack wrote:
> On 2016/03/18 15:49:59, kzar wrote:
> > I guess if the last callback for a name is removed we should also remove the
> > array?
> 
> I had a similar discussion with Wladimir about the implementation of
> messaging.Port for Firefox. And he pointed out that we currently never remove
> messages listeners (that is true for preferences here as well), and if in the
> future we use temporary listeners we don't have to create a new array at least
> when they are added again.

Acknowledged.

[Sebastian Noack, 2016-03-19 19:49:30]

I found a bug caused by misspelled variable name. New patch set is up.

[kzar, 2016-03-20 14:35:33]

LGTM

[Sebastian Noack, 2016-03-22 15:43:10]

Rebased, and updated the adblockplusui dependency as the previous version were
still relying on Prefs.onChanged.

[kzar, 2016-03-22 15:48:59]

LGTM

[Sebastian Noack, 2016-03-23 11:37:22]

https://codereview.adblockplus.org/29338534/diff/29338952/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29338534/diff/29338952/lib/events.js#newco...
lib/events.js:75: let args = [];
Array.prototype.slice.call(arguments, 1), or passing arguments as array-like
object to another function, causes this function to be deoptimized in V8. So I
replaced it with a loop.

https://codereview.adblockplus.org/29338534/diff/29338952/lib/events.js#newco...
lib/events.js:79: let currentListeners = listeners.slice();
This is necessary to avoid side effects when a listener added/removed listeners
itself.

https://codereview.adblockplus.org/29338534/diff/29338952/lib/events.js#newco...
lib/events.js:84: };
Moreover, I think "listener" is a more appropriate name than "callback" in the
context here. So I changed that as well.

[kzar, 2016-03-23 11:56:01]

LGTM

https://codereview.adblockplus.org/29338534/diff/29338952/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29338534/diff/29338952/lib/events.js#newco...
lib/events.js:75: let args = [];
On 2016/03/23 11:37:22, Sebastian Noack wrote:
> Array.prototype.slice.call(arguments, 1), or passing arguments as array-like
> object to another function, causes this function to be deoptimized in V8. So I
> replaced it with a loop.

Acknowledged.

https://codereview.adblockplus.org/29338534/diff/29338952/lib/events.js#newco...
lib/events.js:79: let currentListeners = listeners.slice();
On 2016/03/23 11:37:22, Sebastian Noack wrote:
> This is necessary to avoid side effects when a listener added/removed
listeners
> itself.

Acknowledged.