Issue 29372676: [adblockpluscore] Issue 4825 - Let ElemHideEmulation associate selector and filter when calling add… (Closed)
Description[adblockpluscore] Issue 4825 - Let ElemHideEmulation associate selector and filter when calling add…
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use map instead of object #
Total comments: 2
Patch Set 3 : No longer create temp array and use for of #
Total comments: 4
Patch Set 4 : Removed unnecessary variable and use let instead of var #
Total comments: 6
Patch Set 5 : Changed map to two arrays #MessagesTotal messages: 12
https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elem... chrome/content/elemHideEmulation.js:100: var selectors = [] Nit: Missing semicolon https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elem... chrome/content/elemHideEmulation.js:106: filters[pattern.text] = selectors; While changing the API here anyway, it might be a good opportunity to turn filters into a Map() object, which according to our coding practices, is preferred over misusing plain objects as hashtable, in modern code.
https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elem... chrome/content/elemHideEmulation.js:100: var selectors = [] On 2017/02/06 14:59:21, Sebastian Noack wrote: > Nit: Missing semicolon Done. https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elem... chrome/content/elemHideEmulation.js:106: filters[pattern.text] = selectors; On 2017/02/06 14:59:21, Sebastian Noack wrote: > While changing the API here anyway, it might be a good opportunity to turn > filters into a Map() object, which according to our coding practices, is > preferred over misusing plain objects as hashtable, in modern code. Done.
https://codereview.adblockplus.org/29372676/diff/29374665/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374665/chrome/content/elem... chrome/content/elemHideEmulation.js:107: Array.prototype.push.apply(filters.get(pattern.text), selectors); Creating a temporary array, just to merge it with another array, seems unnecessary, here. How about following approach? let selectors = filters.get(pattern.text); if (!selectors) { selectors = []; filters.set(pattern.text, selectors); } for (let subSelector of subSelectors) selectors.push(pattern.prefix + subSelector + pattern.suffix); Note that while on it, I also replaced the for(;;) loop with a for..of loop, for simplicity and compliance with our coding practices for modern code. You might want to do the same below. And by the way, if you should ever need to pass variable arguments to a function, in modern code, be aware of the rest operator; i.e. `o.f(...vargs)` is equivalent to `f.apply(o, vargs)`, but much more readable.
https://codereview.adblockplus.org/29372676/diff/29374665/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374665/chrome/content/elem... chrome/content/elemHideEmulation.js:107: Array.prototype.push.apply(filters.get(pattern.text), selectors); On 2017/02/07 12:35:59, Sebastian Noack wrote: > Creating a temporary array, just to merge it with another array, seems > unnecessary, here. How about following approach? > > let selectors = filters.get(pattern.text); > if (!selectors) > { > selectors = []; > filters.set(pattern.text, selectors); > } > > for (let subSelector of subSelectors) > selectors.push(pattern.prefix + subSelector + pattern.suffix); > > Note that while on it, I also replaced the for(;;) loop with a for..of loop, for > simplicity and compliance with our coding practices for modern code. You might > want to do the same below. > > And by the way, if you should ever need to pass variable arguments to a > function, in modern code, be aware of the rest operator; i.e. `o.f(...vargs)` is > equivalent to `f.apply(o, vargs)`, but much more readable. Done.
https://codereview.adblockplus.org/29372676/diff/29374670/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374670/chrome/content/elem... chrome/content/elemHideEmulation.js:105: let subSelectors = splitSelector(rule.selectorText); The temporary variable is no longer necessary, as thanks to for..of loop syntax the value only needs to be accessed one, so that the function call can be put directly into the loop head. https://codereview.adblockplus.org/29372676/diff/29374670/chrome/content/elem... chrome/content/elemHideEmulation.js:115: var filters = new Map(); While you took the liberty to convert var-statements to let above, mind doing the same here as well? (However, I wouldn't touch any functions that aren't matter to this change though, in the same patch.)
https://codereview.adblockplus.org/29372676/diff/29374670/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374670/chrome/content/elem... chrome/content/elemHideEmulation.js:105: let subSelectors = splitSelector(rule.selectorText); On 2017/02/07 14:55:53, Sebastian Noack wrote: > The temporary variable is no longer necessary, as thanks to for..of loop syntax > the value only needs to be accessed one, so that the function call can be put > directly into the loop head. Done. https://codereview.adblockplus.org/29372676/diff/29374670/chrome/content/elem... chrome/content/elemHideEmulation.js:115: var filters = new Map(); On 2017/02/07 14:55:53, Sebastian Noack wrote: > While you took the liberty to convert var-statements to let above, mind doing > the same here as well? (However, I wouldn't touch any functions that aren't > matter to this change though, in the same patch.) Done.
https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elem... chrome/content/elemHideEmulation.js:93: Nit: The blank line added here is unrelated at least, and arguably doesn't make the code any more readable. https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elem... chrome/content/elemHideEmulation.js:117: this.addSelectorsFunc(filters); I just realized, the implications for when this function is called for regular element hiding. In that case we just have a plain array of selectors. While we could potentially always pass a Map object, we don't need the addional metadata for regular element hiding, and performance is much more a concern there than when handling emulation filters. So I think the first argument should still be a plain array of selectors, and a second optional argument should specify the addional metadata in case of element hiding emulation. However, if we pass just the selectors as first argument and then the current filters map as second argument, those data structures will have redundancy, which is not great. So it seems the most appropriate data structure for the second argument would be an array of filters that correspond to the selectors in the first argument. Internally, you may use a Map object, using the active selectors as keys, and the corresponding filter as the respective value. Does that sound about right? I may still have missed something.
https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elem... chrome/content/elemHideEmulation.js:117: this.addSelectorsFunc(filters); On 2017/02/07 15:37:57, Sebastian Noack wrote: > I just realized, the implications for when this function is called for regular > element hiding. In that case we just have a plain array of selectors. While we > could potentially always pass a Map object, we don't need the addional metadata > for regular element hiding, and performance is much more a concern there than > when handling emulation filters. So I think the first argument should still be a > plain array of selectors, and a second optional argument should specify the > addional metadata in case of element hiding emulation. > > However, if we pass just the selectors as first argument and then the current > filters map as second argument, those data structures will have redundancy, > which is not great. So it seems the most appropriate data structure for the > second argument would be an array of filters that correspond to the selectors in > the first argument. Internally, you may use a Map object, using the active > selectors as keys, and the corresponding filter as the respective value. > > Does that sound about right? I may still have missed something. > Another advantage of this approach is, that the API remains backwards compatible. Instead of a unique list of the corresponding filters in arbitrary order, you merely get a list of filters whose index match their resulting selector. So Adblock Plus for Firefox will just keep working with its current limitations, without further changes. And that should be good enough, as it gets replaced with the code in adblockpluschrome anyway, later this year. The only potential limitation I can see with this approach is, that if more than one emulation filter will result into the same selector, only one of the filters gets logged. But that is consistent with blocking filters, where it has never been an issue. If a request matches more than one blocking filter, the matcher bails after the first match, and only the first filter match is recorded.
https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elem... chrome/content/elemHideEmulation.js:117: this.addSelectorsFunc(filters); On 2017/02/07 15:59:56, Sebastian Noack wrote: > On 2017/02/07 15:37:57, Sebastian Noack wrote: > > I just realized, the implications for when this function is called for regular > > element hiding. In that case we just have a plain array of selectors. While we > > could potentially always pass a Map object, we don't need the addional > metadata > > for regular element hiding, and performance is much more a concern there than > > when handling emulation filters. So I think the first argument should still be > a > > plain array of selectors, and a second optional argument should specify the > > addional metadata in case of element hiding emulation. > > > > However, if we pass just the selectors as first argument and then the current > > filters map as second argument, those data structures will have redundancy, > > which is not great. So it seems the most appropriate data structure for the > > second argument would be an array of filters that correspond to the selectors > in > > the first argument. Internally, you may use a Map object, using the active > > selectors as keys, and the corresponding filter as the respective value. > > > > Does that sound about right? I may still have missed something. > > > > Another advantage of this approach is, that the API remains backwards > compatible. Instead of a unique list of the corresponding filters in arbitrary > order, you merely get a list of filters whose index match their resulting > selector. So Adblock Plus for Firefox will just keep working with its current > limitations, without further changes. And that should be good enough, as it gets > replaced with the code in adblockpluschrome anyway, later this year. > > The only potential limitation I can see with this approach is, that if more than > one emulation filter will result into the same selector, only one of the filters > gets logged. But that is consistent with blocking filters, where it has never > been an issue. If a request matches more than one blocking filter, the matcher > bails after the first match, and only the first filter match is recorded. Perhaps even better, just use two arrays (instead of a Map object), one for the selectors, and one for the filters. If a selector is generated, also add the filter, and just pass both arrays to addSelectorsFunc(). This would be even simpler, and in the case there are multiple filters that result into the same selector, both are logged.
https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elem... chrome/content/elemHideEmulation.js:93: On 2017/02/07 15:37:57, Sebastian Noack wrote: > Nit: The blank line added here is unrelated at least, and arguably doesn't make > the code any more readable. Done. https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elem... chrome/content/elemHideEmulation.js:117: this.addSelectorsFunc(filters); On 2017/02/08 07:12:29, Sebastian Noack wrote: > On 2017/02/07 15:59:56, Sebastian Noack wrote: > > On 2017/02/07 15:37:57, Sebastian Noack wrote: > > > I just realized, the implications for when this function is called for > regular > > > element hiding. In that case we just have a plain array of selectors. While > we > > > could potentially always pass a Map object, we don't need the addional > > metadata > > > for regular element hiding, and performance is much more a concern there > than > > > when handling emulation filters. So I think the first argument should still > be > > a > > > plain array of selectors, and a second optional argument should specify the > > > addional metadata in case of element hiding emulation. > > > > > > However, if we pass just the selectors as first argument and then the > current > > > filters map as second argument, those data structures will have redundancy, > > > which is not great. So it seems the most appropriate data structure for the > > > second argument would be an array of filters that correspond to the > selectors > > in > > > the first argument. Internally, you may use a Map object, using the active > > > selectors as keys, and the corresponding filter as the respective value. > > > > > > Does that sound about right? I may still have missed something. > > > > > > > Another advantage of this approach is, that the API remains backwards > > compatible. Instead of a unique list of the corresponding filters in arbitrary > > order, you merely get a list of filters whose index match their resulting > > selector. So Adblock Plus for Firefox will just keep working with its current > > limitations, without further changes. And that should be good enough, as it > gets > > replaced with the code in adblockpluschrome anyway, later this year. > > > > The only potential limitation I can see with this approach is, that if more > than > > one emulation filter will result into the same selector, only one of the > filters > > gets logged. But that is consistent with blocking filters, where it has never > > been an issue. If a request matches more than one blocking filter, the matcher > > bails after the first match, and only the first filter match is recorded. > > Perhaps even better, just use two arrays (instead of a Map object), one for the > selectors, and one for the filters. If a selector is generated, also add the > filter, and just pass both arrays to addSelectorsFunc(). This would be even > simpler, and in the case there are multiple filters that result into the same > selector, both are logged. Done.
LGTM. But since Wladimir is module owner for core, he should be aware of this change at least.
LGTM 
|
