Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29338976: Issue 3862 - Make filterListener use the new FilterNotifier API (Closed)

Created:
March 23, 2016, 2:34 p.m. by Sebastian Noack
Modified:
March 24, 2016, 3:25 p.m.
Reviewers:
kzar, Wladimir Palant
CC:
erikvvold
Visibility:
Public.

Description

Issue 3862 - Make filterListener use the new FilterNotifier API

Patch Set 1 #

Total comments: 5

Patch Set 2 : Addressed comments #

Patch Set 3 : Added filter actions that were implicitly handled before #

Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -65 lines) Patch
M lib/filterListener.js View 1 2 3 chunks +108 lines, -65 lines 0 comments Download

Messages

Total messages: 6
Sebastian Noack
March 23, 2016, 2:36 p.m. (2016-03-23 14:36:38 UTC) #1
kzar
https://codereview.adblockplus.org/29338976/diff/29338977/lib/filterListener.js File lib/filterListener.js (right): https://codereview.adblockplus.org/29338976/diff/29338977/lib/filterListener.js#newcode213 lib/filterListener.js:213: if (subscription.url in FilterStorage.knownSubscriptions && !subscription.disabled) Nit: Mind wrapping ...
March 23, 2016, 4:02 p.m. (2016-03-23 16:02:32 UTC) #2
Wladimir Palant
I assume that you didn't run the unit tests on this? Running them currently requires ...
March 23, 2016, 4:12 p.m. (2016-03-23 16:12:40 UTC) #3
Sebastian Noack
On 2016/03/23 16:12:40, Wladimir Palant wrote: > I assume that you didn't run the unit ...
March 23, 2016, 5:56 p.m. (2016-03-23 17:56:09 UTC) #4
Sebastian Noack
I just realized that I missed some actions that were implicitly handled before. New patch ...
March 23, 2016, 6:30 p.m. (2016-03-23 18:30:27 UTC) #5
kzar
March 23, 2016, 6:42 p.m. (2016-03-23 18:42:06 UTC) #6
LGTM

Powered by Google App Engine
This is Rietveld