Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(698)

Issue 29418679: Issue 5042 - Adds handling for requests which are not associated with browser tab

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week ago by Jon Sonesen
Modified:
3 days, 17 hours ago
Reviewers:
Sebastian Noack
CC:
kzar, Thomas Greiner
Visibility:
Public.

Description

Issue 5042 - Adds handling for requests which are not associated with browser tab

Patch Set 1 : #

Total comments: 16
Unified diffs Side-by-side diffs Delta from patch set Stats (+39 lines, -19 lines) Patch
M ext/background.js View 1 chunk +20 lines, -10 lines 8 comments Download
M lib/devtools.js View 1 chunk +8 lines, -4 lines 6 comments Download
M lib/requestBlocker.js View 2 chunks +11 lines, -5 lines 2 comments Download

Messages

Total messages: 3
Jon Sonesen
1 week ago (2017-04-20 21:39:43 UTC) #1
Sebastian Noack
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#oldcode567 ext/background.js:567: // related to a tab or requests loading a ...
6 days, 17 hours ago (2017-04-21 08:39:53 UTC) #2
Jon Sonesen
3 days, 17 hours ago (2017-04-24 08:40:56 UTC) #3
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?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5