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

Issue 29346555: Issue 4156 - Adblocking filter only being removed in advanced tab fix (Closed)

Created:
June 15, 2016, 5:35 p.m. by saroyanm
Modified:
Aug. 23, 2016, 1:15 p.m.
Reviewers:
Thomas Greiner
Visibility:
Public.

Description

Issue 4156 - Adblocking filter only being removed in advanced tab fix

Patch Set 1 #

Total comments: 2

Patch Set 2 : Implementation simplification #

Total comments: 4

Patch Set 3 : Addressed Thomas comments #

Total comments: 14

Patch Set 4 : Merged updateLanguageCollections function #

Total comments: 2

Patch Set 5 : Reverted updateLanguageCollections function #

Patch Set 6 : Rebased to changeset #85 #

Patch Set 7 : Rebased to changeset #87 #

Patch Set 8 : Restricted duplications #

Total comments: 6

Patch Set 9 : Changed addItems method to only accept 1 parameter and changed recommendations initial status #

Unified diffs Side-by-side diffs Delta from patch set Stats (+58 lines, -61 lines) Patch
M new-options.js View 1 2 3 4 5 6 7 8 9 chunks +58 lines, -61 lines 0 comments Download

Messages

Total messages: 19
saroyanm
Thomas can you please have a look, when you have time.
June 15, 2016, 5:41 p.m. (2016-06-15 17:41:16 UTC) #1
Thomas Greiner
https://codereview.adblockplus.org/29346555/diff/29346556/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346556/new-options.js#newcode921 new-options.js:921: var knownSubscription = subscriptionsMap[subscription.url]; I'd prefer not having to ...
June 16, 2016, 11:09 a.m. (2016-06-16 11:09:33 UTC) #2
saroyanm
https://codereview.adblockplus.org/29346555/diff/29346556/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346556/new-options.js#newcode921 new-options.js:921: var knownSubscription = subscriptionsMap[subscription.url]; On 2016/06/16 11:09:33, Thomas Greiner ...
June 16, 2016, 1:35 p.m. (2016-06-16 13:35:48 UTC) #3
Thomas Greiner
https://codereview.adblockplus.org/29346555/diff/29346564/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346564/new-options.js#newcode947 new-options.js:947: if (subscription) This will always be `true`. Compare it ...
June 16, 2016, 4:10 p.m. (2016-06-16 16:10:34 UTC) #4
saroyanm
https://codereview.adblockplus.org/29346555/diff/29346564/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346564/new-options.js#newcode947 new-options.js:947: if (subscription) On 2016/06/16 16:10:34, Thomas Greiner wrote: > ...
June 16, 2016, 4:51 p.m. (2016-06-16 16:51:45 UTC) #5
Thomas Greiner
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldcode396 new-options.js:396: if (subscription.recommended == "ads") Why did you move this ...
June 16, 2016, 5:08 p.m. (2016-06-16 17:08:38 UTC) #6
saroyanm
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldcode396 new-options.js:396: if (subscription.recommended == "ads") On 2016/06/16 17:08:38, Thomas Greiner ...
June 16, 2016, 5:42 p.m. (2016-06-16 17:42:12 UTC) #7
Thomas Greiner
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldcode396 new-options.js:396: if (subscription.recommended == "ads") On 2016/06/16 17:42:11, saroyanm wrote: ...
June 17, 2016, 1:48 p.m. (2016-06-17 13:48:03 UTC) #8
saroyanm
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldcode396 new-options.js:396: if (subscription.recommended == "ads") On 2016/06/17 13:48:03, Thomas Greiner ...
June 21, 2016, 12:53 p.m. (2016-06-21 12:53:07 UTC) #9
Thomas Greiner
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldcode396 new-options.js:396: if (subscription.recommended == "ads") On 2016/06/21 12:53:07, saroyanm wrote: ...
June 22, 2016, 10:08 a.m. (2016-06-22 10:08:06 UTC) #10
saroyanm
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#oldcode396 new-options.js:396: if (subscription.recommended == "ads") On 2016/06/22 10:08:04, Thomas Greiner ...
June 22, 2016, 12:32 p.m. (2016-06-22 12:32:00 UTC) #11
Thomas Greiner
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newcode947 new-options.js:947: else if (subscription.url in subscriptionsMap) On 2016/06/16 17:42:11, saroyanm ...
June 23, 2016, 2:54 p.m. (2016-06-23 14:54:29 UTC) #12
saroyanm
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newcode947 new-options.js:947: else if (subscription.url in subscriptionsMap) On 2016/06/23 14:54:29, Thomas ...
June 23, 2016, 4:27 p.m. (2016-06-23 16:27:13 UTC) #13
saroyanm
Rebased to changeset #85
June 27, 2016, 1:37 p.m. (2016-06-27 13:37:13 UTC) #14
Thomas Greiner
https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newcode947 new-options.js:947: else if (subscription.url in subscriptionsMap) On 2016/06/23 16:27:12, saroyanm ...
July 13, 2016, 11:35 a.m. (2016-07-13 11:35:18 UTC) #15
saroyanm
Patch uploaded. https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29346566/new-options.js#newcode947 new-options.js:947: else if (subscription.url in subscriptionsMap) On 2016/07/13 ...
Aug. 1, 2016, 6:24 p.m. (2016-08-01 18:24:18 UTC) #16
Thomas Greiner
Looks good now. Only two minor things I noted. https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js#newcode73 new-options.js:73: ...
Aug. 4, 2016, 3:54 p.m. (2016-08-04 15:54:03 UTC) #17
saroyanm
https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29346555/diff/29348942/new-options.js#newcode73 new-options.js:73: Collection.prototype.addItems = function() On 2016/08/04 15:54:02, Thomas Greiner wrote: ...
Aug. 17, 2016, 2:59 p.m. (2016-08-17 14:59:16 UTC) #18
Thomas Greiner
Aug. 19, 2016, 10:24 a.m. (2016-08-19 10:24:06 UTC) #19
LGTM

Powered by Google App Engine
This is Rietveld