include.preload.js - Issue 29893559: Issue 6999 - Generate style sheets in background page

Unified Diff: include.preload.js

Issue 29893559: Issue 6999 - Generate style sheets in background page (Closed) Base URL: https://hg.adblockplus.org/adblockpluschrome/

Patch Set: Fix comment wrapping Created Sept. 29, 2018, 10:43 a.m.

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: include.preload.js

===================================================================

--- a/include.preload.js

+++ b/include.preload.js

@@ -220,17 +220,17 @@

{

if (collapse)

{

if (selector)

{

if (!collapsingSelectors.has(selector))

{

collapsingSelectors.add(selector);

- contentFiltering.addSelectors([selector], null, "collapsing", true);

+ contentFiltering.addSelectors([selector], "collapsing", true);

}

else

{

hideElement(element);

}

@@ -392,39 +392,24 @@

clearTimeout(this.timeout);

}

};

function ContentFiltering()

{

this.styles = new Map();

this.tracer = null;

- this.inline = true;

Sebastian Noack 2018/09/29 14:36:13 Nit: The blank line here no longer seems to help t

- this.elemHideEmulation = new ElemHideEmulation(

- () => {},

- this.hideElements.bind(this)

- );

+ this.elemHideEmulation = new ElemHideEmulation(this.hideElements.bind(this));

}

ContentFiltering.prototype = {

- selectorGroupSize: 1024,

- addSelectorsInline(selectors, groupName, appendOnly = false)

+ addStyleSheetInline(styleSheet, groupName = "standard", appendOnly = false)

{

let style = this.styles.get(groupName);

- if (style && !appendOnly)

- {

- while (style.sheet.cssRules.length > 0)

- style.sheet.deleteRule(0);

- }

- if (selectors.length == 0)

- return;

if (!style)

{

// Create <style> element lazily, only if we add styles. Add it to

// the <head> or <html> element. If we have injected a style element

// before that has been removed (the sheet property is null), create a

// new one.

style = document.createElement("style");

(document.head || document.documentElement).appendChild(style);

@@ -434,64 +419,46 @@

// In that case the sheet property may stay null, after adding the

// <style> element.

if (!style.sheet)

return;

this.styles.set(groupName, style);

}

- // Chromium's Blink engine supports only up to 8,192 simple selectors, and

- // even fewer compound selectors, in a rule. The exact number of selectors

- // that would work depends on their sizes (e.g. "#foo .bar" has a

- // size of 2). Since we don't know the sizes of the selectors here, we

- // simply split them into groups of 1,024, based on the reasonable

- // assumption that the average selector won't have a size greater than 8.

- // The alternative would be to calculate the sizes of the selectors and

- // divide them up accordingly, but this approach is more efficient and has

- // worked well in practice. In theory this could still lead to some

- // selectors not working on Chromium, but it is highly unlikely.

- // See issue #6298 and https://crbug.com/804179

- for (let i = 0; i < selectors.length; i += this.selectorGroupSize)

- {

- let selector = selectors.slice(i, i + this.selectorGroupSize).join(", ");

- style.sheet.insertRule(selector + "{display: none !important;}",

- style.sheet.cssRules.length);

- }

+ if (appendOnly)

+ style.textContent += styleSheet;

Sebastian Noack 2018/09/29 14:36:13 I didn't benchmark it, but I'd assume using insert

Manish Jethani 2018/09/29 15:58:48 The problem with using `insertRule` here: we don't

Sebastian Noack 2018/09/29 16:39:30 How about making the function in core return an ar

Manish Jethani 2018/09/29 17:28:04 That's not really an option, it would destroy the

That's not really an option, it would destroy the optimizations we have done for the most common cases.

Makes sense, but then again core can only return a style sheet. Unless you mean that `createStyleSheet` itself should return rules, but then again I don't really understand what the benefit of that would be. In the content script, you would still have two code paths, one for the full style sheet and one for a list of style rules.

Sebastian Noack 2018/09/29 21:01:49 How many ms would that add to the bill if core wil

On 2018/09/29 17:28:04, Manish Jethani wrote: > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > On 2018/09/29 15:58:48, Manish Jethani wrote: > > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > > I didn't benchmark it, but I'd assume using insertRule() would be faster, > > > > especially in the append scenario. IF you reassign textContent on the > other > > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > > > Also, if you assign to textContent, a text node is created, and the > browser > > > may > > > > keep a copy of the raw CSS in memory, while with insertRule(), no node is > > > added > > > > to the DOM, and only the parsed representation needs to be kept in memory. > > > > > > The problem with using `insertRule` here: we don't know that the style sheet > > is > > > always going to be a single rule; we could split the style sheet along > newline > > > characters and treat each part as a rule, but then that's another assumption > > the > > > content script would be making about the background page's logic. > > > > How about making the function in core return an array of rules (rather than > > joining it by newlines in the first place) and the passing this array to the > > content script? This seems to be the most sensible solution to me. > > That's not really an option, it would destroy the optimizations we have done for > the most common cases. > > > > Also it would > > > be a bit odd to user `insertRule` for appending and `textContent` for > > > replacement. > > > > What I had in mind is the old logic we had in here, where we'd use > insertRule() > > in both cases (after using removeRule(), or if it's simpler just recreate the > > <style> element, in case we aren't appending). > > Makes sense, but then again core can only return a style sheet. > > Unless you mean that `createStyleSheet` itself should return rules, but then > again I don't really understand what the benefit of that would be. In the > content script, you would still have two code paths, one for the full style > sheet and one for a list of style rules.

How many ms would that add to the bill if core will cache (and return) an array of rules (which have to be joined every time we inject a user stylesheet) rather than caching/returning a joined string? Perhaps, the optimization will be slightly less effective for platforms that support user stylesheets, but I'd expect caching an array of rules still being more efficient than what we are doing now where the rules have to be generated as well every time (rather than just being joined).

Manish Jethani 2018/09/30 08:06:21 If you run the JS profiler in DevTools with the `g

On 2018/09/29 21:01:49, Sebastian Noack wrote: > On 2018/09/29 17:28:04, Manish Jethani wrote: > > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > > On 2018/09/29 15:58:48, Manish Jethani wrote: > > > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > > > I didn't benchmark it, but I'd assume using insertRule() would be > faster, > > > > > especially in the append scenario. IF you reassign textContent on the > > other > > > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > > > > > Also, if you assign to textContent, a text node is created, and the > > browser > > > > may > > > > > keep a copy of the raw CSS in memory, while with insertRule(), no node > is > > > > added > > > > > to the DOM, and only the parsed representation needs to be kept in > memory. > > > > > > > > The problem with using `insertRule` here: we don't know that the style > sheet > > > is > > > > always going to be a single rule; we could split the style sheet along > > newline > > > > characters and treat each part as a rule, but then that's another > assumption > > > the > > > > content script would be making about the background page's logic. > > > > > > How about making the function in core return an array of rules (rather than > > > joining it by newlines in the first place) and the passing this array to the > > > content script? This seems to be the most sensible solution to me. > > > > That's not really an option, it would destroy the optimizations we have done > for > > the most common cases. > > > > > > Also it would > > > > be a bit odd to user `insertRule` for appending and `textContent` for > > > > replacement. > > > > > > What I had in mind is the old logic we had in here, where we'd use > > insertRule() > > > in both cases (after using removeRule(), or if it's simpler just recreate > the > > > <style> element, in case we aren't appending). > > > > Makes sense, but then again core can only return a style sheet. > > > > Unless you mean that `createStyleSheet` itself should return rules, but then > > again I don't really understand what the benefit of that would be. In the > > content script, you would still have two code paths, one for the full style > > sheet and one for a list of style rules. > > How many ms would that add to the bill if core will cache (and return) an array > of rules (which have to be joined every time we inject a user stylesheet) rather > than caching/returning a joined string?

If you run the JS profiler in DevTools with the `getSelectorsForDomain` version, go to the Bottom-Up tab, and open the V8 Runtime node in the tree, the first things you'll see there are DoJoin, Join, InnerArrayJoin, etc. The extension spends a significant amount of time just joining strings. We are avoiding the native join function by doing it manually [1], but it would still not be free. [1]: https://codereview.adblockplus.org/29892563/

No, we can't avoid rule generation because the array of selectors for known domains is created afresh each time. Anyway, we've already made it more complex than it was, I think the current implementation is most optimal for the most common cases. Why do we even want to change the implementation in core just to address a special case here, which is significantly older versions of Chrome and Firefox, all versions of Edge, and _only_ for collapsing selectors. I'm OK with even disabling this feature for those browsers: the content script can send the injectSelectors message to the background page and not care if it actually worked. This would be the best option at this point. Alternatively, the content script could just call `styleSheet.split("\n")` to get the rules.

Sebastian Noack 2018/09/30 14:19:24 This doesn't seem to only be about element collaps

On 2018/09/30 08:06:21, Manish Jethani wrote: > On 2018/09/29 21:01:49, Sebastian Noack wrote: > > On 2018/09/29 17:28:04, Manish Jethani wrote: > > > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > > > On 2018/09/29 15:58:48, Manish Jethani wrote: > > > > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > > > > I didn't benchmark it, but I'd assume using insertRule() would be > > faster, > > > > > > especially in the append scenario. IF you reassign textContent on the > > > other > > > > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > > > > > > > Also, if you assign to textContent, a text node is created, and the > > > browser > > > > > may > > > > > > keep a copy of the raw CSS in memory, while with insertRule(), no node > > is > > > > > added > > > > > > to the DOM, and only the parsed representation needs to be kept in > > memory. > > > > > > > > > > The problem with using `insertRule` here: we don't know that the style > > sheet > > > > is > > > > > always going to be a single rule; we could split the style sheet along > > > newline > > > > > characters and treat each part as a rule, but then that's another > > assumption > > > > the > > > > > content script would be making about the background page's logic. > > > > > > > > How about making the function in core return an array of rules (rather > than > > > > joining it by newlines in the first place) and the passing this array to > the > > > > content script? This seems to be the most sensible solution to me. > > > > > > That's not really an option, it would destroy the optimizations we have done > > for > > > the most common cases. > > > > > > > > Also it would > > > > > be a bit odd to user `insertRule` for appending and `textContent` for > > > > > replacement. > > > > > > > > What I had in mind is the old logic we had in here, where we'd use > > > insertRule() > > > > in both cases (after using removeRule(), or if it's simpler just recreate > > the > > > > <style> element, in case we aren't appending). > > > > > > Makes sense, but then again core can only return a style sheet. > > > > > > Unless you mean that `createStyleSheet` itself should return rules, but then > > > again I don't really understand what the benefit of that would be. In the > > > content script, you would still have two code paths, one for the full style > > > sheet and one for a list of style rules. > > > > How many ms would that add to the bill if core will cache (and return) an > array > > of rules (which have to be joined every time we inject a user stylesheet) > rather > > than caching/returning a joined string? > > If you run the JS profiler in DevTools with the `getSelectorsForDomain` version, > go to the Bottom-Up tab, and open the V8 Runtime node in the tree, the first > things you'll see there are DoJoin, Join, InnerArrayJoin, etc. The extension > spends a significant amount of time just joining strings. We are avoiding the > native join function by doing it manually [1], but it would still not be free. > > [1]: https://codereview.adblockplus.org/29892563/ > > > Perhaps, the optimization will be slightly less effective for platforms that > > support user stylesheets, but I'd expect caching an array of rules still being > > more efficient than what we are doing now where the rules have to be generated > > as well every time (rather than just being joined). > > No, we can't avoid rule generation because the array of selectors for known > domains is created afresh each time. Anyway, we've already made it more complex > than it was, I think the current implementation is most optimal for the most > common cases. > > Why do we even want to change the implementation in core just to address a > special case here, which is significantly older versions of Chrome and Firefox, > all versions of Edge, and _only_ for collapsing selectors. I'm OK with even > disabling this feature for those browsers: the content script can send the > injectSelectors message to the background page and not care if it actually > worked. This would be the best option at this point. Alternatively, the content > script could just call `styleSheet.split("\n")` to get the rules.

This doesn't seem to only be about element collapsing, only the append scenario is, and yes that the whole CSS has to be parsed and applied again there is a minor issue. However, the major issue here is that you assign to textContent (for element collapsing as well for element hiding), which causes a text node with all the injected CSS to be created, which isn't an option for following reasons: * A copy of the raw CSS text will be kept in memory, increasing the per-document memory footprint. * The website can detect changes we do to the CSS, since changing the textContent triggered a MutationObserver while CSSOM cannot be listened for. * As more as we add to the DOM, as higher the chance this will cause side effects with random web applications. I even have a real world example: Outlook 365 uses (or at least used to use) a subdocument with an <html contenteditable> element for composing emails, and then serializes the DOM in in order to generate the mail body. So everything we add to the DOM will end up in outgoing emails. An empty <style> element would be relative harmless there, but adding 600KB of redundant CSS text to each email is bad. * While technically arguable, copyright trolls used to give ad blockers shit based on how visible there interference with any website is in DOM shown in the devtools. It might confuse other users of the devtools as well to see that long list of selectors.

Manish Jethani 2018/10/01 14:01:36 If assigning `textContent` is an issue, one other

On 2018/09/30 14:19:24, Sebastian Noack wrote: > On 2018/09/30 08:06:21, Manish Jethani wrote: > > On 2018/09/29 21:01:49, Sebastian Noack wrote: > > > On 2018/09/29 17:28:04, Manish Jethani wrote: > > > > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > > > > On 2018/09/29 15:58:48, Manish Jethani wrote: > > > > > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > > > > > I didn't benchmark it, but I'd assume using insertRule() would be > > > faster, > > > > > > > especially in the append scenario. IF you reassign textContent on > the > > > > other > > > > > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > > > > > > > > > Also, if you assign to textContent, a text node is created, and the > > > > browser > > > > > > may > > > > > > > keep a copy of the raw CSS in memory, while with insertRule(), no > node > > > is > > > > > > added > > > > > > > to the DOM, and only the parsed representation needs to be kept in > > > memory. > > > > > > > > > > > > The problem with using `insertRule` here: we don't know that the style > > > sheet > > > > > is > > > > > > always going to be a single rule; we could split the style sheet along > > > > newline > > > > > > characters and treat each part as a rule, but then that's another > > > assumption > > > > > the > > > > > > content script would be making about the background page's logic. > > > > > > > > > > How about making the function in core return an array of rules (rather > > than > > > > > joining it by newlines in the first place) and the passing this array to > > the > > > > > content script? This seems to be the most sensible solution to me. > > > > > > > > That's not really an option, it would destroy the optimizations we have > done > > > for > > > > the most common cases. > > > > > > > > > > Also it would > > > > > > be a bit odd to user `insertRule` for appending and `textContent` for > > > > > > replacement. > > > > > > > > > > What I had in mind is the old logic we had in here, where we'd use > > > > insertRule() > > > > > in both cases (after using removeRule(), or if it's simpler just > recreate > > > the > > > > > <style> element, in case we aren't appending). > > > > > > > > Makes sense, but then again core can only return a style sheet. > > > > > > > > Unless you mean that `createStyleSheet` itself should return rules, but > then > > > > again I don't really understand what the benefit of that would be. In the > > > > content script, you would still have two code paths, one for the full > style > > > > sheet and one for a list of style rules. > > > > > > How many ms would that add to the bill if core will cache (and return) an > > array > > > of rules (which have to be joined every time we inject a user stylesheet) > > rather > > > than caching/returning a joined string? > > > > If you run the JS profiler in DevTools with the `getSelectorsForDomain` > version, > > go to the Bottom-Up tab, and open the V8 Runtime node in the tree, the first > > things you'll see there are DoJoin, Join, InnerArrayJoin, etc. The extension > > spends a significant amount of time just joining strings. We are avoiding the > > native join function by doing it manually [1], but it would still not be free. > > > > [1]: https://codereview.adblockplus.org/29892563/ > > > > > Perhaps, the optimization will be slightly less effective for platforms that > > > support user stylesheets, but I'd expect caching an array of rules still > being > > > more efficient than what we are doing now where the rules have to be > generated > > > as well every time (rather than just being joined). > > > > No, we can't avoid rule generation because the array of selectors for known > > domains is created afresh each time. Anyway, we've already made it more > complex > > than it was, I think the current implementation is most optimal for the most > > common cases. > > > > Why do we even want to change the implementation in core just to address a > > special case here, which is significantly older versions of Chrome and > Firefox, > > all versions of Edge, and _only_ for collapsing selectors. I'm OK with even > > disabling this feature for those browsers: the content script can send the > > injectSelectors message to the background page and not care if it actually > > worked. This would be the best option at this point. Alternatively, the > content > > script could just call `styleSheet.split("\n")` to get the rules. > > This doesn't seem to only be about element collapsing, only the append scenario > is, and yes that the whole CSS has to be parsed and applied again there is a > minor issue. > > However, the major issue here is that you assign to textContent (for element > collapsing as well for element hiding), which causes a text node with all the > injected CSS to be created, which isn't an option for following reasons: > > * A copy of the raw CSS text will be kept in memory, increasing the per-document > memory footprint. > * The website can detect changes we do to the CSS, since changing the > textContent triggered a MutationObserver while CSSOM cannot be listened for. > * As more as we add to the DOM, as higher the chance this will cause side > effects with random web applications. I even have a real world example: Outlook > 365 uses (or at least used to use) a subdocument with an <html contenteditable> > element for composing emails, and then serializes the DOM in in order to > generate the mail body. So everything we add to the DOM will end up in outgoing > emails. An empty <style> element would be relative harmless there, but adding > 600KB of redundant CSS text to each email is bad. > * While technically arguable, copyright trolls used to give ad blockers shit > based on how visible there interference with any website is in DOM shown in the > devtools. It might confuse other users of the devtools as well to see that long > list of selectors.

If assigning `textContent` is an issue, one other option is to undo all the changes in include.preload.js and always pass selectors to the content script if user style sheets is not supported. This would still give the best user experience to people on modern browsers. Remember this whole issue is only because of Edge, otherwise we could have used author style sheets and this wouldn't even be a problem. Maybe we use author style sheets on Chrome and Firefox after all. In any case I think it makes no sense to undo the optimization for the most common case just for an edge case.

Manish Jethani 2018/10/01 14:24:02 OK, I have a solution that would satisfy all cases

On 2018/10/01 14:01:36, Manish Jethani wrote: > On 2018/09/30 14:19:24, Sebastian Noack wrote: > > On 2018/09/30 08:06:21, Manish Jethani wrote: > > > On 2018/09/29 21:01:49, Sebastian Noack wrote: > > > > On 2018/09/29 17:28:04, Manish Jethani wrote: > > > > > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > > > > > On 2018/09/29 15:58:48, Manish Jethani wrote: > > > > > > > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > > > > > > > I didn't benchmark it, but I'd assume using insertRule() would be > > > > faster, > > > > > > > > especially in the append scenario. IF you reassign textContent on > > the > > > > > other > > > > > > > > hand, the browser has to parse and apply the whole CSS again. > > > > > > > > > > > > > > > > Also, if you assign to textContent, a text node is created, and > the > > > > > browser > > > > > > > may > > > > > > > > keep a copy of the raw CSS in memory, while with insertRule(), no > > node > > > > is > > > > > > > added > > > > > > > > to the DOM, and only the parsed representation needs to be kept in > > > > memory. > > > > > > > > > > > > > > The problem with using `insertRule` here: we don't know that the > style > > > > sheet > > > > > > is > > > > > > > always going to be a single rule; we could split the style sheet > along > > > > > newline > > > > > > > characters and treat each part as a rule, but then that's another > > > > assumption > > > > > > the > > > > > > > content script would be making about the background page's logic. > > > > > > > > > > > > How about making the function in core return an array of rules (rather > > > than > > > > > > joining it by newlines in the first place) and the passing this array > to > > > the > > > > > > content script? This seems to be the most sensible solution to me. > > > > > > > > > > That's not really an option, it would destroy the optimizations we have > > done > > > > for > > > > > the most common cases. > > > > > > > > > > > > Also it would > > > > > > > be a bit odd to user `insertRule` for appending and `textContent` > for > > > > > > > replacement. > > > > > > > > > > > > What I had in mind is the old logic we had in here, where we'd use > > > > > insertRule() > > > > > > in both cases (after using removeRule(), or if it's simpler just > > recreate > > > > the > > > > > > <style> element, in case we aren't appending). > > > > > > > > > > Makes sense, but then again core can only return a style sheet. > > > > > > > > > > Unless you mean that `createStyleSheet` itself should return rules, but > > then > > > > > again I don't really understand what the benefit of that would be. In > the > > > > > content script, you would still have two code paths, one for the full > > style > > > > > sheet and one for a list of style rules. > > > > > > > > How many ms would that add to the bill if core will cache (and return) an > > > array > > > > of rules (which have to be joined every time we inject a user stylesheet) > > > rather > > > > than caching/returning a joined string? > > > > > > If you run the JS profiler in DevTools with the `getSelectorsForDomain` > > version, > > > go to the Bottom-Up tab, and open the V8 Runtime node in the tree, the first > > > things you'll see there are DoJoin, Join, InnerArrayJoin, etc. The extension > > > spends a significant amount of time just joining strings. We are avoiding > the > > > native join function by doing it manually [1], but it would still not be > free. > > > > > > [1]: https://codereview.adblockplus.org/29892563/ > > > > > > > Perhaps, the optimization will be slightly less effective for platforms > that > > > > support user stylesheets, but I'd expect caching an array of rules still > > being > > > > more efficient than what we are doing now where the rules have to be > > generated > > > > as well every time (rather than just being joined). > > > > > > No, we can't avoid rule generation because the array of selectors for known > > > domains is created afresh each time. Anyway, we've already made it more > > complex > > > than it was, I think the current implementation is most optimal for the most > > > common cases. > > > > > > Why do we even want to change the implementation in core just to address a > > > special case here, which is significantly older versions of Chrome and > > Firefox, > > > all versions of Edge, and _only_ for collapsing selectors. I'm OK with even > > > disabling this feature for those browsers: the content script can send the > > > injectSelectors message to the background page and not care if it actually > > > worked. This would be the best option at this point. Alternatively, the > > content > > > script could just call `styleSheet.split("\n")` to get the rules. > > > > This doesn't seem to only be about element collapsing, only the append > scenario > > is, and yes that the whole CSS has to be parsed and applied again there is a > > minor issue. > > > > However, the major issue here is that you assign to textContent (for element > > collapsing as well for element hiding), which causes a text node with all the > > injected CSS to be created, which isn't an option for following reasons: > > > > * A copy of the raw CSS text will be kept in memory, increasing the > per-document > > memory footprint. > > * The website can detect changes we do to the CSS, since changing the > > textContent triggered a MutationObserver while CSSOM cannot be listened for. > > * As more as we add to the DOM, as higher the chance this will cause side > > effects with random web applications. I even have a real world example: > Outlook > > 365 uses (or at least used to use) a subdocument with an <html > contenteditable> > > element for composing emails, and then serializes the DOM in in order to > > generate the mail body. So everything we add to the DOM will end up in > outgoing > > emails. An empty <style> element would be relative harmless there, but adding > > 600KB of redundant CSS text to each email is bad. > > * While technically arguable, copyright trolls used to give ad blockers shit > > based on how visible there interference with any website is in DOM shown in > the > > devtools. It might confuse other users of the devtools as well to see that > long > > list of selectors. > > If assigning `textContent` is an issue, one other option is to undo all the > changes in include.preload.js and always pass selectors to the content script if > user style sheets is not supported. This would still give the best user > experience to people on modern browsers. > > Remember this whole issue is only because of Edge, otherwise we could have used > author style sheets and this wouldn't even be a problem. Maybe we use author > style sheets on Chrome and Firefox after all. > > In any case I think it makes no sense to undo the optimization for the most > common case just for an edge case.

OK, I have a solution that would satisfy all cases. We add a function to core: function splitStyleSheet(styleSheet) { ... } It will give you list of style rules. In the background page, use this function to return a list of rules instead of a sheet. Because splitting a string does not allocate new memory for each substring (only for the temporary array), this should be good for memory as well as satisfactory enough for the performance and other requirements for the non-user style sheets case. Does this sound reasonable?

Sebastian Noack 2018/10/01 19:42:27 Sounds good!

+ else

+ style.textContent = styleSheet;

- addSelectors(selectors, filters, groupName = "emulated", appendOnly = false)

+ addSelectors(selectors, groupName = "standard", appendOnly = false)

{

- if (this.inline)

+ browser.runtime.sendMessage({

+ type: "content.injectSelectors",

+ selectors,

+ groupName,

+ appendOnly

+ },

+ styleSheet =>

{

- // Insert the style rules inline if we have been instructed by the

- // background page to do so. This is usually the case, except on platforms

- // that do support user stylesheets via the browser.tabs.insertCSS API

- // (Firefox 53 onwards for now and possibly Chrome in the near future).

- // Once all supported platforms have implemented this API, we can remove

- // the code below. See issue #5090.

- // Related Chrome and Firefox issues:

- // https://bugs.chromium.org/p/chromium/issues/detail?id=632009

- // https://bugzilla.mozilla.org/show_bug.cgi?id=1310026

- this.addSelectorsInline(selectors, groupName, appendOnly);

- }

- else

- {

- browser.runtime.sendMessage({

- type: "content.injectSelectors",

- selectors,

- groupName,

- appendOnly

- });

- }

- // Only trace selectors that are based directly on hiding filters

- // (i.e. leave out collapsing selectors).

- if (this.tracer && groupName != "collapsing")

- this.tracer.addSelectors(selectors, filters);

Sebastian Noack 2018/09/29 14:36:13 It seems ElementHidingTracer.addSelectors() is nev

Manish Jethani 2018/09/29 15:58:48 ElementHidingTracer is a whole pandora's box. If i

Sebastian Noack 2018/09/29 16:39:30 What I was thinking of is this: diff -r ae52536f7

What I was thinking of is this: diff -r ae52536f7389 include.preload.js --- a/include.preload.js Fri Sep 28 17:58:30 2018 +0200 +++ b/include.preload.js Sat Sep 29 18:23:52 2018 +0200 @@ -258,22 +258,19 @@ this.trace(); } ElementHidingTracer.prototype = { - addSelectors(selectors, filters) + addSelectors(selectors) { - let pairs = selectors.map((sel, i) => [sel, filters && filters[i]]); + if (document.readyState != "loading") + this.checkNodes([document], selectors); - if (document.readyState != "loading") - this.checkNodes([document], pairs); - - this.selectors.push(...pairs); + this.selectors.push(...selectors); }, - checkNodes(nodes, pairs) + checkNodes(nodes, selectors) { let selectors = []; - let filters = []; - for (let [selector, filter] of pairs) + for (let selector of selectors) { nodes: for (let node of nodes) { @@ -284,27 +281,19 @@ // priority, or haven't been circumvented in a different way. if (getComputedStyle(element).display == "none") { - // For regular element hiding, we don't know the exact filter, - // but the background page can find it with the given selector. - // In case of element hiding emulation, the generated selector - // we got here is different from the selector part of the filter, - // but in this case we can send the whole filter text instead. - if (filter) - filters.push(filter); - else - selectors.push(selector); - + selectors.push(selector); break nodes; } } } } - if (selectors.length > 0 || filters.length > 0) + if (selectors.length > 0) { browser.runtime.sendMessage({ type: "hitLogger.traceElemHide", - selectors, filters + selectors, + filters: [] }); } }, But regardless of this simplification, after your changes, hitLogger.traceElemHide would only receive either filters or selectors (never both), and it would make sense to go a step further, splitting it up into two message types (which would also improve the performance of the devtools panel in particular when the filter text is known). I'm happy to just add the above change here, and save those changes to lib/hitLogger.js for another patch.

Manish Jethani 2018/09/29 17:28:04 OK, but since I'm not the author of these changes

Sebastian Noack 2018/09/29 21:01:49 Well, your adapting for core changes here that mak

Manish Jethani 2018/09/30 08:36:38 The local `selectors` variable here needs to be re

+ if (styleSheet)

+ {

+ // Insert the style sheet inline if we have been instructed by the

+ // background page to do so. This is rarely the case, except on

+ // platforms that do not support user stylesheets via the

+ // browser.tabs.insertCSS API (Firefox <53, Chrome <66, and Edge).

+ // Once all supported platforms have implemented this API, we can remove

+ // the code below. See issue #5090.

+ // Related Chrome and Firefox issues:

+ // https://bugs.chromium.org/p/chromium/issues/detail?id=632009

+ // https://bugzilla.mozilla.org/show_bug.cgi?id=1310026

+ this.addStyleSheetInline(styleSheet, groupName, appendOnly);

+ }

+ });

hideElements(elements, filters)

{

for (let element of elements)

hideElement(element);

if (this.tracer)

@@ -514,23 +481,21 @@

{

if (this.tracer)

this.tracer.disconnect();

this.tracer = null;

if (response.trace)

this.tracer = new ElementHidingTracer();

- this.inline = response.inline;

- if (this.inline)

- this.addSelectorsInline(response.selectors, "standard");

+ if (response.inline)

+ this.addStyleSheetInline(response.styleSheet.code);

if (this.tracer)

- this.tracer.addSelectors(response.selectors);

+ this.tracer.addSelectors(response.styleSheet.selectors);

this.elemHideEmulation.apply(response.emulatedPatterns);

});

}

};

if (document instanceof HTMLDocument)

{

« no previous file with comments | « dependencies ('k') | lib/contentFiltering.js » ('j') | no next file with comments »