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. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 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 #
MessagesTotal messages: 13
Patch Set 1 This just might get shot down, but nevertheless it helps to see what kind of changes might be required in case we do decide to upgrade the Safari extension to the latest version of abp2blocklist. https://codereview.adblockplus.org/29503587/diff/29503588/dependencies File dependencies (left): https://codereview.adblockplus.org/29503587/diff/29503588/dependencies#oldcode4 dependencies:4: abp2blocklist = abp2blocklist hg:c2c40f08f9ce git:183568e This will likely have to be updated again if we do something about WebRTC. https://codereview.adblockplus.org/29503587/diff/29503588/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29503588/safari/contentBlock... safari/contentBlocking.js:55: return new Promise((resolve, reject) => I made this a promise now because it feels more natural here and also because errors bubble up naturally, whereas they may be lost with the callback. https://codereview.adblockplus.org/29503587/diff/29503588/safari/contentBlock... safari/contentBlocking.js:82: { Added some extra braces as per our style guide. https://codereview.adblockplus.org/29503587/diff/29503588/safari/contentBlock... safari/contentBlocking.js:123: pendingContentBlockerUpdate = { Note that we only need to save the last request. We can discard intermediate ones. We're only interested in making sure the rule set used by Safari is really based on the user's last selection. See also this comment: https://issues.adblockplus.org/ticket/5464#comment:7 https://codereview.adblockplus.org/29503587/diff/29503588/safari/contentBlock... safari/contentBlocking.js:160: afterContentBlockingFinished = null; This should be set to null before fulfilling the promise just to be correct.
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.j... safari/ext/common.js:20: window.addEventListener("load", () => We have to do this in the load handler because the classes haven't been defined yet.
https://codereview.adblockplus.org/29503587/diff/29515595/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29515595/safari/contentBlock... safari/contentBlocking.js:168: setContentBlockerHandler); It seems somewhat dirty to me to register the same function as success and error callback, and then distinguish the case by checking whether an error argument is given inside the function. Wouldn't it be cleaner to use a different functions as success/error callbacks? 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.j... safari/ext/common.js:20: window.addEventListener("load", () => On 2017/08/14 17:23:49, Manish Jethani wrote: > We have to do this in the load handler because the classes haven't been defined > yet. This seems to be the wrong place to do this. If you patch RegExpFilter.typeMap in lib/requestBlocker.js, the event listener wouldn't be necessary.
Patch Set 3: Split out setContentBlocker handlers into then and catch, move patch to lib/requestBlocker.js https://codereview.adblockplus.org/29503587/diff/29515595/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29515595/safari/contentBlock... safari/contentBlocking.js:168: setContentBlockerHandler); On 2017/08/15 15:25:01, Sebastian Noack wrote: > It seems somewhat dirty to me to register the same function as success and error > callback, and then distinguish the case by checking whether an error argument is > given inside the function. Wouldn't it be cleaner to use a different functions > as success/error callbacks? Done (but see comments). 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.j... safari/ext/common.js:20: window.addEventListener("load", () => On 2017/08/15 15:25:01, Sebastian Noack wrote: > On 2017/08/14 17:23:49, Manish Jethani wrote: > > We have to do this in the load handler because the classes haven't been > defined > > yet. > > This seems to be the wrong place to do this. If you patch RegExpFilter.typeMap > in lib/requestBlocker.js, the event listener wouldn't be necessary. This seems to do the trick. Done. https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlock... safari/contentBlocking.js:136: if (error instanceof Error) The code was ignoring errors that weren't instances of Error for some reason. Maybe Safari 9 throws an error that is a string, but isn't really an error (operation successful). I'm leaving this here for now, but maybe we should remove it and let any bugs get discovered in testing. https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlock... safari/contentBlocking.js:170: afterContentBlockingFinished = null; Code that should run regardless of success/failure is here in a separate then handler at the end.
https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlock... safari/contentBlocking.js:136: if (error instanceof Error) On 2017/08/16 10:23:02, Manish Jethani wrote: > The code was ignoring errors that weren't instances of Error for some reason. > Maybe Safari 9 throws an error that is a string, but isn't really an error > (operation successful). I'm leaving this here for now, but maybe we should > remove it and let any bugs get discovered in testing. It seems, it would be simpler, if you move that logic to where you reject/resolve the Promise in the first place. https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlock... safari/contentBlocking.js:174: .then(active => I wonder if this logic, couldn't just go into the success callback above?
Patch Set 4: Merge then handlers https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlock... safari/contentBlocking.js:136: if (error instanceof Error) On 2017/08/16 10:30:34, Sebastian Noack wrote: > On 2017/08/16 10:23:02, Manish Jethani wrote: > > The code was ignoring errors that weren't instances of Error for some reason. > > Maybe Safari 9 throws an error that is a string, but isn't really an error > > (operation successful). I'm leaving this here for now, but maybe we should > > remove it and let any bugs get discovered in testing. > > It seems, it would be simpler, if you move that logic to where you > reject/resolve the Promise in the first place. Done. https://codereview.adblockplus.org/29503587/diff/29517561/safari/contentBlock... safari/contentBlocking.js:174: .then(active => On 2017/08/16 10:30:34, Sebastian Noack wrote: > I wonder if this logic, couldn't just go into the success callback above? You're right, done. Note that we have to add a catch handler at the end now because the then handler before it could throw an error, possibly.
LGTM. But I'd like Dave to review these changes as well.
Patch Set 5: Don't wrap setContentBlocker call into another promise I just realized that there's no need to wrap the setContentBlocker call into another promise (.catch(reject) is usually a good clue something like this is going on).
On 2017/08/16 11:34:55, Manish Jethani wrote: > I just realized that there's no need to wrap the setContentBlocker call into > another promise (.catch(reject) is usually a good clue something like this is > going on). Agreed, this is better. Still LGTM.
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/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29517577/safari/contentBlock... safari/contentBlocking.js:58: let completePromise = error => Nit: Since this is a named function maybe just use the `function completePromise(error)` syntax?
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 = adblockplusui hg:53626d2055a6 git:4ce9666 On 2017/08/21 12:30:12, kzar wrote: > Nit: Unrelated change? That was just vi inserting a newline at the end of the file. Fixed. https://codereview.adblockplus.org/29503587/diff/29517577/safari/contentBlock... File safari/contentBlocking.js (right): https://codereview.adblockplus.org/29503587/diff/29517577/safari/contentBlock... safari/contentBlocking.js:58: let completePromise = error => On 2017/08/21 12:30:12, kzar wrote: > Nit: Since this is a named function maybe just use the `function > completePromise(error)` syntax? Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
