Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(72)

Issue 29338534: Issue 3826 - Filter preference change events (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 11 months ago by Sebastian Noack
Modified:
3 years, 11 months ago
Reviewers:
kzar
Visibility:
Public.

Description

Issue 3826 - Filter preference change events

Patch Set 1 #

Total comments: 6

Patch Set 2 : Fixed typo in comment #

Patch Set 3 : Rebased and fixed a typo #

Patch Set 4 : Rebased and updated adblockplusui dependency #

Patch Set 5 : Fixed some issues #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+122 lines, -37 lines) Patch
M background.js View 1 chunk +1 line, -5 lines 0 comments Download
M dependencies View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
A lib/events.js View 1 2 3 4 1 chunk +84 lines, -0 lines 5 comments Download
M lib/messaging.js View 1 2 3 4 5 chunks +6 lines, -13 lines 0 comments Download
M lib/prefs.js View 1 3 chunks +25 lines, -6 lines 0 comments Download
M lib/stats.js View 1 chunk +1 line, -4 lines 0 comments Download
M lib/uninstall.js View 1 chunk +1 line, -5 lines 0 comments Download
M metadata.common View 1 chunk +2 lines, -1 line 0 comments Download
M qunit/common.js View 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 10
Sebastian Noack
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#oldcode49 qunit/common.js:49: if (!(value instanceof ext._EventTarget)) Note that this logic has ...
3 years, 11 months ago (2016-03-17 16:34:17 UTC) #1
kzar
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#newcode56 lib/events.js:56: if (callbacks) I guess if the last callback for ...
3 years, 11 months ago (2016-03-18 15:49:59 UTC) #2
Sebastian Noack
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#newcode56 lib/events.js:56: if (callbacks) On 2016/03/18 15:49:59, kzar wrote: > I ...
3 years, 11 months ago (2016-03-18 18:15:50 UTC) #3
kzar
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#newcode56 lib/events.js:56: if (callbacks) On 2016/03/18 18:15:49, Sebastian Noack wrote: ...
3 years, 11 months ago (2016-03-18 18:44:14 UTC) #4
Sebastian Noack
I found a bug caused by misspelled variable name. New patch set is up.
3 years, 11 months ago (2016-03-19 19:49:30 UTC) #5
kzar
LGTM
3 years, 11 months ago (2016-03-20 14:35:33 UTC) #6
Sebastian Noack
Rebased, and updated the adblockplusui dependency as the previous version were still relying on Prefs.onChanged.
3 years, 11 months ago (2016-03-22 15:43:10 UTC) #7
kzar
LGTM
3 years, 11 months ago (2016-03-22 15:48:59 UTC) #8
Sebastian Noack
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#newcode75 lib/events.js:75: let args = []; Array.prototype.slice.call(arguments, 1), or passing arguments ...
3 years, 11 months ago (2016-03-23 11:37:22 UTC) #9
kzar
3 years, 11 months ago (2016-03-23 11:56:01 UTC) #10
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5