Issue 29370970: [adblockpluschrome] Issue 3596 - Added support for CSS property filters to devtools panel (Closed)
|
Created:
Jan. 10, 2017, 10:54 a.m. by wspee Modified:
Feb. 23, 2017, 3:57 p.m. Reviewers:
Sebastian Noack CC:
kzar Visibility:
Public. |
DescriptionIssue 3596 - Added support for CSS property filters to devtools panel
Patch Set 1 #
Total comments: 24
Patch Set 2 : No longer use ElementHidingTracer to log ElemHideEmulationFilter in the devpanel #
Total comments: 4
Patch Set 3 : Made first approach work "properly" #Patch Set 4 : Use map instead of object #Patch Set 5 : Changed map to two arrays #
Total comments: 7
Patch Set 6 : Use Array of Arrays instead of Map in ElementHidingTracer #Patch Set 7 : Use two Arrays insteaf of Array of Arrays in ElemHidingTracer #
Total comments: 8
Patch Set 8 : Moved ElementHidindTracer related logic into ElementHidindTracer and addressed review comments #
Total comments: 21
Patch Set 9 : renamed addFilters to addSelectors and moved empty filters initialization to the top #Patch Set 10 : Removed start logic and made filter loading asynchronous again #
Total comments: 10
Patch Set 11 : Removed obsolete ElemHideEmulation.load and addressed review comments #
Total comments: 4
Patch Set 12 : Addressed review comments #
Total comments: 3
Patch Set 13 : Rebased updated dependecies and addressed review comment #
Total comments: 6
Patch Set 14 : Removed obsolete variable and blank line #Patch Set 15 : Addressed review comment #
MessagesTotal messages: 31
https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:197: this.selectors = selectors || []; It seems you removed the only code that initialized the ElementHidingTracer without arguments. So unless I missed something, why not just removing support for the "selectors" argument? https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:514: this.tracer.selectors.push({ Creating a new object for each selector might potentially add a significant memory footprint. A possibly better solution would be to use two arrays, one for the selectors and one for the filters. Perhaps you could benchmark the memory usage with the current approach and compare it against the one I'm suggesting. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:514: this.tracer.selectors.push({ Is it guaranteed that the order of selectors match the order of filters? I have some doubts from looking at the code in elemHideEmulation.js: addSelectors: function(stylesheets) { var selectors = []; var filters = {}; for (var i = 0; i < stylesheets.length; i++) this.findSelectors(stylesheets[i], selectors, filters); this.addSelectorsFunc(selectors, Object.keys(filters)); }, Note that objects in JavaScript don't guarantee properties to be retrieved in insertion order (like dicts in Python). However, we could simply use a Set in elemHideEmulation.js which preserves the insertion order and seems more appropriate anyway, but wasn't supported by all supported browsers we run that code on back then. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:588: if (this.tracer && this.tracer.selectors.length) Any particular reason (except for a hypothetical micro-optimization) to check for this.tracer.selectors.length? https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:592: ext.backgroundPage.sendMessage({type: "get-selectors"}, (response) => The parentheses around the "response" argument are redundant. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:595: this.elemHideEmulation.load(() => { checkLoaded(); }); The arrow function seems redundant. Why not just passing checkLoaded without wrapping to the load() method? https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js#new... lib/devtools.js:20: let { I think I would prefer to wrap this line like this: let {RegExpFilter, WhitelistFilter, ElemHideFilter, ElemHideEmulationFilter} = require("filterClasses"); Or like that: let {RegExpFilter, WhitelistFilter, ElemHideFilter, ElemHideEmulationFilter} = require("filterClasses"); But I guess this is mostly personal preference, though it is more concise and looks a little more consistent with the surrounding one-line destructuring assignments. So let me know if you disagree. https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js#new... lib/devtools.js:179: !(filter instanceof ElemHideEmulationFilter)) This logic could be slightly simplified (one less logical operation): if (!(filter instanceof ElemHideFilter || filter instanceof ElemHideEmulationFilter)) I'm not sure which reads more natural though. I leave it up to you.
https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:197: this.selectors = selectors || []; On 2017/01/11 16:20:40, Sebastian Noack wrote: > It seems you removed the only code that initialized the ElementHidingTracer > without arguments. So unless I missed something, why not just removing support > for the "selectors" argument? No longer relevant. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:514: this.tracer.selectors.push({ On 2017/01/11 16:20:40, Sebastian Noack wrote: > Creating a new object for each selector might potentially add a significant > memory footprint. A possibly better solution would be to use two arrays, one for > the selectors and one for the filters. Perhaps you could benchmark the memory > usage with the current approach and compare it against the one I'm suggesting. No longer relevant. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:514: this.tracer.selectors.push({ On 2017/01/11 16:20:40, Sebastian Noack wrote: > Is it guaranteed that the order of selectors match the order of filters? I have > some doubts from looking at the code in elemHideEmulation.js: > > addSelectors: function(stylesheets) > { > var selectors = []; > var filters = {}; > for (var i = 0; i < stylesheets.length; i++) > this.findSelectors(stylesheets[i], selectors, filters); > this.addSelectorsFunc(selectors, Object.keys(filters)); > }, > > Note that objects in JavaScript don't guarantee properties to be retrieved in > insertion order (like dicts in Python). However, we could simply use a Set in > elemHideEmulation.js which preserves the insertion order and seems more > appropriate anyway, but wasn't supported by all supported browsers we run that > code on back then. Indeed ... if you look at findSelectors there can even be more selectors than filters. findSelectors: function(stylesheet, selectors, filters) { ... var subSelectors = splitSelector(rule.selectorText); for (var k = 0; k < subSelectors.length; k++) selectors.push(pattern.prefix + subSelectors[k] + pattern.suffix); filters[pattern.text] = true; ... }, So I have switched the whole approach to something else entirely .... ElemHideEmulationFilter are only created if there is a style that matches the filter and document changes are also handled by ElemHideEmulation so it wasn't really necessary to use the ElementHidingTracer for ElemHideEmulationFilter. Instead I let addSelectors fire devtools.traceElemHide directly similar to how it works in firefox (adblockplus/lib/child/elemHideEmulation.js:initElemHideEmulation). Only drawback is that devtools.traceElemHide gets fired regardless of whether the devtools are opened or not. I could add a flag to ElemHide but that whould be somewhat redundant with ElemHide.tracer but I cannot test ElemHide.tracer as it gets assigned after addSelectors is called by ElemHideEmulation. But logHiddenElements does (should have?!) check if the panel is opened anyways so it's only a slight penalty. Let me now if you think otherwise. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:588: if (this.tracer && this.tracer.selectors.length) On 2017/01/11 16:20:40, Sebastian Noack wrote: > Any particular reason (except for a hypothetical micro-optimization) to check > for this.tracer.selectors.length? No longer relevant. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:592: ext.backgroundPage.sendMessage({type: "get-selectors"}, (response) => On 2017/01/11 16:20:40, Sebastian Noack wrote: > The parentheses around the "response" argument are redundant. No longer relevant. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:595: this.elemHideEmulation.load(() => { checkLoaded(); }); On 2017/01/11 16:20:40, Sebastian Noack wrote: > The arrow function seems redundant. Why not just passing checkLoaded without > wrapping to the load() method? No longer relevant. https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js#new... lib/devtools.js:20: let { On 2017/01/11 16:20:41, Sebastian Noack wrote: > I think I would prefer to wrap this line like this: > > let {RegExpFilter, WhitelistFilter, > ElemHideFilter, ElemHideEmulationFilter} = require("filterClasses"); > > Or like that: > > let {RegExpFilter, > WhitelistFilter, > ElemHideFilter, > ElemHideEmulationFilter} = require("filterClasses"); > > But I guess this is mostly personal preference, though it is more concise and > looks a little more consistent with the surrounding one-line destructuring > assignments. So let me know if you disagree. Done. https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js#new... lib/devtools.js:179: !(filter instanceof ElemHideEmulationFilter)) On 2017/01/11 16:20:41, Sebastian Noack wrote: > This logic could be slightly simplified (one less logical operation): > > if (!(filter instanceof ElemHideFilter || > filter instanceof ElemHideEmulationFilter)) > > I'm not sure which reads more natural though. I leave it up to you. Done.
https://codereview.adblockplus.org/29370970/diff/29371604/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29371604/include.preload.js#... include.preload.js:503: if (filters) Yeah, this is more like it works on Firefox currently, which is actually broken, as it logs a filter match whenever an emulation filter causes any selectors to be injected regardless whether they match/hide any elements. https://codereview.adblockplus.org/29370970/diff/29371604/include.preload.js#... include.preload.js:503: if (filters) And yes, it's also not great that we message the background page even if there is no open devtools panel for this page, but since this happens only for emulation filters which slow down everything anyway, it might be acceptable. But why not simply checking for this.tracer?
Back to the first approach but I changed ElemHideEmulation [0] to allow me to test the correct filter for each selector. P.s.: To not waste time of the core and ff owners, I plan to submit [0] (and the corresponding ff change) after it's clear that this is the right approach to fix it. Good idea?! [0] https://codereview.adblockplus.org/29372676/ https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:197: this.selectors = selectors || []; On 2017/01/12 13:39:58, wspee wrote: > On 2017/01/11 16:20:40, Sebastian Noack wrote: > > It seems you removed the only code that initialized the ElementHidingTracer > > without arguments. So unless I missed something, why not just removing support > > for the "selectors" argument? > > No longer relevant. Done. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:514: this.tracer.selectors.push({ On 2017/01/11 16:20:40, Sebastian Noack wrote: > Is it guaranteed that the order of selectors match the order of filters? I have > some doubts from looking at the code in elemHideEmulation.js: > > addSelectors: function(stylesheets) > { > var selectors = []; > var filters = {}; > for (var i = 0; i < stylesheets.length; i++) > this.findSelectors(stylesheets[i], selectors, filters); > this.addSelectorsFunc(selectors, Object.keys(filters)); > }, > > Note that objects in JavaScript don't guarantee properties to be retrieved in > insertion order (like dicts in Python). However, we could simply use a Set in > elemHideEmulation.js which preserves the insertion order and seems more > appropriate anyway, but wasn't supported by all supported browsers we run that > code on back then. Done. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:514: this.tracer.selectors.push({ On 2017/01/11 16:20:40, Sebastian Noack wrote: > Creating a new object for each selector might potentially add a significant > memory footprint. A possibly better solution would be to use two arrays, one for > the selectors and one for the filters. Perhaps you could benchmark the memory > usage with the current approach and compare it against the one I'm suggesting. According to the chrome profiler an array with 10000 objects uses less memory than two arrays with 10000 entries, so I kept that approach. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:588: if (this.tracer && this.tracer.selectors.length) On 2017/01/11 16:20:40, Sebastian Noack wrote: > Any particular reason (except for a hypothetical micro-optimization) to check > for this.tracer.selectors.length? Done. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:592: ext.backgroundPage.sendMessage({type: "get-selectors"}, (response) => On 2017/01/11 16:20:40, Sebastian Noack wrote: > The parentheses around the "response" argument are redundant. Done. https://codereview.adblockplus.org/29370970/diff/29370971/include.preload.js#... include.preload.js:595: this.elemHideEmulation.load(() => { checkLoaded(); }); On 2017/01/11 16:20:40, Sebastian Noack wrote: > The arrow function seems redundant. Why not just passing checkLoaded without > wrapping to the load() method? Done. https://codereview.adblockplus.org/29370970/diff/29371604/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29371604/include.preload.js#... include.preload.js:503: if (filters) On 2017/01/12 17:03:29, Sebastian Noack wrote: > Yeah, this is more like it works on Firefox currently, which is actually broken, > as it logs a filter match whenever an emulation filter causes any selectors to > be injected regardless whether they match/hide any elements. Done. https://codereview.adblockplus.org/29370970/diff/29371604/include.preload.js#... include.preload.js:503: if (filters) On 2017/01/12 17:03:29, Sebastian Noack wrote: > And yes, it's also not great that we message the background page even if there > is no open devtools panel for this page, but since this happens only for > emulation filters which slow down everything anyway, it might be acceptable. But > why not simply checking for this.tracer? Done.
Changed filters type from object to map, see https://codereview.adblockplus.org/29372676/#msg2.
Changed map to two arrays, see: https://codereview.adblockplus.org/29372676/#msg10
https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js#new... lib/devtools.js:20: let { On 2017/01/12 13:39:58, wspee wrote: > On 2017/01/11 16:20:41, Sebastian Noack wrote: > > I think I would prefer to wrap this line like this: > > > > let {RegExpFilter, WhitelistFilter, > > ElemHideFilter, ElemHideEmulationFilter} = require("filterClasses"); > > > > Or like that: > > > > let {RegExpFilter, > > WhitelistFilter, > > ElemHideFilter, > > ElemHideEmulationFilter} = require("filterClasses"); > > > > But I guess this is mostly personal preference, though it is more concise and > > looks a little more consistent with the surrounding one-line destructuring > > assignments. So let me know if you disagree. > > Done. It seems you haven't changed that code. But since you said "Done", it seems you don't disagree, but it's rather an accident that this code still looks the same? https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js#... include.preload.js:197: this.filters = new Map(); Why to use a a Map object? It seems the logic would be much simpler, if you just have one array for selectors and one for the filters? https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js#... include.preload.js:256: matched = true; It seems, the "match" variable would be unnecessary, if you break out of the outer loop here, directly: nodes: for (let node of nodes) { for (let element of elements) { ... break nodes; } } } https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js#... include.preload.js:519: if (!selectors) Why did you change this line? Note that an empty array evaluates to true.
https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29370970/diff/29370971/lib/devtools.js#new... lib/devtools.js:20: let { On 2017/02/09 11:07:38, Sebastian Noack wrote: > On 2017/01/12 13:39:58, wspee wrote: > > On 2017/01/11 16:20:41, Sebastian Noack wrote: > > > I think I would prefer to wrap this line like this: > > > > > > let {RegExpFilter, WhitelistFilter, > > > ElemHideFilter, ElemHideEmulationFilter} = require("filterClasses"); > > > > > > Or like that: > > > > > > let {RegExpFilter, > > > WhitelistFilter, > > > ElemHideFilter, > > > ElemHideEmulationFilter} = require("filterClasses"); > > > > > > But I guess this is mostly personal preference, though it is more concise > and > > > looks a little more consistent with the surrounding one-line destructuring > > > assignments. So let me know if you disagree. > > > > Done. > > It seems you haven't changed that code. But since you said "Done", it seems you > don't disagree, but it's rather an accident that this code still looks the same? Done. https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js#... include.preload.js:197: this.filters = new Map(); On 2017/02/09 11:07:39, Sebastian Noack wrote: > Why to use a a Map object? It seems the logic would be much simpler, if you just > have one array for selectors and one for the filters? If you have information that are are dependent from one another it makes sense to also use a data structure to reflects that. The selectors belong to a filter so it makes sense to use a key/value data structure for that. The performance arguments we used against a map previously don't apply here because (as it stands now) the ElementHidingTracer will hold all ElementHiding filters in memory anyways (could change that as well but might be out of scope) which is OK because it will only do that if the panel is opened. But a an array of arrays is actually simpler here than an map because you don't have to use nested loops to iterate over all filters and its maps straighter to an two arrays so the conversion is simpler. The dependency between filter and selector will still be clear. So I changed the approach to use a [[filter_a, selector_a_1], [filter_a, selector_a_2]] instead of a Map([filter_a, [selector_a_1, selector_a_2]]). What do you think? https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js#... include.preload.js:256: matched = true; On 2017/02/09 11:07:39, Sebastian Noack wrote: > It seems, the "match" variable would be unnecessary, if you break out of the > outer loop here, directly: > > nodes: for (let node of nodes) > { > for (let element of elements) > { > ... > break nodes; > } > } > } Done. https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js#... include.preload.js:519: if (!selectors) On 2017/02/09 11:07:39, Sebastian Noack wrote: > Why did you change this line? Note that an empty array evaluates to true. Done.
https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js#... include.preload.js:197: this.filters = new Map(); On 2017/02/10 10:46:46, wspee wrote: > On 2017/02/09 11:07:39, Sebastian Noack wrote: > > Why to use a a Map object? It seems the logic would be much simpler, if you > just > > have one array for selectors and one for the filters? > > If you have information that are are dependent from one another it makes sense > to also use a data structure to reflects that. The selectors belong to a filter > so it makes sense to use a key/value data structure for that. The performance > arguments we used against a map previously don't apply here because (as it > stands now) the ElementHidingTracer will hold all ElementHiding filters in > memory anyways (could change that as well but might be out of scope) which is OK > because it will only do that if the panel is opened. > > But a an array of arrays is actually simpler here than an map because you don't > have to use nested loops to iterate over all filters and its maps straighter to > an two arrays so the conversion is simpler. The dependency between filter and > selector will still be clear. > > So I changed the approach to use a [[filter_a, selector_a_1], [filter_a, > selector_a_2]] instead of a Map([filter_a, [selector_a_1, selector_a_2]]). > > What do you think? I'd generally agree, that data that have a mutual dependency should be stored in the same data structure. Though, an n-dimensional array isn't the most efficient data structure in JavaScript, as each array is a first-class object with notable memory boilerplate. But more importantly, elemHideEmulation.js works with two separate arrays, and translating these into a two-dimensional array (or Map previously) adds quite some complexity, with not much of a benefit since we access those data only once. Not to mention the leaky abstraction where you store selectors (redundantly) as filter when the filter is unknown.
On 2017/02/10 14:52:55, Sebastian Noack wrote: > https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js > File include.preload.js (right): > > https://codereview.adblockplus.org/29370970/diff/29374871/include.preload.js#... > include.preload.js:197: this.filters = new Map(); > On 2017/02/10 10:46:46, wspee wrote: > > On 2017/02/09 11:07:39, Sebastian Noack wrote: > > > Why to use a a Map object? It seems the logic would be much simpler, if you > > just > > > have one array for selectors and one for the filters? > > > > If you have information that are are dependent from one another it makes sense > > to also use a data structure to reflects that. The selectors belong to a > filter > > so it makes sense to use a key/value data structure for that. The performance > > arguments we used against a map previously don't apply here because (as it > > stands now) the ElementHidingTracer will hold all ElementHiding filters in > > memory anyways (could change that as well but might be out of scope) which is > OK > > because it will only do that if the panel is opened. > > > > But a an array of arrays is actually simpler here than an map because you > don't > > have to use nested loops to iterate over all filters and its maps straighter > to > > an two arrays so the conversion is simpler. The dependency between filter and > > selector will still be clear. > > > > So I changed the approach to use a [[filter_a, selector_a_1], [filter_a, > > selector_a_2]] instead of a Map([filter_a, [selector_a_1, selector_a_2]]). > > > > What do you think? > > I'd generally agree, that data that have a mutual dependency should be stored in > the same data structure. Though, an n-dimensional array isn't the most efficient > data structure in JavaScript, as each array is a first-class object with notable > memory boilerplate. But more importantly, elemHideEmulation.js works with two > separate arrays, and translating these into a two-dimensional array (or Map > previously) adds quite some complexity, with not much of a benefit since we > access those data only once. Not to mention the leaky abstraction where you > store selectors (redundantly) as filter when the filter is unknown. Done
https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js#... include.preload.js:211: this.trace(); Please correct me if I'm wrong, but it seems this is the only place where trace() is called. So if you just set the started property inside the trace() function, we wouldn't need this wrapper. https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js#... include.preload.js:228: Array.prototype.push.apply(this.selectors, selectors); As mentioned before, please use the rest operator instead of .apply(): this.selectors.push(...selectors); this.filters.push(...filters); https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js#... include.preload.js:238: let selector = selectors[i]; It seems "selector" and "filter" are only used once each. So these temporary variables seem unnecessary. Instead you could inline selectors[i]/filters[i]. https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js#... include.preload.js:519: Array.prototype.push.apply(filters, selectors); As indicated before, I'm not sure if it is a good idea to redundantly store the selectors as filters, when we don't know the actual filters. This is a leaky abstraction. While it works for the code above, it raises wrong assumptions. Not to mention the redundancy in the data structure. I'd rather create an array which holds the value of undefined for each selector we don't know the filter of (i.e. `new Array(selectors.length)`), and handle that case explicitly, when extracting the original selectors. Moreover, I feel that this logic belongs into the addFilter() method.
https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js#... include.preload.js:211: this.trace(); On 2017/02/13 15:09:17, Sebastian Noack wrote: > Please correct me if I'm wrong, but it seems this is the only place where > trace() is called. So if you just set the started property inside the trace() > function, we wouldn't need this wrapper. Done. https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js#... include.preload.js:228: Array.prototype.push.apply(this.selectors, selectors); On 2017/02/13 15:09:17, Sebastian Noack wrote: > As mentioned before, please use the rest operator instead of .apply(): > > this.selectors.push(...selectors); > this.filters.push(...filters); Done. https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js#... include.preload.js:238: let selector = selectors[i]; On 2017/02/13 15:09:17, Sebastian Noack wrote: > It seems "selector" and "filter" are only used once each. So these temporary > variables seem unnecessary. Instead you could inline selectors[i]/filters[i]. Done. https://codereview.adblockplus.org/29370970/diff/29375567/include.preload.js#... include.preload.js:519: Array.prototype.push.apply(filters, selectors); On 2017/02/13 15:09:17, Sebastian Noack wrote: > As indicated before, I'm not sure if it is a good idea to redundantly store the > selectors as filters, when we don't know the actual filters. This is a leaky > abstraction. While it works for the code above, it raises wrong assumptions. Not > to mention the redundancy in the data structure. > > I'd rather create an array which holds the value of undefined for each selector > we don't know the filter of (i.e. `new Array(selectors.length)`), and handle > that case explicitly, when extracting the original selectors. > > Moreover, I feel that this logic belongs into the addFilter() method. Done.
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:208: start: function() Do we even need the start method (and started flag)? What if we just put following back into the constructor: if (document.readyState == "loading") document.addEventListener("DOMContentLoaded", this.trace); But without the else branch. And then in addFilters(), instead of checking for started (which we'd no longer need then), we check for document.readyState: if (document.readyState != "loading") this.checkNodes([document], selectors, filters); Note that I also removed the timeout, which I think we can get rid of, if we just make sure to call addFilters() after adding the rules to the stylesheet. https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:216: addFilters: function(selectors, filters) I think this method should rather be called addSelectors() now, where it always takes selectors, and the corresponding filter are optional. https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:223: if (!filters) I'd move this to the top of the function. If we get rid of the timeout, we even have to. But regardless, I think it makes the code flow clearer. https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:589: this.elemHideEmulation.load(checkLoaded); Why did you move this inside this callback? That way "get-selectors" and elemHideEmulation.load() are no longer requested simultaneously, adding a delay.
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:232: matchedSelectors.push(selector); Renaming this variable to matchedFilters seems inappropriate, since we still extract the selector part of the filter, and only send that part. https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:208: start: function() On 2017/02/14 10:58:16, Sebastian Noack wrote: > Do we even need the start method (and started flag)? What if we just put > following back into the constructor: > > if (document.readyState == "loading") > document.addEventListener("DOMContentLoaded", this.trace); > > But without the else branch. And then in addFilters(), instead of checking for > started (which we'd no longer need then), we check for document.readyState: > > if (document.readyState != "loading") > this.checkNodes([document], selectors, filters); > > Note that I also removed the timeout, which I think we can get rid of, if we > just make sure to call addFilters() after adding the rules to the stylesheet. I just noticed that currently we have to call trace(), to setup the MutationObserver. But even if we separate that logic, it still seems simpler than the current approach.
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:232: matchedSelectors.push(selector); On 2017/02/14 11:27:58, Sebastian Noack wrote: > Renaming this variable to matchedFilters seems inappropriate, since we still > extract the selector part of the filter, and only send that part. In case of an ElementHidingEmulationFilter we want to send the filter not the selector. The selector array contains the constructed selector used to identify the element to hide and not the selector part of the filter. If we sent only the selector the panel is unable to identify which filter is responsible for a hidden element. So we send the filter and fallback to the selector if the filter is not available and therefore the name should be matchedFilters and not matchedSelectors, right? https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:208: start: function() On 2017/02/14 11:27:58, Sebastian Noack wrote: > On 2017/02/14 10:58:16, Sebastian Noack wrote: > > Do we even need the start method (and started flag)? What if we just put > > following back into the constructor: > > > > if (document.readyState == "loading") > > document.addEventListener("DOMContentLoaded", this.trace); > > > > But without the else branch. And then in addFilters(), instead of checking for > > started (which we'd no longer need then), we check for document.readyState: > > > > if (document.readyState != "loading") > > this.checkNodes([document], selectors, filters); > > > > Note that I also removed the timeout, which I think we can get rid of, if we > > just make sure to call addFilters() after adding the rules to the stylesheet. > > I just noticed that currently we have to call trace(), to setup the > MutationObserver. But even if we separate that logic, it still seems simpler > than the current approach. If we implement your suggestions wouldn't that mean that the tracer wouldn't trace anything if the dom is still loading if trace is called? We have to make sure that trace is called not only to set up the observer but also to call checkNodes for the whole document? https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:216: addFilters: function(selectors, filters) On 2017/02/14 10:58:16, Sebastian Noack wrote: > I think this method should rather be called addSelectors() now, where it always > takes selectors, and the corresponding filter are optional. Done. https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:223: if (!filters) On 2017/02/14 10:58:16, Sebastian Noack wrote: > I'd move this to the top of the function. If we get rid of the timeout, we even > have to. But regardless, I think it makes the code flow clearer. Done. https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:589: this.elemHideEmulation.load(checkLoaded); On 2017/02/14 10:58:16, Sebastian Noack wrote: > Why did you move this inside this callback? That way "get-selectors" and > elemHideEmulation.load() are no longer requested simultaneously, adding a delay. Hmm ... because this way checkLoaded is only called once and is stateless ... but I didn't take into account that sendMessage could be asynchronous. But I'm not sure if there is a way to take advantage of the async execution. Whether or not to trace elements depends on the response of get-selectors (response.trace) and there is not much you could do without knowing whether or not to activate the tracer?
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:232: matchedSelectors.push(selector); On 2017/02/15 14:09:57, wspee wrote: > On 2017/02/14 11:27:58, Sebastian Noack wrote: > > Renaming this variable to matchedFilters seems inappropriate, since we still > > extract the selector part of the filter, and only send that part. > > In case of an ElementHidingEmulationFilter we want to send the filter not the > selector. The selector array contains the constructed selector used to identify > the element to hide and not the selector part of the filter. If we sent only the > selector the panel is unable to identify which filter is responsible for a > hidden element. > > So we send the filter and fallback to the selector if the filter is not > available and therefore the name should be matchedFilters and not > matchedSelectors, right? We never send the actual filter, as we remove the part before including the ## if any. In fact, the string we are sending is the value of the ElemHideFilter or ElemHideEmulationFilter object's "selector" property. In case of element hiding emulation this value is still different from the resulting selector that is injected in the end. But calling it a filter isn't accurate either. If we'd need distinct terms for these things, I'd call them "raw/emulated selectors" and "generated selectors". But that doesn't matter much for the code here. https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:208: start: function() On 2017/02/15 14:09:57, wspee wrote: > On 2017/02/14 11:27:58, Sebastian Noack wrote: > > On 2017/02/14 10:58:16, Sebastian Noack wrote: > > > Do we even need the start method (and started flag)? What if we just put > > > following back into the constructor: > > > > > > if (document.readyState == "loading") > > > document.addEventListener("DOMContentLoaded", this.trace); > > > > > > But without the else branch. And then in addFilters(), instead of checking > for > > > started (which we'd no longer need then), we check for document.readyState: > > > > > > if (document.readyState != "loading") > > > this.checkNodes([document], selectors, filters); > > > > > > Note that I also removed the timeout, which I think we can get rid of, if we > > > just make sure to call addFilters() after adding the rules to the > stylesheet. > > > > I just noticed that currently we have to call trace(), to setup the > > MutationObserver. But even if we separate that logic, it still seems simpler > > than the current approach. > > If we implement your suggestions wouldn't that mean that the tracer wouldn't > trace anything if the dom is still loading if trace is called? > We have to make sure that trace is called not only to set up the observer but > also to call checkNodes for the whole document? We should defer tracing (i.e. traversing through the whole document and start observing further mutations) until when the DOM has been loaded. That is what the document.readyState check and the DOMContentLoaded listener has been here for in the first place. This is necessary because the MutationObserver doesn't reliably observe nodes added while the DOM is still being parsed. This depends a little bit on the browser (and its version) though. But even if the MutationObserver is fired for DOM fragments as they are parsed, it's still way faster to just wait until the DOM is ready and then check all nodes at once. With the suggested approach, we would still make sure everything is eventually traced, as addFilters() will keep adding selectors/filters to this.selectors/this.filters, which are then processed on DOMContentLoaded. Or if the DOM has already been loaded, we process these selectors/filters immediately. https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:589: this.elemHideEmulation.load(checkLoaded); On 2017/02/15 14:09:56, wspee wrote: > On 2017/02/14 10:58:16, Sebastian Noack wrote: > > Why did you move this inside this callback? That way "get-selectors" and > > elemHideEmulation.load() are no longer requested simultaneously, adding a > delay. > > Hmm ... because this way checkLoaded is only called once and is stateless ... > but I didn't take into account that sendMessage could be asynchronous. > > But I'm not sure if there is a way to take advantage of the async execution. > Whether or not to trace elements depends on the response of get-selectors > (response.trace) and there is not much you could do without knowing whether or > not to activate the tracer? Well, the whole point of having checkLoaded(), as opposed to passing an anonymous function here, or naming it "onLoaded" at least, is to synchronize sendMessage({type: "get-selectors"}, ...) and elemHideEmulation.load(...). First of all, the synchronization is necessary, as you already noticed, so that we know whether to trace element hiding, in order to keep track of the selectors/filters if necessary, before injecting any selectors. But there is also another benefit of the synchronization here. That is if element hiding gets reapplied (e.g. when using the "Block element" dialog from the icon menu), replacing the stylesheet happens in one go before the page is redrawn, so that you don't see any elements showing up shortly before they are hidden again. The same could be achieved by sending the messages sequentially, like you did. But this will cause a (small) delay, since the background page, after responding to the first message, has to wait for us to send the second message. But if we send the second message already while the first message hasn't been processed yet, like this code originally did, there is no delay in between both messages being processed by the background page. While I don't consider your approach an improvement, I might see some potential for improvement here. First of all, the synchronization adds some complexity, that would be great to get rid of, as you noticed. Moreover, while it makes sure that emulation filters are ready as soon as possible, it adds some delay for regular element hiding filter (which is the common case), with either approach (but even more with yours). So how about this: We setup the stylesheet and tracer, and start injecting selectors, as soon as we got a response for "get-selectors". And then we asynchronously retrieve and apply the emulation filters. This will get rid of the synchronization and minimizes the delay for regular element hiding filters as much as possible. The only downside then is that we have a visible delay in between regular element hiding filters, and emulation filters, being applied. However, in the common case this is preferable, if that means regular element hiding filters become applied earlier. And for the case where the "Block element" dialog causes element hiding to be reapplied, this would be still acceptable, considering the performance improvement in the common case and that this only effect emulation filters.
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:232: matchedSelectors.push(selector); On 2017/02/15 18:49:03, Sebastian Noack wrote: > On 2017/02/15 14:09:57, wspee wrote: > > On 2017/02/14 11:27:58, Sebastian Noack wrote: > > > Renaming this variable to matchedFilters seems inappropriate, since we still > > > extract the selector part of the filter, and only send that part. > > > > In case of an ElementHidingEmulationFilter we want to send the filter not the > > selector. The selector array contains the constructed selector used to > identify > > the element to hide and not the selector part of the filter. If we sent only > the > > selector the panel is unable to identify which filter is responsible for a > > hidden element. > > > > So we send the filter and fallback to the selector if the filter is not > > available and therefore the name should be matchedFilters and not > > matchedSelectors, right? > > We never send the actual filter, as we remove the part before including the ## > if any. In fact, the string we are sending is the value of the ElemHideFilter or > ElemHideEmulationFilter object's "selector" property. In case of element hiding > emulation this value is still different from the resulting selector that is > injected in the end. But calling it a filter isn't accurate either. If we'd need > distinct terms for these things, I'd call them "raw/emulated selectors" and > "generated selectors". But that doesn't matter much for the code here. No, that's the change I made, for ElemHideEmulationFilter we send the actual filter not the selector and neither the generated selector. http://i.imgur.com/9pj1UOJ.png https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:208: start: function() On 2017/02/15 18:49:03, Sebastian Noack wrote: > On 2017/02/15 14:09:57, wspee wrote: > > On 2017/02/14 11:27:58, Sebastian Noack wrote: > > > On 2017/02/14 10:58:16, Sebastian Noack wrote: > > > > Do we even need the start method (and started flag)? What if we just put > > > > following back into the constructor: > > > > > > > > if (document.readyState == "loading") > > > > document.addEventListener("DOMContentLoaded", this.trace); > > > > > > > > But without the else branch. And then in addFilters(), instead of checking > > for > > > > started (which we'd no longer need then), we check for > document.readyState: > > > > > > > > if (document.readyState != "loading") > > > > this.checkNodes([document], selectors, filters); > > > > > > > > Note that I also removed the timeout, which I think we can get rid of, if > we > > > > just make sure to call addFilters() after adding the rules to the > > stylesheet. > > > > > > I just noticed that currently we have to call trace(), to setup the > > > MutationObserver. But even if we separate that logic, it still seems simpler > > > than the current approach. > > > > If we implement your suggestions wouldn't that mean that the tracer wouldn't > > trace anything if the dom is still loading if trace is called? > > We have to make sure that trace is called not only to set up the observer but > > also to call checkNodes for the whole document? > > We should defer tracing (i.e. traversing through the whole document and start > observing further mutations) until when the DOM has been loaded. That is what > the document.readyState check and the DOMContentLoaded listener has been here > for in the first place. > > This is necessary because the MutationObserver doesn't reliably observe nodes > added while the DOM is still being parsed. This depends a little bit on the > browser (and its version) though. But even if the MutationObserver is fired for > DOM fragments as they are parsed, it's still way faster to just wait until the > DOM is ready and then check all nodes at once. > > With the suggested approach, we would still make sure everything is eventually > traced, as addFilters() will keep adding selectors/filters to > this.selectors/this.filters, which are then processed on DOMContentLoaded. Or if > the DOM has already been loaded, we process these selectors/filters immediately. Done. https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:589: this.elemHideEmulation.load(checkLoaded); On 2017/02/15 18:49:03, Sebastian Noack wrote: > On 2017/02/15 14:09:56, wspee wrote: > > On 2017/02/14 10:58:16, Sebastian Noack wrote: > > > Why did you move this inside this callback? That way "get-selectors" and > > > elemHideEmulation.load() are no longer requested simultaneously, adding a > > delay. > > > > Hmm ... because this way checkLoaded is only called once and is stateless ... > > but I didn't take into account that sendMessage could be asynchronous. > > > > But I'm not sure if there is a way to take advantage of the async execution. > > Whether or not to trace elements depends on the response of get-selectors > > (response.trace) and there is not much you could do without knowing whether or > > not to activate the tracer? > > Well, the whole point of having checkLoaded(), as opposed to passing an > anonymous function here, or naming it "onLoaded" at least, is to synchronize > sendMessage({type: "get-selectors"}, ...) and elemHideEmulation.load(...). > > First of all, the synchronization is necessary, as you already noticed, > so that we know whether to trace element hiding, in order to keep track of the > selectors/filters if necessary, before injecting any selectors. But there is > also another benefit of the synchronization here. That is if element hiding gets > reapplied (e.g. when using the "Block element" dialog from the icon menu), > replacing the stylesheet happens in one go before the page is redrawn, so that > you don't see any elements showing up shortly before they are hidden again. > > The same could be achieved by sending the messages sequentially, like you did. > But this will cause a (small) delay, since the background page, after responding > to the first message, has to wait for us to send the second message. But if we > send the second message already while the first message hasn't been processed > yet, like this code originally did, there is no delay in between both messages > being processed by the background page. > > While I don't consider your approach an improvement, I might see some potential > for improvement here. First of all, the synchronization adds some complexity, > that would be great to get rid of, as you noticed. Moreover, while it makes sure > that emulation filters are ready as soon as possible, it adds some delay for > regular element hiding filter (which is the common case), with either approach > (but even more with yours). So how about this: We setup the stylesheet and > tracer, and start injecting selectors, as soon as we got a response for > "get-selectors". And then we asynchronously retrieve and apply the emulation > filters. This will get rid of the synchronization and minimizes the delay for > regular element hiding filters as much as possible. The only downside then is > that we have a visible delay in between regular element hiding filters, and > emulation filters, being applied. However, in the common case this is > preferable, if that means regular element hiding filters become applied earlier. > And for the case where the "Block element" dialog causes element hiding to be > reapplied, this would be still acceptable, considering the performance > improvement in the common case and that this only effect emulation filters. Like so?
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:232: matchedSelectors.push(selector); On 2017/02/17 10:51:38, wspee wrote: > On 2017/02/15 18:49:03, Sebastian Noack wrote: > > On 2017/02/15 14:09:57, wspee wrote: > > > On 2017/02/14 11:27:58, Sebastian Noack wrote: > > > > Renaming this variable to matchedFilters seems inappropriate, since we > still > > > > extract the selector part of the filter, and only send that part. > > > > > > In case of an ElementHidingEmulationFilter we want to send the filter not > the > > > selector. The selector array contains the constructed selector used to > > identify > > > the element to hide and not the selector part of the filter. If we sent only > > the > > > selector the panel is unable to identify which filter is responsible for a > > > hidden element. > > > > > > So we send the filter and fallback to the selector if the filter is not > > > available and therefore the name should be matchedFilters and not > > > matchedSelectors, right? > > > > We never send the actual filter, as we remove the part before including the ## > > if any. In fact, the string we are sending is the value of the ElemHideFilter > or > > ElemHideEmulationFilter object's "selector" property. In case of element > hiding > > emulation this value is still different from the resulting selector that is > > injected in the end. But calling it a filter isn't accurate either. If we'd > need > > distinct terms for these things, I'd call them "raw/emulated selectors" and > > "generated selectors". But that doesn't matter much for the code here. > > No, that's the change I made, for ElemHideEmulationFilter we send the actual > filter not the selector and neither the generated selector. > > http://i.imgur.com/9pj1UOJ.png And what do you do in the line below? ;) matchedFilters.push(filter.replace(/^.*?##/, "")); You only store the (raw) selector part, and eventually send it. https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:589: this.elemHideEmulation.load(checkLoaded); On 2017/02/17 10:51:38, wspee wrote: > On 2017/02/15 18:49:03, Sebastian Noack wrote: > > On 2017/02/15 14:09:56, wspee wrote: > > > On 2017/02/14 10:58:16, Sebastian Noack wrote: > > > > Why did you move this inside this callback? That way "get-selectors" and > > > > elemHideEmulation.load() are no longer requested simultaneously, adding a > > > delay. > > > > > > Hmm ... because this way checkLoaded is only called once and is stateless > ... > > > but I didn't take into account that sendMessage could be asynchronous. > > > > > > But I'm not sure if there is a way to take advantage of the async execution. > > > Whether or not to trace elements depends on the response of get-selectors > > > (response.trace) and there is not much you could do without knowing whether > or > > > not to activate the tracer? > > > > Well, the whole point of having checkLoaded(), as opposed to passing an > > anonymous function here, or naming it "onLoaded" at least, is to synchronize > > sendMessage({type: "get-selectors"}, ...) and elemHideEmulation.load(...). > > > > First of all, the synchronization is necessary, as you already noticed, > > so that we know whether to trace element hiding, in order to keep track of the > > selectors/filters if necessary, before injecting any selectors. But there is > > also another benefit of the synchronization here. That is if element hiding > gets > > reapplied (e.g. when using the "Block element" dialog from the icon menu), > > replacing the stylesheet happens in one go before the page is redrawn, so that > > you don't see any elements showing up shortly before they are hidden again. > > > > The same could be achieved by sending the messages sequentially, like you did. > > But this will cause a (small) delay, since the background page, after > responding > > to the first message, has to wait for us to send the second message. But if we > > send the second message already while the first message hasn't been processed > > yet, like this code originally did, there is no delay in between both messages > > being processed by the background page. > > > > While I don't consider your approach an improvement, I might see some > potential > > for improvement here. First of all, the synchronization adds some complexity, > > that would be great to get rid of, as you noticed. Moreover, while it makes > sure > > that emulation filters are ready as soon as possible, it adds some delay for > > regular element hiding filter (which is the common case), with either approach > > (but even more with yours). So how about this: We setup the stylesheet and > > tracer, and start injecting selectors, as soon as we got a response for > > "get-selectors". And then we asynchronously retrieve and apply the emulation > > filters. This will get rid of the synchronization and minimizes the delay for > > regular element hiding filters as much as possible. The only downside then is > > that we have a visible delay in between regular element hiding filters, and > > emulation filters, being applied. However, in the common case this is > > preferable, if that means regular element hiding filters become applied > earlier. > > And for the case where the "Block element" dialog causes element hiding to be > > reapplied, this would be still acceptable, considering the performance > > improvement in the common case and that this only effect emulation filters. > > Like so? Almost. ;) https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#... include.preload.js:212: addSelectors: function(selectors, filters) Nit: The indentation is one space off here. https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#... include.preload.js:501: if (!selectors.length) I find the purpose of this check more clear when comparing the length explicitly against zero (as this code initially did), rather than checking whether the length evaluates to false. It's also more consistent with our existing code. https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#... include.preload.js:541: for (var i = 0; i < preparedSelectors.length; i += this.selectorGroupSize) Wouldn't preparedSelectors be undefined, on browsers where ShadowDOM does not exist, and therefore this.shadow is null, and therefore the conditional block in which preparedSelectors is defined won't be executed? https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#... include.preload.js:554: if (this.tracer) We should still wait until we have a response for "get-selectors" at least, before removing the stylesheet and tracer, in order to minimize the side effects when element hiding is reapplied. So please move this logic inside the callback. https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#... include.preload.js:570: this.elemHideEmulation.load( The of the load() and apply() method did only exist in the first place, for the synchronization which we just got rid off. So while changing elemHideEmulation.js in adblockpluscore anyway, mind merging those two functions, to simplify the logic here (as well as in Adblock Plus for Firefox, where we do the same).
https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#... include.preload.js:212: addSelectors: function(selectors, filters) On 2017/02/17 13:55:31, Sebastian Noack wrote: > Nit: The indentation is one space off here. Done. https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#... include.preload.js:501: if (!selectors.length) On 2017/02/17 13:55:31, Sebastian Noack wrote: > I find the purpose of this check more clear when comparing the length explicitly > against zero (as this code initially did), rather than checking whether the > length evaluates to false. It's also more consistent with our existing code. Done. https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#... include.preload.js:541: for (var i = 0; i < preparedSelectors.length; i += this.selectorGroupSize) On 2017/02/17 13:55:31, Sebastian Noack wrote: > Wouldn't preparedSelectors be undefined, on browsers where ShadowDOM does not > exist, and therefore this.shadow is null, and therefore the conditional block in > which preparedSelectors is defined won't be executed? Done. https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#... include.preload.js:554: if (this.tracer) On 2017/02/17 13:55:31, Sebastian Noack wrote: > We should still wait until we have a response for "get-selectors" at least, > before removing the stylesheet and tracer, in order to minimize the side effects > when element hiding is reapplied. So please move this logic inside the callback. Done. https://codereview.adblockplus.org/29370970/diff/29375863/include.preload.js#... include.preload.js:570: this.elemHideEmulation.load( On 2017/02/17 13:55:31, Sebastian Noack wrote: > The of the load() and apply() method did only exist in the first place, for the > synchronization which we just got rid off. So while changing > elemHideEmulation.js in adblockpluscore anyway, mind merging those two > functions, to simplify the logic here (as well as in Adblock Plus for Firefox, > where we do the same). Done, see: https://codereview.adblockplus.org/29375888/ https://codereview.adblockplus.org/29375891/
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:232: matchedSelectors.push(selector); On 2017/02/17 13:55:30, Sebastian Noack wrote: > On 2017/02/17 10:51:38, wspee wrote: > > On 2017/02/15 18:49:03, Sebastian Noack wrote: > > > On 2017/02/15 14:09:57, wspee wrote: > > > > On 2017/02/14 11:27:58, Sebastian Noack wrote: > > > > > Renaming this variable to matchedFilters seems inappropriate, since we > > still > > > > > extract the selector part of the filter, and only send that part. > > > > > > > > In case of an ElementHidingEmulationFilter we want to send the filter not > > the > > > > selector. The selector array contains the constructed selector used to > > > identify > > > > the element to hide and not the selector part of the filter. If we sent > only > > > the > > > > selector the panel is unable to identify which filter is responsible for a > > > > hidden element. > > > > > > > > So we send the filter and fallback to the selector if the filter is not > > > > available and therefore the name should be matchedFilters and not > > > > matchedSelectors, right? > > > > > > We never send the actual filter, as we remove the part before including the > ## > > > if any. In fact, the string we are sending is the value of the > ElemHideFilter > > or > > > ElemHideEmulationFilter object's "selector" property. In case of element > > hiding > > > emulation this value is still different from the resulting selector that is > > > injected in the end. But calling it a filter isn't accurate either. If we'd > > need > > > distinct terms for these things, I'd call them "raw/emulated selectors" and > > > "generated selectors". But that doesn't matter much for the code here. > > > > No, that's the change I made, for ElemHideEmulationFilter we send the actual > > filter not the selector and neither the generated selector. > > > > http://i.imgur.com/9pj1UOJ.png > > And what do you do in the line below? ;) > > matchedFilters.push(filter.replace(/^.*?##/, "")); > > You only store the (raw) selector part, and eventually send it. This comment hasn't been resolved yet. https://codereview.adblockplus.org/29370970/diff/29375885/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375885/include.preload.js#... include.preload.js:558: ext.backgroundPage.sendMessage({type: "get-selectors"}, selectors => Perhaps, call the argument "response", as it's not the selectors, but a complex object which stores the the selectors in its "selectors" property. Hence "response.selectors" would look less awkward than "selectors.selectors". https://codereview.adblockplus.org/29370970/diff/29375885/include.preload.js#... include.preload.js:571: if (selectors.selectors) This check is redundant, as "selectors" is always an array, which always evaluates to true.
https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375678/include.preload.js#... include.preload.js:232: matchedSelectors.push(selector); On 2017/02/18 11:41:18, Sebastian Noack wrote: > On 2017/02/17 13:55:30, Sebastian Noack wrote: > > On 2017/02/17 10:51:38, wspee wrote: > > > On 2017/02/15 18:49:03, Sebastian Noack wrote: > > > > On 2017/02/15 14:09:57, wspee wrote: > > > > > On 2017/02/14 11:27:58, Sebastian Noack wrote: > > > > > > Renaming this variable to matchedFilters seems inappropriate, since we > > > still > > > > > > extract the selector part of the filter, and only send that part. > > > > > > > > > > In case of an ElementHidingEmulationFilter we want to send the filter > not > > > the > > > > > selector. The selector array contains the constructed selector used to > > > > identify > > > > > the element to hide and not the selector part of the filter. If we sent > > only > > > > the > > > > > selector the panel is unable to identify which filter is responsible for > a > > > > > hidden element. > > > > > > > > > > So we send the filter and fallback to the selector if the filter is not > > > > > available and therefore the name should be matchedFilters and not > > > > > matchedSelectors, right? > > > > > > > > We never send the actual filter, as we remove the part before including > the > > ## > > > > if any. In fact, the string we are sending is the value of the > > ElemHideFilter > > > or > > > > ElemHideEmulationFilter object's "selector" property. In case of element > > > hiding > > > > emulation this value is still different from the resulting selector that > is > > > > injected in the end. But calling it a filter isn't accurate either. If > we'd > > > need > > > > distinct terms for these things, I'd call them "raw/emulated selectors" > and > > > > "generated selectors". But that doesn't matter much for the code here. > > > > > > No, that's the change I made, for ElemHideEmulationFilter we send the actual > > > filter not the selector and neither the generated selector. > > > > > > http://i.imgur.com/9pj1UOJ.png > > > > And what do you do in the line below? ;) > > > > matchedFilters.push(filter.replace(/^.*?##/, "")); > > > > You only store the (raw) selector part, and eventually send it. > > This comment hasn't been resolved yet. Done, you were right after all ;). https://codereview.adblockplus.org/29370970/diff/29375885/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375885/include.preload.js#... include.preload.js:558: ext.backgroundPage.sendMessage({type: "get-selectors"}, selectors => On 2017/02/18 11:41:18, Sebastian Noack wrote: > Perhaps, call the argument "response", as it's not the selectors, but a complex > object which stores the the selectors in its "selectors" property. Hence > "response.selectors" would look less awkward than "selectors.selectors". Done. https://codereview.adblockplus.org/29370970/diff/29375885/include.preload.js#... include.preload.js:571: if (selectors.selectors) On 2017/02/18 11:41:19, Sebastian Noack wrote: > This check is redundant, as "selectors" is always an array, which always > evaluates to true. Done.
The changes basically look good to me. But two more things (besides that nit): 1. You have to rebase 2. We have to land the adblockpluscore changes first, and update the respective dependency here. https://codereview.adblockplus.org/29370970/diff/29375896/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29375896/include.preload.js#... include.preload.js:574: Nit: I'd remove the blank line here (as it was before, see above).
https://codereview.adblockplus.org/29370970/diff/29375896/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375896/include.preload.js#... include.preload.js:215: filters = new Array(selectors.length); In the end, it turns out the logic would be somewhat simpler when we just pass the selectors as filters, instead of creating a new array here, so that we also only need to consider the filters array when extracting the raw selector, like you originally did. I was initially against that approach because that practice exposes a slightly leaky abstraction. (You would assume that the filters array stores filters, while most of the time it will be just the selector part, potentially raising wrong assumptions.) But seeing the result of this change, I notice that we merely moved the ambiguous logic from one location to another, while adding additional complexity. Hence I'm preferring your original approach now. I'm very sorry for changing my mind. Something that seems fishy on first glance, might turn out to be still the best compromise in the end, which is hard to tell before seeing the changed code. So please never take mine (or anybody's) feedback on a review for granted. Often, it's just a first impression.
https://codereview.adblockplus.org/29370970/diff/29375896/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29375896/include.preload.js#... include.preload.js:215: filters = new Array(selectors.length); On 2017/02/20 12:19:55, Sebastian Noack wrote: > In the end, it turns out the logic would be somewhat simpler when we just pass > the selectors as filters, instead of creating a new array here, so that we also > only need to consider the filters array when extracting the raw selector, like > you originally did. > > I was initially against that approach because that practice exposes a slightly > leaky abstraction. (You would assume that the filters array stores filters, > while most of the time it will be just the selector part, potentially raising > wrong assumptions.) But seeing the result of this change, I notice that we > merely moved the ambiguous logic from one location to another, while adding > additional complexity. Hence I'm preferring your original approach now. > > I'm very sorry for changing my mind. Something that seems fishy on first glance, > might turn out to be still the best compromise in the end, which is hard to tell > before seeing the changed code. So please never take mine (or anybody's) > feedback on a review for granted. Often, it's just a first impression. NP, Done.
https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js#... include.preload.js:570: Nit: I'd remove the blank line here (as it was before). https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js#... include.preload.js:237: let filter = filters[i] || selectors[i]; This is no longer necessary. You don't have to consider selectors here anymore. Since there will always be an entry in filters.
https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js#... include.preload.js:570: On 2017/02/23 11:08:22, Sebastian Noack wrote: > Nit: I'd remove the blank line here (as it was before). Done. https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js#... include.preload.js:237: let filter = filters[i] || selectors[i]; On 2017/02/23 11:08:23, Sebastian Noack wrote: > This is no longer necessary. You don't have to consider selectors here anymore. > Since there will always be an entry in filters. Done.
https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js#... include.preload.js:566: this.addSelectors(response.selectors, response.selectors); I wonder whether it would be nicer to handle this inside the addSelectors() method, like that: this.tracer.addSelectors(selectors, filters || selectors); That way we wouldn't have to duplicate the property lookup, and the semantics of the second argument can be completely ignored, here. Also the code would be slightly more concise. On the other hand, if we explicitly pass the selectors as filters here, we avoid a logical condition. Not sure which is more preferable.
LGTM. I didn't see that you already uploaded a new patch. But let me know what you think about my last comment though? As I said, I'm fine either way.
https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29370970/diff/29376871/include.preload.js#... include.preload.js:566: this.addSelectors(response.selectors, response.selectors); On 2017/02/23 11:24:03, Sebastian Noack wrote: > I wonder whether it would be nicer to handle this inside the addSelectors() > method, like that: > > this.tracer.addSelectors(selectors, filters || selectors); > > That way we wouldn't have to duplicate the property lookup, and the semantics of > the second argument can be completely ignored, here. Also the code would be > slightly more concise. On the other hand, if we explicitly pass the selectors as > filters here, we avoid a logical condition. Not sure which is more preferable. Done.
Still LGTM. Could you please send me the patch? 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
