Issue 29705719: Issue 6402 - Split filter hit / request logging out into own API (Closed)
|
Created:
Feb. 22, 2018, 3:23 p.m. by kzar Modified:
May 10, 2018, 8:22 a.m. CC:
Thomas Greiner, saroyanm Visibility:
Public. |
DescriptionIssue 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 #
MessagesTotal messages: 31
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
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#ol... lib/devLogger.js:100: type: "add-record", This abstraction is specific to the devtools panel. 1. Filters are merely serialized so that they can be send to another process. 2. Responding to filter changes (i.e. the "update-record" event) and page reload (the "reset" event) would make the implementation of the issue reporter just unnecessarily complicated, without any benefit. 3. The hasRecord() check exposes a leaky abstraction if there is more than one listener. One listener might not get a record if the same record has already been reported to another listener that has been registered earlier. I think the listeners registered through this module should simply be called for each filter hit and request reported though the log*() functions. Higher-level logic specific to the devtools panel should stay in the devtools module. https://codereview.adblockplus.org/29705719/diff/29705741/lib/devLogger.js#ol... lib/devLogger.js:124: * @param {?Page} page The page the request occured on or null if It seems you turned all other "page" parameters into "pageId" (which I absolutely support, in regard of getting rid of ext.* foo), but why not here? Furthermore, I would rather call it tabId instead of pageId to match the browser.* API terminology. https://codereview.adblockplus.org/29705719/diff/29705741/lib/devLogger.js File lib/devLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29705741/lib/devLogger.js#ne... lib/devLogger.js:18: /** @module devLogger */ I'm not too happy with the name "devLogger". It is meant to be an abstraction of the devtools use case, but yet it relates to it in the name. More accurate would be "filterAndRequestLogger", this however looks quite ugly and long. So the best I can come up with, at the moment, would be just "logger". What do you think? https://codereview.adblockplus.org/29705719/diff/29705741/lib/devLogger.js#ne... lib/devLogger.js:399: pageLoggers.delete(pageId); Is this logic correct? So if we have only one listener left, for that page, we implicitly assume that it matches the given callback, or do I miss something?
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 File lib/devLogger.js (left): https://codereview.adblockplus.org/29705719/diff/29705741/lib/devLogger.js#ol... lib/devLogger.js:124: * @param {?Page} page The page the request occured on or null if On 2018/03/20 01:28:25, Sebastian Noack wrote: > It seems you turned all other "page" parameters into "pageId" (which I > absolutely support, in regard of getting rid of ext.* foo), but why not here? > > Furthermore, I would rather call it tabId instead of pageId to match the > browser.* API terminology. Done. https://codereview.adblockplus.org/29705719/diff/29705741/lib/devLogger.js File lib/devLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29705741/lib/devLogger.js#ne... lib/devLogger.js:18: /** @module devLogger */ On 2018/03/20 01:28:26, Sebastian Noack wrote: > I'm not too happy with the name "devLogger". It is meant to be an abstraction of > the devtools use case, but yet it relates to it in the name. More accurate would > be "filterAndRequestLogger", this however looks quite ugly and long. So the best > I can come up with, at the moment, would be just "logger". What do you think? I agree the name sucked but I'm also struggling to do much better. How about hitLogger? logger seems a bit vague to me. https://codereview.adblockplus.org/29705719/diff/29729619/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29705719/diff/29729619/lib/devtools.js#new... lib/devtools.js:96: if (isActivePanel(panel) && !hasRecord(panel, request, filter)) Since we can no longer do the `isActivePanel` check in `logRequest` I moved it here, which means it's additionally called as a result of `logWhitelistedDocument` and `logHiddenElements` as well. I'm not sure if that matters. https://codereview.adblockplus.org/29705719/diff/29729619/lib/hitLogger.js File lib/hitLogger.js (right): https://codereview.adblockplus.org/29705719/diff/29729619/lib/hitLogger.js#ne... lib/hitLogger.js:34: this.__proto__.off(tabId, listener); I wanted to use `super` here instead of `this.__proto__` but that didn't work. I'm not sure why.
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#ne... lib/hitLogger.js:81: let request = {url, type, docDomain, thirdParty, sitekey, specificOnly}; Note: I'm not adding the tabId in here any more Thomas since with Sebastian's recent changes we're making use of the initiatorUrl / originUrl fields to figure out which tabs requests are associated with.
Any chance you could take a look at this instead Manish? I think Sebastian's a bit busy.
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#ne... lib/hitLogger.js:34: let HitLogger = exports.HitLogger = Object.create(new EventEmitter(), desc({ We can get rid of desc here now with Object.assign. exports.HitLogger = Object.assign(Object.create(new EventEmitter()), { off(tabId, listener) { .. } }); It does the same thing but is more straightforward and probably also more efficient. https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... lib/hitLogger.js:73: exports.logRequest = function(tabIds, url, type, docDomain, Since "request" is now a type of object, I'm wondering if this function shouldn't just be logRequest(tabIds, request, filter) The documentation above can still expand on what the request object includes. https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... lib/hitLogger.js:79: if (hasListener(tabId)) Seems unnecessary to call hasListener here. If there's no listener then nothing will happen, the emit function calls _listeners.get anyway. https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... lib/hitLogger.js:138: for (let type of nonRequestTypes) We can avoid this for loop here. nonRequestTypes should be declared like so: nonRequestTypes = RegExpFilter.typeMap.DOCUMENT | RegExpFilter.typeMap.ELEMHIDE | RegExpFilter.typeMap.GENERICBLOCK | RegExpFilter.typeMap.GENERICHIDE | RegExpFilter.typeMap.CSP; Then we can simply do: if (typeMask & filter.contentType & nonRequestTypes) HitLogger.emit(tabId, {url, type, domain}, filter); This way we can also avoid the hasListener check above.
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#ne... lib/hitLogger.js:138: for (let type of nonRequestTypes) On 2018/04/23 12:32:40, Manish Jethani wrote: > We can avoid this for loop here. nonRequestTypes should be declared like so: > > nonRequestTypes = RegExpFilter.typeMap.DOCUMENT | > RegExpFilter.typeMap.ELEMHIDE | > RegExpFilter.typeMap.GENERICBLOCK | > RegExpFilter.typeMap.GENERICHIDE | > RegExpFilter.typeMap.CSP; > > Then we can simply do: > > if (typeMask & filter.contentType & nonRequestTypes) > HitLogger.emit(tabId, {url, type, domain}, filter); Sorry I misunderstood, so we want to know which of the nonRequestTypes matches and emit an event for each one. In that case this won't work, please ignore this comment.
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#new... lib/devtools.js:261: let hitListener = addRecord.bind(null, panel); I'm wondering if panel should be an object of a Panel class. function Panel(port) { this.port = port; this.records = []; } Panel.prototype = { addRecord(request, filter) { if (!this.hasRecord(request, filter)) { ... } }, hasRecord(request, filter) { ... } }; 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#ne... lib/hitLogger.js:22: const {desc} = require("../adblockpluscore/lib/coreUtils"); I'm assuming the paths here are temporary and will be updated. https://codereview.adblockplus.org/29705719/diff/29751602/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29705719/diff/29751602/metadata.chrome#new... metadata.chrome:125: lib/devtools.js Where is lib/hitLogger.js included?
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#ne... lib/hitLogger.js:34: let HitLogger = exports.HitLogger = Object.create(new EventEmitter(), desc({ On 2018/04/23 12:32:41, Manish Jethani wrote: > exports.HitLogger = Object.assign(Object.create(new EventEmitter()), { Out of curiosity what's the point of calling `Object.create` here? In other words why couldn't we do it like this? exports.HitLogger = Object.assign(new EventEmitter(), {
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#ne... lib/hitLogger.js:34: let HitLogger = exports.HitLogger = Object.create(new EventEmitter(), desc({ On 2018/04/30 14:20:37, kzar wrote: > On 2018/04/23 12:32:41, Manish Jethani wrote: > > exports.HitLogger = Object.assign(Object.create(new EventEmitter()), { > > Out of curiosity what's the point of calling `Object.create` here? In other > words why couldn't we do it like this? > > exports.HitLogger = Object.assign(new EventEmitter(), { It depends what you're trying to do. What I suggested is the exact equivalent of what was being done. In that case HitLogger instance of EventEmitter is true, HitLogger has an own property "off", HitLogger.__proto__ has an own property "_listeners", and HitLogger.__proto__.__proto__ has own properties "on", "off", "emit", etc. If we use only Object.assign then we'll merge HitLogger and HitLogger.__proto__ which is fine because it is effectively a singleton, an instance of EventEmitter that overrides the "off" method at the instance level. So yeah it should be OK. For what it's worth I wonder if we could just avoid this by writing hasListener as: return (HitLogger._listeners.get(tabId) || []).length > 0; We should probably also encapsulate the event emitter so that there's an addListener and removeListener to complement hasListener. In that case there would be a private EventEmitter instance in this module and the outside world wouldn't know how it was managing the events. let eventEmitter = new EventEmitter(); exports.addListener = (tabId, listener) => eventEmitter.on(tabId, listener); exports.removeListener = (tabId, listener) => eventEmitter.off(tabId, listener); exports.hasListeners = tabId => (eventEmitter._listeners.get(tabId) || []).length > 0;
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#ne... lib/hitLogger.js:34: let HitLogger = exports.HitLogger = Object.create(new EventEmitter(), desc({ On 2018/04/30 18:50:16, Manish Jethani wrote: > [...] > > let eventEmitter = new EventEmitter(); > > exports.addListener = (tabId, listener) => eventEmitter.on(tabId, listener); > exports.removeListener = (tabId, listener) => eventEmitter.off(tabId, > listener); Or: exports.addListener = eventEmitter.on.bind(eventEmitter); exports.removeListener = eventEmitter.off.bind(eventEmitter);
Patch Set 7 : Addressed Manish's feedback I've just smoke-tested the latest changes for now, since it takes some time to test properly. I'll test everything properly again after it passes review. 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#new... lib/devtools.js:261: let hitListener = addRecord.bind(null, panel); On 2018/04/23 15:30:58, Manish Jethani wrote: > I'm wondering if panel should be an object of a Panel class. > > function Panel(port) > { > this.port = port; > this.records = []; > } > > Panel.prototype = { > addRecord(request, filter) > { > if (!this.hasRecord(request, filter)) > { > ... > } > }, > hasRecord(request, filter) > { > ... > } > }; Seems like kind of an unrelated change to me, I'll leave this one for now. 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#ne... lib/hitLogger.js:22: const {desc} = require("../adblockpluscore/lib/coreUtils"); On 2018/04/23 15:30:58, Manish Jethani wrote: > I'm assuming the paths here are temporary and will be updated. I'm not sure I understand this comment, but no these paths aren't temporary. https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... lib/hitLogger.js:34: let HitLogger = exports.HitLogger = Object.create(new EventEmitter(), desc({ On 2018/04/30 18:50:16, Manish Jethani wrote: > > For what it's worth I wonder if we could just avoid this by writing hasListener > as: ... > > We should probably also encapsulate the event emitter so that there's an > addListener and removeListener to complement hasListener. In that case there > would be a private EventEmitter instance in this module and the outside world > wouldn't know how it was managing the events... Sure I can do it this way if you prefer, Done. https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... lib/hitLogger.js:73: exports.logRequest = function(tabIds, url, type, docDomain, On 2018/04/23 12:32:40, Manish Jethani wrote: > Since "request" is now a type of object, I'm wondering if this function > shouldn't just be logRequest(tabIds, request, filter) > > The documentation above can still expand on what the request object includes. Done. https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... lib/hitLogger.js:79: if (hasListener(tabId)) On 2018/04/23 12:32:40, Manish Jethani wrote: > Seems unnecessary to call hasListener here. If there's no listener then nothing > will happen, the emit function calls _listeners.get anyway. Done. https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... lib/hitLogger.js:138: for (let type of nonRequestTypes) On 2018/04/23 13:32:03, Manish Jethani wrote: > On 2018/04/23 12:32:40, Manish Jethani wrote: > > We can avoid this for loop here. nonRequestTypes should be declared like so: > > > > nonRequestTypes = RegExpFilter.typeMap.DOCUMENT | > > RegExpFilter.typeMap.ELEMHIDE | > > RegExpFilter.typeMap.GENERICBLOCK | > > RegExpFilter.typeMap.GENERICHIDE | > > RegExpFilter.typeMap.CSP; > > > > Then we can simply do: > > > > if (typeMask & filter.contentType & nonRequestTypes) > > HitLogger.emit(tabId, {url, type, domain}, filter); > > Sorry I misunderstood, so we want to know which of the nonRequestTypes matches > and emit an event for each one. In that case this won't work, please ignore this > comment. Acknowledged. https://codereview.adblockplus.org/29705719/diff/29751602/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29705719/diff/29751602/metadata.chrome#new... metadata.chrome:125: lib/devtools.js On 2018/04/23 15:30:59, Manish Jethani wrote: > Where is lib/hitLogger.js included? The modules listed here are "entry points" which WebPack includes in the bundle and has the bundle run right away. Any other modules which are required by one of the entry points will be included in the bundle automatically by WebPack. 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); I renamed this in order to use the Object shorthand later. It makes sense now that we're passing through a request Object to logRequest.
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#new... lib/devtools.js:261: let hitListener = addRecord.bind(null, panel); On 2018/05/01 10:53:07, kzar wrote: > On 2018/04/23 15:30:58, Manish Jethani wrote: > > I'm wondering if panel should be an object of a Panel class. > > > > function Panel(port) > > { > > this.port = port; > > this.records = []; > > } > > > > Panel.prototype = { > > addRecord(request, filter) > > { > > if (!this.hasRecord(request, filter)) > > { > > ... > > } > > }, > > hasRecord(request, filter) > > { > > ... > > } > > }; > > Seems like kind of an unrelated change to me, I'll leave this one for now. Acknowledged. 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#ne... lib/hitLogger.js:22: const {desc} = require("../adblockpluscore/lib/coreUtils"); On 2018/05/01 10:53:08, kzar wrote: > On 2018/04/23 15:30:58, Manish Jethani wrote: > > I'm assuming the paths here are temporary and will be updated. > > I'm not sure I understand this comment, but no these paths aren't temporary. Sorry, I wasn't aware of #5760. It looks right to me now. https://codereview.adblockplus.org/29705719/diff/29751602/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29705719/diff/29751602/metadata.chrome#new... metadata.chrome:125: lib/devtools.js On 2018/05/01 10:53:08, kzar wrote: > On 2018/04/23 15:30:59, Manish Jethani wrote: > > Where is lib/hitLogger.js included? > > The modules listed here are "entry points" which WebPack includes in the bundle > and has the bundle run right away. Any other modules which are required by one > of the entry points will be included in the bundle automatically by WebPack. Acknowledged. 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/01 10:53:09, kzar wrote: > I renamed this in order to use the Object shorthand later. It makes sense now > that we're passing through a request Object to logRequest. Acknowledged. Maybe rename urlString to url and and url to urlObject?
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, Manish Jethani wrote: > On 2018/05/01 10:53:09, kzar wrote: > > I renamed this in order to use the Object shorthand later. It makes sense now > > that we're passing through a request Object to logRequest. > > Acknowledged. > > Maybe rename urlString to url and and url to urlObject? I think I prefer it like it is, I find the name urlObject kind of ugly somehow.
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, kzar wrote: > On 2018/05/02 13:25:14, Manish Jethani wrote: > > On 2018/05/01 10:53:09, kzar wrote: > > > I renamed this in order to use the Object shorthand later. It makes sense > now > > > that we're passing through a request Object to logRequest. > > > > Acknowledged. > > > > Maybe rename urlString to url and and url to urlObject? > > I think I prefer it like it is, I find the name urlObject kind of ugly somehow. Agreed with kzar (also in favor to avoid wrapping the line here). https://codereview.adblockplus.org/29705719/diff/29767555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29705719/diff/29767555/lib/devtools.js#new... lib/devtools.js:261: let hitListener = addRecord.bind(null, panel); Nit: It seems weird to define this variable here, but "panel" above. I would define them in subsequent lines, either above or here. 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#ne... lib/hitLogger.js:41: * Adds a listener for requests, filter hits etc related to the tab. Can we please document/specify that the calling code is responsible for removing any listener themselves and that the listener is not automatically removed when the tab is closed/navigates/etc? https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#ne... lib/hitLogger.js:62: hasListener(tabId) Does this method needs to be public (i.e. in the HitLogger object)? It seems it is only used for some internal optimizations inside this module? https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#ne... lib/hitLogger.js:91: exports.logRequest = function(tabIds, request, filter) Any particular reason why you pass in the request information in the internal data structure used by the hitLogger? It seems to just add boilerplate to the calling code and requires to create an additional object no matter what? https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#ne... lib/hitLogger.js:143: exports.logWhitelistedDocument = function(tabId, url, typeMask, docDomain, Nit: It seems in other modules we use arrow function syntax for exported functions. As a side effect this might also avoid wrapping the line here. https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#ne... lib/hitLogger.js:158: logHiddenElements( I wonder it would make more sense to just inline the logic here, since logHiddenElements() is only called once and not exported.
Patch Set 8 : Addressed Sebastian's feedback (Again I only smoke tested these latest changes, I will test more thoroughly again after this has passed review.) https://codereview.adblockplus.org/29705719/diff/29767555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29705719/diff/29767555/lib/devtools.js#new... lib/devtools.js:261: let hitListener = addRecord.bind(null, panel); On 2018/05/02 16:11:32, Sebastian Noack wrote: > Nit: It seems weird to define this variable here, but "panel" above. I would > define them in subsequent lines, either above or here. Done. 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#ne... lib/hitLogger.js:41: * Adds a listener for requests, filter hits etc related to the tab. On 2018/05/02 16:11:33, Sebastian Noack wrote: > Can we please document/specify that the calling code is responsible for removing > any listener themselves and that the listener is not automatically removed when > the tab is closed/navigates/etc? Done. https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#ne... lib/hitLogger.js:62: hasListener(tabId) On 2018/05/02 16:11:33, Sebastian Noack wrote: > Does this method needs to be public (i.e. in the HitLogger object)? It seems it > is only used for some internal optimizations inside this module? Yea, since it's also used in lib/cssInjection.js. https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#ne... lib/hitLogger.js:91: exports.logRequest = function(tabIds, request, filter) On 2018/05/02 16:11:33, Sebastian Noack wrote: > Any particular reason why you pass in the request information in the internal > data structure used by the hitLogger? It seems to just add boilerplate to the > calling code and requires to create an additional object no matter what? Well I did it this way since Manish suggested it[1]. FWIW I didn't mind too much either way but I probably prefer it how it is now, passing in the request Object. [1] - https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#ne... lib/hitLogger.js:143: exports.logWhitelistedDocument = function(tabId, url, typeMask, docDomain, On 2018/05/02 16:11:33, Sebastian Noack wrote: > Nit: It seems in other modules we use arrow function syntax for exported > functions. As a side effect this might also avoid wrapping the line here. Done. https://codereview.adblockplus.org/29705719/diff/29767555/lib/hitLogger.js#ne... lib/hitLogger.js:158: logHiddenElements( On 2018/05/02 16:11:33, Sebastian Noack wrote: > I wonder it would make more sense to just inline the logic here, since > logHiddenElements() is only called once and not exported. I prefer it how it is, I think it's easier to reason with this way.
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#ne... lib/hitLogger.js:91: exports.logRequest = function(tabIds, request, filter) On 2018/05/03 15:56:52, kzar wrote: > On 2018/05/02 16:11:33, Sebastian Noack wrote: > > Any particular reason why you pass in the request information in the internal > > data structure used by the hitLogger? It seems to just add boilerplate to the > > calling code and requires to create an additional object no matter what? > > Well I did it this way since Manish suggested it[1]. FWIW I didn't mind too much > either way but I probably prefer it how it is now, passing in the request > Object. > > [1] - > https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... Well, the main argument for creating the request object on demand, rather than passing it in externally, is to avoid creating the object if there is no listener registered (i.e. in the common case). Manish, what is your reasoning here, for doing it the other way around?
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#ne... lib/hitLogger.js:91: exports.logRequest = function(tabIds, request, filter) On 2018/05/03 16:40:58, Sebastian Noack wrote: > On 2018/05/03 15:56:52, kzar wrote: > > On 2018/05/02 16:11:33, Sebastian Noack wrote: > > > Any particular reason why you pass in the request information in the > internal > > > data structure used by the hitLogger? It seems to just add boilerplate to > the > > > calling code and requires to create an additional object no matter what? > > > > Well I did it this way since Manish suggested it[1]. FWIW I didn't mind too > much > > either way but I probably prefer it how it is now, passing in the request > > Object. > > > > [1] - > > > https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... > > Well, the main argument for creating the request object on demand, rather than > passing it in externally, is to avoid creating the object if there is no > listener registered (i.e. in the common case). Manish, what is your reasoning > here, for doing it the other way around? No reasoning other than it makes the code easy to reason about. My counter question would be, what is the reason to avoid creating the object? Note that we're not checking whether there's a listener now because it's no longer needed. If there's any performance benefit to avoid creating an object, I would be in favor of it. We would also have to include the cost of checking whether there's a listener in that calculation. Right now the code seems fairly straightforward.
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#ne... lib/hitLogger.js:91: exports.logRequest = function(tabIds, request, filter) On 2018/05/05 01:26:09, Manish Jethani wrote: > On 2018/05/03 16:40:58, Sebastian Noack wrote: > > On 2018/05/03 15:56:52, kzar wrote: > > > On 2018/05/02 16:11:33, Sebastian Noack wrote: > > > > Any particular reason why you pass in the request information in the > > internal > > > > data structure used by the hitLogger? It seems to just add boilerplate to > > the > > > > calling code and requires to create an additional object no matter what? > > > > > > Well I did it this way since Manish suggested it[1]. FWIW I didn't mind too > > much > > > either way but I probably prefer it how it is now, passing in the request > > > Object. > > > > > > [1] - > > > > > > https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne... > > > > Well, the main argument for creating the request object on demand, rather than > > passing it in externally, is to avoid creating the object if there is no > > listener registered (i.e. in the common case). Manish, what is your reasoning > > here, for doing it the other way around? > > No reasoning other than it makes the code easy to reason about. My counter > question would be, what is the reason to avoid creating the object? Note that > we're not checking whether there's a listener now because it's no longer needed. > If there's any performance benefit to avoid creating an object, I would be in > favor of it. We would also have to include the cost of checking whether there's > a listener in that calculation. Right now the code seems fairly straightforward. Fair enough. I'm still not entirely convinced that this change made the code more readable (when I look at the calling code), but you made a point, that with the added abstraction, creating the request object on demand will require additional complexity, and the benefits of creating the object on demand might be cancelled out. So LGTM if both of you prefer to leave it like that.
LGTM
Patch Set 9 : Another rebase!
Still LGTM.
On 2018/05/09 11:22:46, kzar wrote: > Patch Set 9 : Another rebase! There's something wrong with the rebase. For example it shouldn't want to delete lib/punycode.js because it's already deleted.
Patch Set 9 : Another try at uploading the rebase diff > There's something wrong with the rebase. For example it > shouldn't want to delete lib/punycode.js because it's > already deleted. Argh you're right, I think I uploaded the diff vs `master` instead of `next`. Does this look OK to you now?
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 hostname? 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 The hostname is no longer IDN-decoded. Also we should call it hostname instead of docDomain here for consistency. https://codereview.adblockplus.org/29705719/diff/29775583/lib/popupBlocker.js File lib/popupBlocker.js (right): https://codereview.adblockplus.org/29705719/diff/29775583/lib/popupBlocker.js... lib/popupBlocker.js:47: let docDomain = extractHostFromFrame(popup.sourceFrame); Let's keep it documentHost here? 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}, Actually I think we should just call it hostname everywhere, even if Sebastian's change didn't do it.
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 = extractHostFromFrame(parentFrame) || url.hostname; On 2018/05/09 15:11:11, Manish Jethani wrote: > This should remain hostname? Done. 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 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. https://codereview.adblockplus.org/29705719/diff/29775583/lib/popupBlocker.js File lib/popupBlocker.js (right): https://codereview.adblockplus.org/29705719/diff/29775583/lib/popupBlocker.js... lib/popupBlocker.js:47: let docDomain = extractHostFromFrame(popup.sourceFrame); On 2018/05/09 15:11:11, Manish Jethani wrote: > Let's keep it documentHost here? Done. 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 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.
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. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
