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

Issue 29860589: Issue 6829 - Free filters when subscriptions are empty

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 weeks ago by Jon Sonesen
Modified:
2 weeks, 6 days ago
Reviewers:
Manish Jethani
CC:
kzar
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 6829 - Free filters when subscriptions are empty

Patch Set 1 #

Total comments: 9

Patch Set 2 : Address PS1 comment #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -0 lines) Patch
M lib/filterStorage.js View 1 1 chunk +5 lines, -0 lines 0 comments Download
M test/filterStorage.js View 1 1 chunk +1 line, -0 lines 1 comment Download

Messages

Total messages: 8
Jon Sonesen
4 weeks ago (2018-08-21 21:44:47 UTC) #1
Jon Sonesen
Once I saw this method removeSubscriptionFilters which is iterating over all the filters in a ...
4 weeks ago (2018-08-21 21:50:41 UTC) #2
Manish Jethani
Similar to what we did here: https://github.com/adblockplus/adblockpluscore/commit/5aca7b994bd142bf44da691a98ec30e779eb5447 I think we should add some tests for ...
3 weeks, 4 days ago (2018-08-25 15:03:39 UTC) #3
jsonesen
https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js#newcode683 lib/filterStorage.js:683: if (filter.subscriptions.size == 0) On 2018/08/25 15:03:38, Manish Jethani ...
3 weeks, 4 days ago (2018-08-25 15:18:46 UTC) #4
jsonesen
https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js#newcode683 lib/filterStorage.js:683: if (filter.subscriptions.size == 0) On 2018/08/25 15:18:46, jsonesen wrote: ...
3 weeks, 4 days ago (2018-08-25 15:32:08 UTC) #5
Manish Jethani
https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js#newcode683 lib/filterStorage.js:683: if (filter.subscriptions.size == 0) On 2018/08/25 15:32:08, jsonesen wrote: ...
3 weeks, 3 days ago (2018-08-26 15:56:19 UTC) #6
Jon Sonesen
https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29860589/diff/29860590/lib/filterStorage.js#newcode148 lib/filterStorage.js:148: On 2018/08/25 15:03:38, Manish Jethani wrote: > We should ...
3 weeks, 2 days ago (2018-08-27 16:28:03 UTC) #7
Jon Sonesen
2 weeks, 6 days ago (2018-08-30 17:11:43 UTC) #8
https://codereview.adblockplus.org/29860589/diff/29869582/test/filterStorage.js
File test/filterStorage.js (right):

https://codereview.adblockplus.org/29860589/diff/29869582/test/filterStorage....
test/filterStorage.js:498: test.equal(Filter.knownFilters[filter1.text],
undefined);
This test is currently a noop (removing changes does not effect outcome) Not
sure exactly how to test this change here but I realize it should be done.
Sign in to reply to this message.

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