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

Issue 29503587: Issue 5464 - Upgrade to new asynchronous version of abp2blocklist (Closed)

Created:
Aug. 2, 2017, 10:47 a.m. by Manish Jethani
Modified:
Aug. 21, 2017, 8:12 p.m.
Reviewers:
Sebastian Noack, kzar
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Issue 5464 - Upgrade to new asynchronous version of abp2blocklist

Patch Set 1 #

Total comments: 5

Patch Set 2 : Patch RegExpFilter.typeMap with WEBRTC #

Total comments: 5

Patch Set 3 : Split out setContentBlocker handlers into then and catch, move patch to lib/requestBlocker.js #

Total comments: 6

Patch Set 4 : Merge then handlers #

Patch Set 5 : Don't wrap setContentBlocker call into another promise #

Total comments: 4

Patch Set 6 : Use function declaration syntax #

Unified diffs Side-by-side diffs Delta from patch set Stats (+114 lines, -60 lines) Patch
M dependencies View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M lib/requestBlocker.js View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M safari/contentBlocking.js View 1 2 3 4 5 1 chunk +109 lines, -59 lines 0 comments Download

Messages

Total messages: 13
Manish Jethani
Aug. 2, 2017, 10:47 a.m. (2017-08-02 10:47:26 UTC) #1
Manish Jethani
Patch Set 1 This just might get shot down, but nevertheless it helps to see ...
Aug. 2, 2017, 10:55 a.m. (2017-08-02 10:55:11 UTC) #2
Manish Jethani
Patch Set 2: Patch RegExpFilter.typeMap with WEBRTC https://codereview.adblockplus.org/29503587/diff/29515595/safari/ext/common.js File safari/ext/common.js (right): https://codereview.adblockplus.org/29503587/diff/29515595/safari/ext/common.js#newcode20 safari/ext/common.js:20: window.addEventListener("load", () ...
Aug. 14, 2017, 5:23 p.m. (2017-08-14 17:23:49 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29503587/diff/29515595/safari/contentBlocking.js File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29515595/safari/contentBlocking.js#newcode168 safari/contentBlocking.js:168: setContentBlockerHandler); It seems somewhat dirty to me to register ...
Aug. 15, 2017, 3:25 p.m. (2017-08-15 15:25:01 UTC) #4
Manish Jethani
Patch Set 3: Split out setContentBlocker handlers into then and catch, move patch to lib/requestBlocker.js ...
Aug. 16, 2017, 10:23 a.m. (2017-08-16 10:23:02 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlocking.js File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlocking.js#newcode136 safari/contentBlocking.js:136: if (error instanceof Error) On 2017/08/16 10:23:02, Manish Jethani ...
Aug. 16, 2017, 10:30 a.m. (2017-08-16 10:30:35 UTC) #6
Manish Jethani
Patch Set 4: Merge then handlers https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlocking.js File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlocking.js#newcode136 safari/contentBlocking.js:136: if (error instanceof ...
Aug. 16, 2017, 11:13 a.m. (2017-08-16 11:13:38 UTC) #7
Sebastian Noack
LGTM. But I'd like Dave to review these changes as well.
Aug. 16, 2017, 11:20 a.m. (2017-08-16 11:20:44 UTC) #8
Manish Jethani
Patch Set 5: Don't wrap setContentBlocker call into another promise I just realized that there's ...
Aug. 16, 2017, 11:34 a.m. (2017-08-16 11:34:55 UTC) #9
Sebastian Noack
On 2017/08/16 11:34:55, Manish Jethani wrote: > I just realized that there's no need to ...
Aug. 16, 2017, 11:38 a.m. (2017-08-16 11:38:13 UTC) #10
kzar
https://codereview.adblockplus.org/29503587/diff/29517577/dependencies File dependencies (left): https://codereview.adblockplus.org/29503587/diff/29517577/dependencies#oldcode7 dependencies:7: adblockplusui = adblockplusui hg:53626d2055a6 git:4ce9666 Nit: Unrelated change? https://codereview.adblockplus.org/29503587/diff/29517577/safari/contentBlocking.js ...
Aug. 21, 2017, 12:30 p.m. (2017-08-21 12:30:12 UTC) #11
Manish Jethani
Patch Set 6: Use function declaration syntax https://codereview.adblockplus.org/29503587/diff/29517577/dependencies File dependencies (left): https://codereview.adblockplus.org/29503587/diff/29517577/dependencies#oldcode7 dependencies:7: adblockplusui = ...
Aug. 21, 2017, 2:17 p.m. (2017-08-21 14:17:04 UTC) #12
kzar
Aug. 21, 2017, 2:53 p.m. (2017-08-21 14:53:33 UTC) #13
LGTM

Powered by Google App Engine
This is Rietveld