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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months ago by Jon Sonesen
Modified:
2 months, 3 weeks 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

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -26 lines) Patch
M ext/background.js View 1 2 3 4 2 chunks +12 lines, -9 lines 0 comments Download
M lib/devtools.js View 1 2 3 4 5 2 chunks +15 lines, -8 lines 0 comments Download
M lib/requestBlocker.js View 1 2 3 4 5 1 chunk +16 lines, -9 lines 0 comments Download

Messages

Total messages: 16
Jon Sonesen
4 months 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 ...
4 months ago (2017-04-21 08:39:53 UTC) #2
Jon Sonesen
Thanks for the comments Sebastian. Sorry for the slight delay here, was not feeling well ...
4 months ago (2017-04-24 08:40:56 UTC) #3
Jon Sonesen
Here you go, perhaps there are other changes to make due to the new map ...
3 months, 3 weeks ago (2017-04-28 10:54:06 UTC) #4
Sebastian Noack
I didn't have a look at lib/devtools.js yet. Manish has a patch under review that ...
3 months, 3 weeks ago (2017-04-29 23:08:00 UTC) #5
Jon Sonesen
https://codereview.adblockplus.org/29418679/diff/29424705/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29418679/diff/29424705/lib/requestBlocker.js#newcode28 lib/requestBlocker.js:28: const {stringifyURL, getDecodedHostname, On 2017/04/29 23:07:59, Sebastian Noack wrote: ...
3 months, 2 weeks ago (2017-05-04 10:26:06 UTC) #6
Sebastian Noack
Can you please rebase? Thanks.
3 months ago (2017-05-22 08:18:51 UTC) #7
Jon Sonesen
On 2017/05/22 08:18:51, Sebastian Noack wrote: > Can you please rebase? Thanks. Done.
2 months, 3 weeks ago (2017-05-30 09:05:02 UTC) #8
Sebastian Noack
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#newcode592 ext/background.js:592: type = "SUBDOCUMENT"; We no longer map "sub_frame" to ...
2 months, 3 weeks ago (2017-05-30 11:10:50 UTC) #9
Jon Sonesen
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#newcode592 ext/background.js:592: type = "SUBDOCUMENT"; ...
2 months, 3 weeks ago (2017-05-31 08:28:18 UTC) #10
Sebastian Noack
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#oldcode126 lib/devtools.js:126: * @param {?boolean} specificOnly Whether generic filters should be ...
2 months, 3 weeks ago (2017-05-31 08:35:01 UTC) #11
Jon Sonesen
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#oldcode126 ...
2 months, 3 weeks ago (2017-05-31 08:44:33 UTC) #12
Sebastian Noack
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#newcode124 lib/devtools.js:124: * @param {?Page} page The page the request occured ...
2 months, 3 weeks ago (2017-06-01 17:32:21 UTC) #13
Jon Sonesen
Thanks for catching those things, forgetting to remove the redundant logic was silly, the white ...
2 months, 3 weeks ago (2017-06-02 05:02:58 UTC) #14
Sebastian Noack
LGTM
2 months, 3 weeks ago (2017-06-02 09:16:22 UTC) #15
Jon Sonesen
2 months, 3 weeks ago (2017-06-02 09:32:52 UTC) #16
On 2017/06/02 09:16:22, Sebastian Noack wrote:
> LGTM

Pushed.
Sign in to reply to this message.

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