Issue 29418679: Issue 5042 - Adds handling for requests which are not associated with browser tab (Closed)
|
Created:
April 20, 2017, 9:39 p.m. by Jon Sonesen Modified:
June 2, 2017, 9:33 a.m. Reviewers:
Sebastian Noack CC:
kzar, Thomas Greiner Visibility:
Public. |
DescriptionIssue 5042 - Adds handling for requests which are not associated with browser tab
Patch Set 1 : #
Total comments: 16
Patch Set 2 : address errors, style, new map object for panels #
Total comments: 6
Patch Set 3 : address nit, redundancies #Patch Set 4 : rebase #
Total comments: 15
Patch Set 5 : addresses comments, nits, redundancies, white space #
Total comments: 8
Patch Set 6 : fix whitespace, add comment abut null page, remove redundancies #
MessagesTotal messages: 16
https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js#o... ext/background.js:567: // related to a tab or requests loading a top-level document, This comment needs to be adapted too. https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js#o... ext/background.js:586: let {frameId, type} = details; If you remove the variable declaration here, the type == "sub_frame" branch below, will define frameId and type as global variables, which then get shadowed when you declare these variables below. https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js#n... ext/background.js:596: // However, since tabId's are assumed to be unique elsewhere in the code IMO, the second sentence is rather confusing, and not necessary here. https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js#n... ext/background.js:611: if (results.indexOf(false) != -1) Please use results.includes(false), which is equivalent to, but more readable than results.indexOf(false) != -1. You might also be able to get rid of the temporary variable then: if (ext.webRequest.onBeforeRequest._dispatch(url, type.toUpperCase(), page, frame).includes(false)) https://codereview.adblockplus.org/29418679/diff/29418690/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29418679/diff/29418690/lib/devtools.js#new... lib/devtools.js:139: let request = {url, type, docDomain, thirdParty, sitekey, specificOnly}; Perhaps, we should bail if there are no panels, before we create new objects and do other stuff, to make sure to keep the overhead as small as possible when the devtools panel isn't used. https://codereview.adblockplus.org/29418679/diff/29418690/lib/devtools.js#new... lib/devtools.js:147: for (let panelId in panels) Some of the code added here, and in this file in general, could be simplified by turning panels into a Map object, which is also in line with our coding practices, but back when this code was originally written, the Map object wasn't supported yet in Chrome. This might be a good opportunity to clean this up. https://codereview.adblockplus.org/29418679/diff/29418690/lib/devtools.js#new... lib/devtools.js:148: addRecord(panels[panelId], request, filter); getActivePanel(), called when logging for a specific page, is excluding panels of pages that are reloading, we should do the same here. https://codereview.adblockplus.org/29418679/diff/29418690/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29418679/diff/29418690/lib/requestBlocker.... lib/requestBlocker.js:55: let docDomain = getDecodedHostname(url); Is there a reason you derive the parent document's domain from the request URL? Note that these makes requests that don't have an associated tab indistinguishable from those that that have (in the devtools panel). Also it seems incorrect, as *$domain=example.com is supposed to match subrequests from documents served from example.com, not requests that are served from example.com in documents served from other domains.
Thanks for the comments Sebastian. Sorry for the slight delay here, was not feeling well on Friday and had to take a half day to rest. https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js#o... ext/background.js:567: // related to a tab or requests loading a top-level document, On 2017/04/21 08:39:53, Sebastian Noack wrote: > This comment needs to be adapted too. Acknowledged. https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js#o... ext/background.js:586: let {frameId, type} = details; On 2017/04/21 08:39:53, Sebastian Noack wrote: > If you remove the variable declaration here, the type == "sub_frame" branch > below, will define frameId and type as global variables, which then get shadowed > when you declare these variables below. Acknowledged. https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js#n... ext/background.js:596: // However, since tabId's are assumed to be unique elsewhere in the code On 2017/04/21 08:39:53, Sebastian Noack wrote: > IMO, the second sentence is rather confusing, and not necessary here. Acknowledged. https://codereview.adblockplus.org/29418679/diff/29418690/ext/background.js#n... ext/background.js:611: if (results.indexOf(false) != -1) On 2017/04/21 08:39:53, Sebastian Noack wrote: > Please use results.includes(false), which is equivalent to, but more readable > than results.indexOf(false) != -1. You might also be able to get rid of the > temporary variable then: > > if (ext.webRequest.onBeforeRequest._dispatch(url, type.toUpperCase(), > page, frame).includes(false)) Acknowledged. https://codereview.adblockplus.org/29418679/diff/29418690/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29418679/diff/29418690/lib/devtools.js#new... lib/devtools.js:139: let request = {url, type, docDomain, thirdParty, sitekey, specificOnly}; On 2017/04/21 08:39:53, Sebastian Noack wrote: > Perhaps, we should bail if there are no panels, before we create new objects and > do other stuff, to make sure to keep the overhead as small as possible when the > devtools panel isn't used. Acknowledged. https://codereview.adblockplus.org/29418679/diff/29418690/lib/devtools.js#new... lib/devtools.js:147: for (let panelId in panels) On 2017/04/21 08:39:53, Sebastian Noack wrote: > Some of the code added here, and in this file in general, could be simplified by > turning panels into a Map object, which is also in line with our coding > practices, but back when this code was originally written, the Map object wasn't > supported yet in Chrome. This might be a good opportunity to clean this up. Awesome, I agree here. I am wondering though, should I adapt the whole file for this change or just convert to a map here in the function? I am happy to adapt the initialization of the panels and the rest of the code path but perhaps it is better to then open a separate issue? https://codereview.adblockplus.org/29418679/diff/29418690/lib/devtools.js#new... lib/devtools.js:148: addRecord(panels[panelId], request, filter); On 2017/04/21 08:39:53, Sebastian Noack wrote: > getActivePanel(), called when logging for a specific page, is excluding panels > of pages that are reloading, we should do the same here. Acknowledged. https://codereview.adblockplus.org/29418679/diff/29418690/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29418679/diff/29418690/lib/requestBlocker.... lib/requestBlocker.js:55: let docDomain = getDecodedHostname(url); On 2017/04/21 08:39:53, Sebastian Noack wrote: > Is there a reason you derive the parent document's domain from the request URL? > Note that these makes requests that don't have an associated tab > indistinguishable from those that that have (in the devtools panel). Also it > seems incorrect, as *$domain=example.com is supposed to match subrequests from > documents served from http://example.com, not requests that are served from http://example.com > in documents served from other domains. I see, I misunderstood what I was doing here. I just saw that the docDomain was being defined before but was not sure how to fetch it without the page variable and saw that the docDomain can also be obtained via the getDecodedHostname function. So what is the best option here? Should I just set the domain as the URL?
Here you go, perhaps there are other changes to make due to the new map object for the panels which can further utilize maps to simplify code. Perhaps finding a way to use foreach in the log_requests code path when the request is not associated with a tab.
I didn't have a look at lib/devtools.js yet. Manish has a patch under review that introduces Maps all over the place, which we probably should land first, and then rebases this one on. https://codereview.adblockplus.org/29418679/diff/29424705/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29418679/diff/29424705/lib/requestBlocker.... lib/requestBlocker.js:28: const {stringifyURL, getDecodedHostname, It seems the newly imported function (i.e. getDecodedHostname) isn't used anywhere. https://codereview.adblockplus.org/29418679/diff/29424705/lib/requestBlocker.... lib/requestBlocker.js:65: return true; Nit: I'd always add a blank line when returning before the end of the function. https://codereview.adblockplus.org/29418679/diff/29424705/lib/requestBlocker.... lib/requestBlocker.js:68: isThirdParty(url, docDomain); As this code stands, this call is redundant. Did you forget to assign the result to a variable?
https://codereview.adblockplus.org/29418679/diff/29424705/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29418679/diff/29424705/lib/requestBlocker.... lib/requestBlocker.js:28: const {stringifyURL, getDecodedHostname, On 2017/04/29 23:07:59, Sebastian Noack wrote: > It seems the newly imported function (i.e. getDecodedHostname) isn't used > anywhere. Done. https://codereview.adblockplus.org/29418679/diff/29424705/lib/requestBlocker.... lib/requestBlocker.js:65: return true; On 2017/04/29 23:08:00, Sebastian Noack wrote: > Nit: I'd always add a blank line when returning before the end of the function. Done. https://codereview.adblockplus.org/29418679/diff/29424705/lib/requestBlocker.... lib/requestBlocker.js:68: isThirdParty(url, docDomain); On 2017/04/29 23:07:59, Sebastian Noack wrote: > As this code stands, this call is redundant. Did you forget to assign the result > to a variable? Yeah that's what happened there. Thanks. Done.
Can you please rebase? Thanks.
On 2017/05/22 08:18:51, Sebastian Noack wrote: > Can you please rebase? Thanks. Done.
https://codereview.adblockplus.org/29418679/diff/29451564/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29418679/diff/29451564/ext/background.js#n... ext/background.js:592: type = "SUBDOCUMENT"; We no longer map "sub_frame" to "SUBDOCUMENT" here. https://codereview.adblockplus.org/29418679/diff/29451564/ext/background.js#n... ext/background.js:606: url, type.toUpperCase(), page, frame).includes(false)) We no longer convert the type to upper case here. https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js File lib/devtools.js (left): https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js#old... lib/devtools.js:126: * @param {?boolean} specificOnly Whether generic filters should be ignored Please replace {Page} with {?Page} above, in order to indicate that this argument can be null, and update the description for the page argument to consider that scenario. https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js#new... lib/devtools.js:39: if (panel && !panel.reload && !panel.reloading) The if/else is redundant, You can just return the expression: return !!panel && !panel.reload && !panel.reloading; https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js#new... lib/devtools.js:153: addRecord(panel, request, filter); This would be somewhat less efficient, in case you have a lot of devtools panels open, but simplifies the code and minimizes the diverge a bit: for (let [tabId, panel] of panels) if ((!page || page.id == tabId) && isActivePanel(panel)) addRecord(panel, request, filter); What do you think? https://codereview.adblockplus.org/29418679/diff/29451564/lib/requestBlocker.js File lib/requestBlocker.js (left): https://codereview.adblockplus.org/29418679/diff/29451564/lib/requestBlocker.... lib/requestBlocker.js:82: ); Nit: You added one level off indentation to the code above, but not to this closing parentheses. Alternatively, this function call could be indented like below, but up to you: specificOnly = !!checkWhitelisted(page, frame, RegExpFilter.typeMap.GENERICBLOCK); https://codereview.adblockplus.org/29418679/diff/29451564/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29418679/diff/29451564/lib/requestBlocker.... lib/requestBlocker.js:75: let specificOnly = false; Nit: The blank line above looks redundant, but instead I think a blank line above the if-statement would improve readability.
Thanks for catching those redundancies :) https://codereview.adblockplus.org/29418679/diff/29451564/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29418679/diff/29451564/ext/background.js#n... ext/background.js:592: type = "SUBDOCUMENT"; On 2017/05/30 11:10:48, Sebastian Noack wrote: > We no longer map "sub_frame" to "SUBDOCUMENT" here. Acknowledged. https://codereview.adblockplus.org/29418679/diff/29451564/ext/background.js#n... ext/background.js:606: url, type.toUpperCase(), page, frame).includes(false)) On 2017/05/30 11:10:48, Sebastian Noack wrote: > We no longer convert the type to upper case here. Acknowledged. https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js File lib/devtools.js (left): https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js#old... lib/devtools.js:126: * @param {?boolean} specificOnly Whether generic filters should be ignored On 2017/05/30 11:10:48, Sebastian Noack wrote: > Please replace {Page} with {?Page} above, in order to indicate that this > argument can be null, and update the description for the page argument to > consider that scenario. Sure, does this mean it should be moved down to where the other optional parameters are defined as well? https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js#new... lib/devtools.js:39: if (panel && !panel.reload && !panel.reloading) On 2017/05/30 11:10:49, Sebastian Noack wrote: > The if/else is redundant, You can just return the expression: > > return !!panel && !panel.reload && !panel.reloading; Acknowledged. https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js#new... lib/devtools.js:153: addRecord(panel, request, filter); On 2017/05/30 11:10:49, Sebastian Noack wrote: > This would be somewhat less efficient, in case you have a lot of devtools panels > open, but simplifies the code and minimizes the diverge a bit: > > for (let [tabId, panel] of panels) > if ((!page || page.id == tabId) && isActivePanel(panel)) > addRecord(panel, request, filter); > > What do you think? Yeah I like this, will do. Thanks. https://codereview.adblockplus.org/29418679/diff/29451564/lib/requestBlocker.js File lib/requestBlocker.js (left): https://codereview.adblockplus.org/29418679/diff/29451564/lib/requestBlocker.... lib/requestBlocker.js:82: ); On 2017/05/30 11:10:49, Sebastian Noack wrote: > Nit: You added one level off indentation to the code above, but not to this > closing parentheses. > > Alternatively, this function call could be indented like below, but up to you: > > specificOnly = !!checkWhitelisted(page, frame, > RegExpFilter.typeMap.GENERICBLOCK); I will do it as you suggest. https://codereview.adblockplus.org/29418679/diff/29451564/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29418679/diff/29451564/lib/requestBlocker.... lib/requestBlocker.js:75: let specificOnly = false; On 2017/05/30 11:10:49, Sebastian Noack wrote: > Nit: The blank line above looks redundant, but instead I think a blank line > above the if-statement would improve readability. Acknowledged.
https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js File lib/devtools.js (left): https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js#old... lib/devtools.js:126: * @param {?boolean} specificOnly Whether generic filters should be ignored On 2017/05/31 08:28:17, Jon Sonesen wrote: > On 2017/05/30 11:10:48, Sebastian Noack wrote: > > Please replace {Page} with {?Page} above, in order to indicate that this > > argument can be null, and update the description for the page argument to > > consider that scenario. > > Sure, does this mean it should be moved down to where the other optional > parameters are defined as well? No. The arguments should be listed in the order they are passed to the function.
On 2017/05/31 08:35:01, Sebastian Noack wrote: > https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js > File lib/devtools.js (left): > > https://codereview.adblockplus.org/29418679/diff/29451564/lib/devtools.js#old... > lib/devtools.js:126: * @param {?boolean} specificOnly Whether generic filters > should be ignored > On 2017/05/31 08:28:17, Jon Sonesen wrote: > > On 2017/05/30 11:10:48, Sebastian Noack wrote: > > > Please replace {Page} with {?Page} above, in order to indicate that this > > > argument can be null, and update the description for the page argument to > > > consider that scenario. > > > > Sure, does this mean it should be moved down to where the other optional > > parameters are defined as well? > > No. The arguments should be listed in the order they are passed to the function. Ack.
https://codereview.adblockplus.org/29418679/diff/29452565/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29418679/diff/29452565/lib/devtools.js#new... lib/devtools.js:124: * @param {?Page} page The page the request occured on Please describe the scenario in which Page is null: The page the request occurred on, or null if the request isn't associated with a particular page https://codereview.adblockplus.org/29418679/diff/29452565/lib/devtools.js#new... lib/devtools.js:149: for (let [tabId, panel] of panels) The logic above, handling the case of page being non-null, is redundant. https://codereview.adblockplus.org/29418679/diff/29452565/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29418679/diff/29452565/lib/requestBlocker.... lib/requestBlocker.js:78: if (frame && page) Nit: As mentioned before, I'd add a blank line above, in order to separate the declarations from the condition. https://codereview.adblockplus.org/29418679/diff/29452565/lib/requestBlocker.... lib/requestBlocker.js:89: Nit: One blank line is sufficient here.
Thanks for catching those things, forgetting to remove the redundant logic was silly, the white space too. I'll be sure to take a bit more time after receiving comments before I upload so I don't miss that stuff. XD https://codereview.adblockplus.org/29418679/diff/29452565/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29418679/diff/29452565/lib/devtools.js#new... lib/devtools.js:124: * @param {?Page} page The page the request occured on On 2017/06/01 17:32:21, Sebastian Noack wrote: > Please describe the scenario in which Page is null: > > The page the request occurred on, or null if the > request isn't associated with a particular page Acknowledged. https://codereview.adblockplus.org/29418679/diff/29452565/lib/devtools.js#new... lib/devtools.js:149: for (let [tabId, panel] of panels) On 2017/06/01 17:32:21, Sebastian Noack wrote: > The logic above, handling the case of page being non-null, is redundant. Acknowledged. https://codereview.adblockplus.org/29418679/diff/29452565/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29418679/diff/29452565/lib/requestBlocker.... lib/requestBlocker.js:78: if (frame && page) On 2017/06/01 17:32:21, Sebastian Noack wrote: > Nit: As mentioned before, I'd add a blank line above, in order to separate the > declarations from the condition. Acknowledged. https://codereview.adblockplus.org/29418679/diff/29452565/lib/requestBlocker.... lib/requestBlocker.js:89: On 2017/06/01 17:32:21, Sebastian Noack wrote: > Nit: One blank line is sufficient here. Acknowledged.
LGTM
On 2017/06/02 09:16:22, Sebastian Noack wrote: > LGTM Pushed. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
