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

Issue 29886685: Issue 6856 - Remove FilterStorage.moveSubscription (Closed)

Created:
Sept. 20, 2018, 7:33 p.m. by Jon Sonesen
Modified:
Oct. 2, 2018, 5:16 p.m.
Reviewers:
Manish Jethani, hub
Base URL:
https://hg.adblockplus.org/adblockpluscore
Visibility:
Public.

Description

Issue 6856 - Remove FilterStorage.moveSubscription

Patch Set 1 #

Total comments: 10

Patch Set 2 : Address PS1 Comments #

Total comments: 1

Patch Set 3 : Remove erroneous subscription.moved listener #

Patch Set 4 : Fix JSdoc String #

Total comments: 9

Patch Set 5 : Address PS4 Comment #

Total comments: 9

Patch Set 6 : Address PS5 Comments #

Patch Set 7 : Address PS6 Comment #

Patch Set 8 : Address PS7 comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -104 lines) Patch
M lib/filterListener.js View 1 2 chunks +1 line, -2 lines 0 comments Download
M lib/filterStorage.js View 1 2 3 4 5 6 7 6 chunks +31 lines, -52 lines 0 comments Download
M lib/synchronizer.js View 1 chunk +1 line, -1 line 0 comments Download
M test/filterStorage.js View 2 2 chunks +3 lines, -34 lines 0 comments Download
M test/filterStorage_readwrite.js View 2 chunks +6 lines, -6 lines 0 comments Download
M test/synchronizer.js View 2 chunks +9 lines, -9 lines 0 comments Download

Messages

Total messages: 18
Jon Sonesen
Sept. 20, 2018, 7:33 p.m. (2018-09-20 19:33:26 UTC) #1
Manish Jethani
Patch Set 1 Some initial comment, I'll get into this in more detail in the ...
Sept. 21, 2018, 9:21 a.m. (2018-09-21 09:21:59 UTC) #2
Jon Sonesen
Thanks Manish, sorry for the delay. Gonna try and knock this out asap. https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.js File ...
Sept. 27, 2018, 3:14 p.m. (2018-09-27 15:14:05 UTC) #3
Manish Jethani
I'll take another look at this tomorrow, but a couple of comments for now. https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js ...
Sept. 27, 2018, 5:42 p.m. (2018-09-27 17:42:55 UTC) #4
Jon Sonesen
I'll wait on the rest of the comments before adding a new patch then. Thanks
Sept. 27, 2018, 6:36 p.m. (2018-09-27 18:36:11 UTC) #5
Jon Sonesen
https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js#newcode447 lib/filterStorage.js:447: let subscriptions = [...this.subscriptions()].filter( On 2018/09/27 17:42:54, Manish Jethani ...
Sept. 27, 2018, 6:47 p.m. (2018-09-27 18:47:00 UTC) #6
Manish Jethani
https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js#newcode447 lib/filterStorage.js:447: let subscriptions = [...this.subscriptions()].filter( On 2018/09/27 18:46:59, Jon Sonesen ...
Sept. 28, 2018, 8:15 a.m. (2018-09-28 08:15:49 UTC) #7
Manish Jethani
I still want to take a proper look at the whole patch as well as ...
Sept. 28, 2018, 10:33 p.m. (2018-09-28 22:33:25 UTC) #8
Jon Sonesen
https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js#newcode93 lib/filterStorage.js:93: * @returns {number} On 2018/09/27 17:42:54, Manish Jethani wrote: ...
Oct. 1, 2018, 3:02 a.m. (2018-10-01 03:02:23 UTC) #9
Manish Jethani
https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js#newcode115 lib/filterStorage.js:115: for (let subscription of FilterStorage.subscriptions()) Since this is within ...
Oct. 1, 2018, 10:40 a.m. (2018-10-01 10:40:46 UTC) #10
Manish Jethani
https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js#oldcode376 lib/filterStorage.js:376: this.subscriptions = parser.subscriptions; Note: An implication of this change ...
Oct. 1, 2018, 10:53 a.m. (2018-10-01 10:53:14 UTC) #11
Jon Sonesen
https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js#oldcode376 lib/filterStorage.js:376: this.subscriptions = parser.subscriptions; On 2018/10/01 10:53:13, Manish Jethani wrote: ...
Oct. 1, 2018, 3:40 p.m. (2018-10-01 15:40:57 UTC) #12
Manish Jethani
https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js#newcode115 lib/filterStorage.js:115: for (let subscription of FilterStorage.subscriptions()) On 2018/10/01 15:40:57, Jon ...
Oct. 1, 2018, 4:12 p.m. (2018-10-01 16:12:52 UTC) #13
Manish Jethani
https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js#newcode373 lib/filterStorage.js:373: if (this.subscriptionCount == 0) You mean to change this ...
Oct. 2, 2018, 12:41 a.m. (2018-10-02 00:41:30 UTC) #14
Jon Sonesen
On 2018/10/02 00:41:30, Manish Jethani wrote: > https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js > File lib/filterStorage.js (right): > > https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js#newcode373 ...
Oct. 2, 2018, 3:06 p.m. (2018-10-02 15:06:12 UTC) #15
Manish Jethani
On 2018/10/02 15:06:12, Jon Sonesen wrote: > On 2018/10/02 00:41:30, Manish Jethani wrote: > [..] ...
Oct. 2, 2018, 4:05 p.m. (2018-10-02 16:05:25 UTC) #16
Jon Sonesen
https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js#newcode115 lib/filterStorage.js:115: for (let subscription of FilterStorage.subscriptions()) On 2018/10/01 16:12:52, Manish ...
Oct. 2, 2018, 4:23 p.m. (2018-10-02 16:23:49 UTC) #17
Manish Jethani
Oct. 2, 2018, 4:47 p.m. (2018-10-02 16:47:58 UTC) #18
LGTM

Powered by Google App Engine
This is Rietveld