Issue 29522656: Issue 5316 - Adds supported filter types export value to requestBlocker (Closed)
|
Created:
Aug. 21, 2017, 3:26 p.m. by Jon Sonesen Modified:
Sept. 27, 2017, 4:22 p.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 5316 - Adds supported filter types export value to requestBlocker
Patch Set 1 : #
Total comments: 9
Patch Set 2 : #
Total comments: 6
Patch Set 3 : add commments #
Total comments: 7
Patch Set 4 : add whitespace fix comment #Patch Set 5 : update dependency file #MessagesTotal messages: 10
https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.... lib/requestBlocker.js:56: // Expose supported resource types for devtools panel I think this comment could be improved but not sure if it is that important, since it seems clear what the code is doing.
https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.... lib/requestBlocker.js:56: // Expose supported resource types for devtools panel On 2017/08/21 15:33:07, Jon Sonesen wrote: > I think this comment could be improved but not sure if it is that important, > since it seems clear what the code is doing. Yeah, I'd rather just remove that comment. https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.... lib/requestBlocker.js:59: for (let type in chrome.webRequest.ResourceType) webRequest.ResourceType doesn't exist in Microsoft Edge. However, neither does the devtools panel, so the value of filterTypes is irrelevant there, but we still have to detect whether ResourceType exists in order to prevent this script from throwing an error on Microsoft Edge. https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.... lib/requestBlocker.js:60: yield resourceTypes.get(chrome.webRequest.ResourceType[type]) || "OTHER"; There are some filter types which we need to hard-code here, in addition to those which we can detect based on webRequest.ResourceType. These are: * WEBSOCKET (supported through a workaround if not in ResourceType) * WEBRTC (supported through a workaround) * POPUP (non-request type) * ELEMHIDE (non-request type)
https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.... lib/requestBlocker.js:56: // Expose supported resource types for devtools panel On 2017/08/22 09:47:28, Sebastian Noack wrote: > On 2017/08/21 15:33:07, Jon Sonesen wrote: > > I think this comment could be improved but not sure if it is that important, > > since it seems clear what the code is doing. > > Yeah, I'd rather just remove that comment. Acknowledged. https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.... lib/requestBlocker.js:59: for (let type in chrome.webRequest.ResourceType) On 2017/08/22 09:47:28, Sebastian Noack wrote: > webRequest.ResourceType doesn't exist in Microsoft Edge. However, neither does > the devtools panel, so the value of filterTypes is irrelevant there, but we > still have to detect whether ResourceType exists in order to prevent this script > from throwing an error on Microsoft Edge. Cool, can I just return if !(chrome.webRequest.ResourceType)? https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.... lib/requestBlocker.js:60: yield resourceTypes.get(chrome.webRequest.ResourceType[type]) || "OTHER"; On 2017/08/22 09:47:28, Sebastian Noack wrote: > There are some filter types which we need to hard-code here, in addition to > those which we can detect based on webRequest.ResourceType. These are: > > * WEBSOCKET (supported through a workaround if not in ResourceType) > * WEBRTC (supported through a workaround) > * POPUP (non-request type) > * ELEMHIDE (non-request type) Cool, does it make sense to just add yields with each type after the iteration?
https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.... lib/requestBlocker.js:59: for (let type in chrome.webRequest.ResourceType) On 2017/09/01 08:09:06, Jon Sonesen wrote: > On 2017/08/22 09:47:28, Sebastian Noack wrote: > > webRequest.ResourceType doesn't exist in Microsoft Edge. However, neither does > > the devtools panel, so the value of filterTypes is irrelevant there, but we > > still have to detect whether ResourceType exists in order to prevent this > script > > from throwing an error on Microsoft Edge. > > Cool, can I just return if !(chrome.webRequest.ResourceType)? Yes, that will do. https://codereview.adblockplus.org/29522656/diff/29522659/lib/requestBlocker.... lib/requestBlocker.js:60: yield resourceTypes.get(chrome.webRequest.ResourceType[type]) || "OTHER"; On 2017/09/01 08:09:06, Jon Sonesen wrote: > On 2017/08/22 09:47:28, Sebastian Noack wrote: > > There are some filter types which we need to hard-code here, in addition to > > those which we can detect based on webRequest.ResourceType. These are: > > > > * WEBSOCKET (supported through a workaround if not in ResourceType) > > * WEBRTC (supported through a workaround) > > * POPUP (non-request type) > > * ELEMHIDE (non-request type) > > Cool, does it make sense to just add yields with each type after the iteration? Yes, I think this is the simplest approach. https://codereview.adblockplus.org/29522656/diff/29536698/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29522656/diff/29536698/lib/requestBlocker.... lib/requestBlocker.js:58: if (!(chrome.webRequest.ResourceType)) Please add a comment like this: "Microsoft Edge does not have webRequest.ResourceType. However, since filterTypes is only used by the devtools panel which isn't supported either on Microsoft Edge, we can just bail out here." https://codereview.adblockplus.org/29522656/diff/29536698/lib/requestBlocker.... lib/requestBlocker.js:62: yield "WEBSOCKET"; Please add a comment like this: "WEBSOCKET and WEBRTC are supported through workarounds, even if the webRequest API is lacking support to block these kind of requests." https://codereview.adblockplus.org/29522656/diff/29536698/lib/requestBlocker.... lib/requestBlocker.js:64: yield "POPUP"; Please add a comment like this: "POPUP and ELEMHIDE filters aren't blocked on the request level but by other means, hence they don't have a corresponding value in webRequest.ResourceType."
https://codereview.adblockplus.org/29522656/diff/29536698/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29522656/diff/29536698/lib/requestBlocker.... lib/requestBlocker.js:58: if (!(chrome.webRequest.ResourceType)) On 2017/09/05 18:08:10, Sebastian Noack wrote: > Please add a comment like this: > > "Microsoft Edge does not have webRequest.ResourceType. However, since > filterTypes is only used by the devtools panel which isn't supported either on > Microsoft Edge, we can just bail out here." Acknowledged. https://codereview.adblockplus.org/29522656/diff/29536698/lib/requestBlocker.... lib/requestBlocker.js:62: yield "WEBSOCKET"; On 2017/09/05 18:08:11, Sebastian Noack wrote: > Please add a comment like this: > > "WEBSOCKET and WEBRTC are supported through workarounds, even if the webRequest > API is lacking support to block these kind of requests." Acknowledged. https://codereview.adblockplus.org/29522656/diff/29536698/lib/requestBlocker.... lib/requestBlocker.js:64: yield "POPUP"; On 2017/09/05 18:08:10, Sebastian Noack wrote: > Please add a comment like this: > > "POPUP and ELEMHIDE filters aren't blocked on the request level but by other > means, hence they don't have a corresponding value in webRequest.ResourceType." Acknowledged.
https://codereview.adblockplus.org/29522656/diff/29537632/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29522656/diff/29537632/lib/requestBlocker.... lib/requestBlocker.js:62: for (let type in chrome.webRequest.ResourceType) The code looks a little squashed. Perhaps add a blank line above this line? https://codereview.adblockplus.org/29522656/diff/29537632/lib/requestBlocker.... lib/requestBlocker.js:64: // WEBSOCKET and WEBRTC get addressed through workarounds, even if webRequest "webrequest" => "the webRequest API" https://codereview.adblockplus.org/29522656/diff/29537632/lib/requestBlocker.... lib/requestBlocker.js:64: // WEBSOCKET and WEBRTC get addressed through workarounds, even if webRequest Perhaps also add a blank line here? https://codereview.adblockplus.org/29522656/diff/29537632/lib/requestBlocker.... lib/requestBlocker.js:68: // POPUP and ELEMHIDE filters aren't blocked on the request level but by other Perhaps also add a blank line here?
https://codereview.adblockplus.org/29522656/diff/29537632/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29522656/diff/29537632/lib/requestBlocker.... lib/requestBlocker.js:62: for (let type in chrome.webRequest.ResourceType) On 2017/09/06 18:06:48, Sebastian Noack wrote: > The code looks a little squashed. Perhaps add a blank line above this line? Done. https://codereview.adblockplus.org/29522656/diff/29537632/lib/requestBlocker.... lib/requestBlocker.js:64: // WEBSOCKET and WEBRTC get addressed through workarounds, even if webRequest On 2017/09/06 18:06:48, Sebastian Noack wrote: > Perhaps also add a blank line here? Done. https://codereview.adblockplus.org/29522656/diff/29537632/lib/requestBlocker.... lib/requestBlocker.js:68: // POPUP and ELEMHIDE filters aren't blocked on the request level but by other On 2017/09/06 18:06:49, Sebastian Noack wrote: > Perhaps also add a blank line here? Done.
Looks good the me. Eventually we still have to update the adblockplusui dependency here, once the changes landed there.
LGTM. But note that the "master" bookmark is in code freeze for the upcoming release. So please land this change in the "next" bookmark instead! 
|
