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

Issue 29705719: Issue 6402 - Split filter hit / request logging out into own API (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 4 weeks ago by kzar
Modified:
4 months, 1 week ago
CC:
Thomas Greiner, saroyanm
Visibility:
Public.

Description

Issue 6402 - Split filter hit / request logging out into own API

Patch Set 1 #

Patch Set 2 : Fixed typo and exposed pageId for logged requests #

Patch Set 3 : Ensure devtools module is included in the bundle #

Total comments: 6

Patch Set 4 : Addressed Sebastian's feedback #

Total comments: 2

Patch Set 5 : Nightmarish rebase #

Total comments: 22

Patch Set 6 : Rebased #

Patch Set 7 : Addressed Manish's feedback #

Total comments: 19

Patch Set 8 : Addressed Sebastian's feedback #

Patch Set 9 : Another try at uploading the rebase diff #

Total comments: 10

Patch Set 10 : Addressed Manish's feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+199 lines, -140 lines) Patch
M include.preload.js View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M lib/csp.js View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -3 lines 0 comments Download
M lib/cssInjection.js View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M lib/devtools.js View 1 2 3 4 5 6 7 5 chunks +6 lines, -113 lines 0 comments Download
A lib/hitLogger.js View 1 2 3 4 5 6 7 8 9 1 chunk +163 lines, -0 lines 0 comments Download
M lib/popupBlocker.js View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -4 lines 0 comments Download
M lib/requestBlocker.js View 1 2 3 4 5 6 7 8 4 chunks +15 lines, -15 lines 0 comments Download
M lib/whitelisting.js View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M metadata.chrome View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 31
kzar
Patch Set 1
6 months, 4 weeks ago (2018-02-22 15:25:51 UTC) #1
kzar
Patch Set 2 : Fixed typo and exposed pageId for logged requests
6 months, 4 weeks ago (2018-02-22 15:57:29 UTC) #2
kzar
Patch Set 3 : Ensure devtools module is included in the bundle
6 months, 4 weeks ago (2018-02-22 16:30:32 UTC) #3
saroyanm
6 months, 1 week ago (2018-03-15 15:30:01 UTC) #4
saroyanm
6 months, 1 week ago (2018-03-15 15:30:36 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29705719/diff/29705741/lib/devLogger.js File lib/devLogger.js (left): https://codereview.adblockplus.org/29705719/diff/29705741/lib/devLogger.js#oldcode100 lib/devLogger.js:100: type: "add-record", This abstraction is specific to the devtools ...
6 months ago (2018-03-20 01:28:26 UTC) #6
kzar
Patch Set 4 : Addressed Sebastian's feedback Now depends on this adblockpluscore change https://codereview.adblockplus.org/29729594/ https://codereview.adblockplus.org/29705719/diff/29705741/lib/devLogger.js ...
6 months ago (2018-03-21 15:18:46 UTC) #7
kzar
Patch Set 5 : Nightmarish rebase https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#newcode81 lib/hitLogger.js:81: let request = ...
5 months, 1 week ago (2018-04-13 15:25:04 UTC) #8
kzar
Any chance you could take a look at this instead Manish? I think Sebastian's a ...
4 months, 4 weeks ago (2018-04-23 09:01:59 UTC) #9
Manish Jethani
https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#newcode34 lib/hitLogger.js:34: let HitLogger = exports.HitLogger = Object.create(new EventEmitter(), desc({ We ...
4 months, 4 weeks ago (2018-04-23 12:32:41 UTC) #10
Manish Jethani
https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#newcode138 lib/hitLogger.js:138: for (let type of nonRequestTypes) On 2018/04/23 12:32:40, Manish ...
4 months, 4 weeks ago (2018-04-23 13:32:04 UTC) #11
Manish Jethani
https://codereview.adblockplus.org/29705719/diff/29751602/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29705719/diff/29751602/lib/devtools.js#newcode261 lib/devtools.js:261: let hitListener = addRecord.bind(null, panel); I'm wondering if panel ...
4 months, 4 weeks ago (2018-04-23 15:30:59 UTC) #12
kzar
Patch Set 6 : Rebased https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#newcode34 lib/hitLogger.js:34: let HitLogger = exports.HitLogger ...
4 months, 3 weeks ago (2018-04-30 14:20:38 UTC) #13
Manish Jethani
https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#newcode34 lib/hitLogger.js:34: let HitLogger = exports.HitLogger = Object.create(new EventEmitter(), desc({ On ...
4 months, 3 weeks ago (2018-04-30 18:50:16 UTC) #14
Manish Jethani
https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#newcode34 lib/hitLogger.js:34: let HitLogger = exports.HitLogger = Object.create(new EventEmitter(), desc({ On ...
4 months, 3 weeks ago (2018-04-30 18:53:55 UTC) #15
kzar
Patch Set 7 : Addressed Manish's feedback I've just smoke-tested the latest changes for now, ...
4 months, 3 weeks ago (2018-05-01 10:53:09 UTC) #16
Manish Jethani
https://codereview.adblockplus.org/29705719/diff/29751602/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29705719/diff/29751602/lib/devtools.js#newcode261 lib/devtools.js:261: let hitListener = addRecord.bind(null, panel); On 2018/05/01 10:53:07, kzar ...
4 months, 2 weeks ago (2018-05-02 13:25:15 UTC) #17
kzar
https://codereview.adblockplus.org/29705719/diff/29767555/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29705719/diff/29767555/lib/csp.js#newcode36 lib/csp.js:36: let docDomain = extractHostFromFrame(parentFrame) || getDecodedHostname(url); On 2018/05/02 13:25:14, ...
4 months, 2 weeks ago (2018-05-02 15:41:50 UTC) #18
Sebastian Noack
https://codereview.adblockplus.org/29705719/diff/29767555/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29705719/diff/29767555/lib/csp.js#newcode36 lib/csp.js:36: let docDomain = extractHostFromFrame(parentFrame) || getDecodedHostname(url); On 2018/05/02 15:41:50, ...
4 months, 2 weeks ago (2018-05-02 16:11:34 UTC) #19
kzar
Patch Set 8 : Addressed Sebastian's feedback (Again I only smoke tested these latest changes, ...
4 months, 2 weeks ago (2018-05-03 15:56:53 UTC) #20
Sebastian Noack
https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#newcode91 lib/hitLogger.js:91: exports.logRequest = function(tabIds, request, filter) On 2018/05/03 15:56:52, kzar ...
4 months, 2 weeks ago (2018-05-03 16:40:58 UTC) #21
Manish Jethani
https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#newcode91 lib/hitLogger.js:91: exports.logRequest = function(tabIds, request, filter) On 2018/05/03 16:40:58, Sebastian ...
4 months, 2 weeks ago (2018-05-05 01:26:09 UTC) #22
Sebastian Noack
https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#newcode91 lib/hitLogger.js:91: exports.logRequest = function(tabIds, request, filter) On 2018/05/05 01:26:09, Manish ...
4 months, 2 weeks ago (2018-05-05 12:56:24 UTC) #23
Manish Jethani
LGTM
4 months, 2 weeks ago (2018-05-07 18:16:28 UTC) #24
kzar
Patch Set 9 : Another rebase!
4 months, 1 week ago (2018-05-09 11:22:46 UTC) #25
Sebastian Noack
Still LGTM.
4 months, 1 week ago (2018-05-09 11:39:48 UTC) #26
Manish Jethani
On 2018/05/09 11:22:46, kzar wrote: > Patch Set 9 : Another rebase! There's something wrong ...
4 months, 1 week ago (2018-05-09 14:01:44 UTC) #27
kzar
Patch Set 9 : Another try at uploading the rebase diff > There's something wrong ...
4 months, 1 week ago (2018-05-09 14:13:50 UTC) #28
Manish Jethani
https://codereview.adblockplus.org/29705719/diff/29775583/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29705719/diff/29775583/lib/csp.js#newcode34 lib/csp.js:34: let docDomain = extractHostFromFrame(parentFrame) || url.hostname; This should remain ...
4 months, 1 week ago (2018-05-09 15:11:12 UTC) #29
kzar
Patch Set 10 : Addressed Manish's feedback https://codereview.adblockplus.org/29705719/diff/29775583/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29705719/diff/29775583/lib/csp.js#newcode34 lib/csp.js:34: let docDomain ...
4 months, 1 week ago (2018-05-09 17:58:44 UTC) #30
Manish Jethani
4 months, 1 week ago (2018-05-09 22:29:18 UTC) #31
LGTM

https://codereview.adblockplus.org/29705719/diff/29775583/lib/hitLogger.js
File lib/hitLogger.js (right):

https://codereview.adblockplus.org/29705719/diff/29775583/lib/hitLogger.js#ne...
lib/hitLogger.js:84: *  The IDN-decoded hostname of the document
On 2018/05/09 17:58:42, kzar wrote:
> On 2018/05/09 15:11:11, Manish Jethani wrote:
> > The hostname is no longer IDN-decoded. Also we should call it hostname
instead
> > of docDomain here for consistency.
> 
> To rename it hostname involves changing adblockplusui as well, so I've kept it
> as docDomain for now.

Acknowledged.

https://codereview.adblockplus.org/29705719/diff/29775583/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29705719/diff/29775583/lib/requestBlocker....
lib/requestBlocker.js:192: {url: details.url, type, docDomain, thirdParty,
sitekey, specificOnly},
On 2018/05/09 17:58:43, kzar wrote:
> On 2018/05/09 15:11:12, Manish Jethani wrote:
> > Actually I think we should just call it hostname everywhere, even if
> Sebastian's
> > change didn't do it.
> 
> Well fine by me if we start calling it hostname consistently, but I'd rather
do
> that separately.

Acknowledged.
Sign in to reply to this message.

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