Issue 29865587: Issue 6843 - Log snippet filter hits (Closed)
|
Created:
Aug. 27, 2018, 3:45 a.m. by hub Modified:
Aug. 28, 2018, 7:41 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 6843 - Log snippet filter hits
Patch Set 1 #
Total comments: 9
Patch Set 2 : Rebased and addressed comments #Patch Set 3 : Put the actual revision #
Total comments: 6
Patch Set 4 : Fixed nits #
Total comments: 7
Patch Set 5 : fixed wrapping #
Total comments: 2
Patch Set 6 : rebased on top of issue 6889 #
MessagesTotal messages: 16
This need the patch in adblockpluscore found at https://codereview.adblockplus.org/29865583/ to work (hence the dependency issue)
https://codereview.adblockplus.org/29865587/diff/29865588/dependencies File dependencies (right): https://codereview.adblockplus.org/29865587/diff/29865588/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:d1b1f5bac888 git:e752ad4 This will need to be updated when the dependency lands.
https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... lib/contentFiltering.js:250: docDomain: hostname, Nit: If you rename the variable "hostname" to "docDomain", you can use the object creation shorthand here, and consequently this code also wraps nicer: executeScript(filter.script, sender.page.id, sender.frame.id, {docDomain, url: sender.frame.url.href, filter});
https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... lib/contentFiltering.js:196: .then(() => Ideally this would take the form `.then(() => {}, error => {})` rather than `.then(() => {}).catch(error => {})` since we're only specifically interested in the errors coming from the call itself rather than any subsequent handlers. https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... lib/contentFiltering.js:198: let {filter} = request; We could read out `request.url` and `request.docDomain` right here as well. https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... lib/contentFiltering.js:227: } I think it would be cleaner to keep executeScript limited to its original purpose, which is to call tabs.executeScript with some boilerplate. In that case, we can return the promise returned by tabs.executeScript and let the caller add a `then` handler to log the hit (perhaps in a separate `logSnippetFilterHit` function). The catch block here would also then have to return a rejected promise (`return Promise.reject(error)`).
Updated patch https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... lib/contentFiltering.js:196: .then(() => On 2018/08/27 05:47:36, Manish Jethani wrote: > Ideally this would take the form `.then(() => {}, error => {})` rather than > `.then(() => {}).catch(error => {})` since we're only specifically interested in > the errors coming from the call itself rather than any subsequent handlers. this is obsolete now. https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... lib/contentFiltering.js:198: let {filter} = request; On 2018/08/27 05:47:36, Manish Jethani wrote: > We could read out `request.url` and `request.docDomain` right here as well. this is obsolete now. https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... lib/contentFiltering.js:227: } On 2018/08/27 05:47:36, Manish Jethani wrote: > I think it would be cleaner to keep executeScript limited to its original > purpose, which is to call tabs.executeScript with some boilerplate. In that > case, we can return the promise returned by tabs.executeScript and let the > caller add a `then` handler to log the hit (perhaps in a separate > `logSnippetFilterHit` function). The catch block here would also then have to > return a rejected promise (`return Promise.reject(error)`). Done. https://codereview.adblockplus.org/29865587/diff/29865588/lib/contentFilterin... lib/contentFiltering.js:250: docDomain: hostname, On 2018/08/27 04:51:35, Sebastian Noack wrote: > Nit: If you rename the variable "hostname" to "docDomain", you can use the > object creation shorthand here, and consequently this code also wraps nicer: > > executeScript(filter.script, sender.page.id, sender.frame.id, > {docDomain, url: sender.frame.url.href, filter}); Done.
https://codereview.adblockplus.org/29865587/diff/29866572/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29865587/diff/29866572/lib/contentFilterin... lib/contentFiltering.js:214: // tabs.insertCSS for why we catch and simply reject the promise. I think this would read better as "... why we catch any errors here and simply return a rejected promise." https://codereview.adblockplus.org/29865587/diff/29866572/lib/contentFilterin... lib/contentFiltering.js:236: for (let filter of Snippets.getFiltersForDomain(docDomain)) Nit: As a convention we have been adding braces around the block if it spans multiple lines (even if it's a single JS statement), maybe we should do that here as well. https://codereview.adblockplus.org/29865587/diff/29866572/lib/contentFilterin... lib/contentFiltering.js:245: url: sender.frame.url.href, type: "SNIPPET", Nit: When an object literal spans multiple lines, we typically place each property on its own line.
updated patch https://codereview.adblockplus.org/29865587/diff/29866572/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29865587/diff/29866572/lib/contentFilterin... lib/contentFiltering.js:214: // tabs.insertCSS for why we catch and simply reject the promise. On 2018/08/27 15:40:49, Manish Jethani wrote: > I think this would read better as "... why we catch any errors here and simply > return a rejected promise." Done. https://codereview.adblockplus.org/29865587/diff/29866572/lib/contentFilterin... lib/contentFiltering.js:236: for (let filter of Snippets.getFiltersForDomain(docDomain)) On 2018/08/27 15:40:49, Manish Jethani wrote: > Nit: As a convention we have been adding braces around the block if it spans > multiple lines (even if it's a single JS statement), maybe we should do that > here as well. Done. https://codereview.adblockplus.org/29865587/diff/29866572/lib/contentFilterin... lib/contentFiltering.js:245: url: sender.frame.url.href, type: "SNIPPET", On 2018/08/27 15:40:49, Manish Jethani wrote: > Nit: When an object literal spans multiple lines, we typically place each > property on its own line. Done.
https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... lib/contentFiltering.js:216: return Promise.reject(error); It appears I'm missing something here. But what is the difference between "p" and "p.catch(e => Promise.reject(e))"? https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... lib/contentFiltering.js:240: .then(() => Nit: Wrapping here appears unnecessary.
https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... lib/contentFiltering.js:216: return Promise.reject(error); On 2018/08/27 17:37:34, Sebastian Noack wrote: > It appears I'm missing something here. But what is the difference > between "p" and "p.catch(e => Promise.reject(e))"? Promise.catch(f) with call Promise.then(undefined, f) under the hood (this is the MDN documentation) However, here, we are in a plain exception catch(), so we don't have a promise. https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... lib/contentFiltering.js:240: .then(() => On 2018/08/27 17:37:34, Sebastian Noack wrote: > Nit: Wrapping here appears unnecessary. without wrapping the `>` reach column 80 so I felt it was better this way. done
https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... lib/contentFiltering.js:216: return Promise.reject(error); On 2018/08/27 17:37:34, Sebastian Noack wrote: > It appears I'm missing something here. But what is the difference > between "p" and "p.catch(e => Promise.reject(e))"? If the call to `tabs.executeScript` throws, the function won't return a promise, it'll return undefined; we need to return a "thenable" here (and it seems appropriate to return a rejected promise here as it makes it consistent between Firefox and Chrome, see comment in `addStyleSheet` for the background).
https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... lib/contentFiltering.js:216: return Promise.reject(error); On 2018/08/27 20:07:54, Manish Jethani wrote: > On 2018/08/27 17:37:34, Sebastian Noack wrote: > > It appears I'm missing something here. But what is the difference > > between "p" and "p.catch(e => Promise.reject(e))"? > > If the call to `tabs.executeScript` throws, the function won't return a promise, > it'll return undefined; we need to return a "thenable" here (and it seems > appropriate to return a rejected promise here as it makes it consistent between > Firefox and Chrome, see comment in `addStyleSheet` for the background). Acknowledged. I misread the code and thought we are in catch() call of a promise, not catch-block here. https://codereview.adblockplus.org/29865587/diff/29866587/lib/contentFilterin... lib/contentFiltering.js:240: .then(() => On 2018/08/27 19:45:24, hub wrote: > On 2018/08/27 17:37:34, Sebastian Noack wrote: > > Nit: Wrapping here appears unnecessary. > > without wrapping the `>` reach column 80 so I felt it was better this way. > > done Yes, the line may have up to 80 characters, not more, exactly 80 is fine.
https://codereview.adblockplus.org/29865587/diff/29866598/dependencies File dependencies (right): https://codereview.adblockplus.org/29865587/diff/29866598/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:6ad6dd7c2ff1 git:b4a3d60 About this, see my comment here: https://issues.adblockplus.org/ticket/6843#comment:12 We need to make a change in lib/whitelisting.js as well. Maybe we have to do this in two separate steps, I'm not sure.
https://codereview.adblockplus.org/29865587/diff/29866598/dependencies File dependencies (right): https://codereview.adblockplus.org/29865587/diff/29866598/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:6ad6dd7c2ff1 git:b4a3d60 On 2018/08/27 20:40:59, Manish Jethani wrote: > About this, see my comment here: > > https://issues.adblockplus.org/ticket/6843#comment:12 > > We need to make a change in lib/whitelisting.js as well. Maybe we have to do > this in two separate steps, I'm not sure. Filed https://issues.adblockplus.org/ticket/6889
Now rebased on top of https://codereview.adblockplus.org/29867599/
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
