Issue 29418659: Issue 5130 - Changes listener to register <all_urls> and filter non ws http requests (Closed)
|
Created:
April 20, 2017, 7:18 p.m. by Jon Sonesen Modified:
April 20, 2017, 8:16 p.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 5130 - Changes listener to register <all_urls> and filter non ws http requests
Patch Set 1 #
Total comments: 12
Patch Set 2 : remove regex, style changes, remove redundant object creations #
Total comments: 4
Patch Set 3 : pick nit, move changes to below high level bail out #MessagesTotal messages: 8
https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:567: // 58 added support for webSocket connections to the webRequests API this comment should likely be improved. perhaps we can decide on that here
https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:567: // 58 added support for webSocket connections to the webRequests API On 2017/04/20 19:19:58, Jon Sonesen wrote: > this comment should likely be improved. perhaps we can decide on that here "Filter out requests from non web protocols. Ideally, we'd explicitly specify the protocols we are interested in (i.e. http://, https://, ws:// and wss://) with the url patterns, given below, when adding this listener. But unfortunately, Chrome <=57 doesn't support the WebSocket protocol and is causing an error if it is given." https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:569: let url = new URL(details.url); We are now creating two URL objects, one to validate the protocol here, and on we pass to ext.webRequest.onBeforeRequest._dispatch(). Please reuse the URL object which we create here below. https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:570: if (!blockURLs.exec(url.protocol)) Since the regular expression doesn't make the code here much simpler, I'd rather check for the 4 possible values explicitly (which is also faster): if (url.protocol == 'http:' || url.protocol == 'https:' || url.protocol == 'ws:' || url.protocol == 'wss:') https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:571: { We generally omit the braces if there is only one line in the block. https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:572: return {cancel: false}; For consistency with the other bailout condition below, and to avoid creating an unnecessary object, please omit the optional return value (which is equivalent to returning {cancel: false}).
https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:570: if (!blockURLs.exec(url.protocol)) On 2017/04/20 19:34:15, Sebastian Noack wrote: > Since the regular expression doesn't make the code here much simpler, I'd rather > check for the 4 possible values explicitly (which is also faster): > > if (url.protocol == 'http:' || url.protocol == 'https:' || > url.protocol == 'ws:' || url.protocol == 'wss:') I accidentally inverted the logic, should be of course: if (url.protocol != 'http:' && url.protocol != 'https:' && url.protocol != 'ws:' && url.protocol != 'wss:')
Thanks for the quick response :D https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:567: // 58 added support for webSocket connections to the webRequests API On 2017/04/20 19:34:15, Sebastian Noack wrote: > On 2017/04/20 19:19:58, Jon Sonesen wrote: > > this comment should likely be improved. perhaps we can decide on that here > > "Filter out requests from non web protocols. Ideally, we'd explicitly specify > the protocols we are interested in (i.e. http://, https://, ws:// and wss://) > with the url patterns, given below, when adding this listener. But > unfortunately, Chrome <=57 doesn't support the WebSocket protocol and is causing > an error if it is given." Acknowledged. Thank you, I think that is good. https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:569: let url = new URL(details.url); On 2017/04/20 19:34:15, Sebastian Noack wrote: > We are now creating two URL objects, one to validate the protocol here, and on > we pass to ext.webRequest.onBeforeRequest._dispatch(). Please reuse the URL > object which we create here below. Ah, sorry about that. https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:570: if (!blockURLs.exec(url.protocol)) On 2017/04/20 19:36:22, Sebastian Noack wrote: > On 2017/04/20 19:34:15, Sebastian Noack wrote: > > Since the regular expression doesn't make the code here much simpler, I'd > rather > > check for the 4 possible values explicitly (which is also faster): > > > > if (url.protocol == 'http:' || url.protocol == 'https:' || > > url.protocol == 'ws:' || url.protocol == 'wss:') > > I accidentally inverted the logic, should be of course: > > if (url.protocol != 'http:' && url.protocol != 'https:' && > url.protocol != 'ws:' && url.protocol != 'wss:') Yeah, happy to use this instead. https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:571: { On 2017/04/20 19:34:15, Sebastian Noack wrote: > We generally omit the braces if there is only one line in the block. Done. https://codereview.adblockplus.org/29418659/diff/29418660/ext/background.js#n... ext/background.js:572: return {cancel: false}; On 2017/04/20 19:34:15, Sebastian Noack wrote: > For consistency with the other bailout condition below, and to avoid creating an > unnecessary object, please omit the optional return value (which is equivalent > to returning {cancel: false}). Done.
https://codereview.adblockplus.org/29418659/diff/29418662/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29418659/diff/29418662/ext/background.js#n... ext/background.js:571: let url = new URL(details.url); Perhaps move this after the check below, so that we can skip parsing the URL for top-level documents which we ignore anyway. https://codereview.adblockplus.org/29418659/diff/29418662/ext/background.js#n... ext/background.js:597: url, Nit: Wrapping after each argument, while most arguments are not longer than a few characters anymore, seems unnecessary now: let results = ext.webRequest.onBeforeRequest._dispatch( url, type.toUpperCase(), new Page({id: details.tabId}), frame );
https://codereview.adblockplus.org/29418659/diff/29418662/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29418659/diff/29418662/ext/background.js#n... ext/background.js:571: let url = new URL(details.url); On 2017/04/20 19:59:48, Sebastian Noack wrote: > Perhaps move this after the check below, so that we can skip parsing the URL for > top-level documents which we ignore anyway. Done. https://codereview.adblockplus.org/29418659/diff/29418662/ext/background.js#n... ext/background.js:597: url, On 2017/04/20 19:59:48, Sebastian Noack wrote: > Nit: Wrapping after each argument, while most arguments are not longer than a > few characters anymore, seems unnecessary now: > > let results = ext.webRequest.onBeforeRequest._dispatch( > url, type.toUpperCase(), new Page({id: details.tabId}), frame > ); Done.
LGTM 
|
