Issue 29893559: Issue 6999 - Generate style sheets in background page (Closed)
|
Created:
Sept. 27, 2018, 5:35 a.m. by Manish Jethani Modified:
Oct. 2, 2018, 8:27 p.m. Reviewers:
Sebastian Noack CC:
geo Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
Patch Set 1 #Patch Set 2 : Updated implementation #
Total comments: 6
Patch Set 3 : Rebase #Patch Set 4 : Remove individual rule injection #Patch Set 5 : Use generateStyleSheetForDomain from core #
Total comments: 3
Patch Set 6 : Update dependency, remove first argument to ElemHideEmulation constructor #Patch Set 7 : Generate collapsing style sheet also in background page #Patch Set 8 : Fix comment wrapping #
Total comments: 17
Patch Set 9 : Apply patch for ElementHidingTracer #Patch Set 10 : Insert individual rules #
Total comments: 7
Patch Set 11 : Pass selectors if and only if trace is true #
MessagesTotal messages: 49
Patch Set 1 I'll have to test this out, but this is the idea.
What is about the logic creating a shadow root (if supported), and creating the <style> element, etc. Wouldn't all of this be dead code after this change, and therefore should be removed as well?
On 2018/09/27 15:26:04, Sebastian Noack wrote: > What is about the logic creating a shadow root (if supported), and creating the > <style> element, etc. Wouldn't all of this be dead code after this change, and > therefore should be removed as well? I have removed it from include.preload.js in this patch.
On 2018/09/27 16:05:12, Manish Jethani wrote: > On 2018/09/27 15:26:04, Sebastian Noack wrote: > > What is about the logic creating a shadow root (if supported), and creating > the > > <style> element, etc. Wouldn't all of this be dead code after this change, and > > therefore should be removed as well? > > I have removed it from include.preload.js in this patch. How could I miss that, my bad. The changes look good to me. Geo, can you try out this patch (note that you have to apply https://codereview.adblockplus.org/29893555/ first) and test it a bit on Microsoft Edge? Mostly, making sure that element hiding generally still works: https://testpages.adblockplus.org/en/filters/element-hiding
On 2018/09/27 16:13:11, Sebastian Noack wrote: > On 2018/09/27 16:05:12, Manish Jethani wrote: > > On 2018/09/27 15:26:04, Sebastian Noack wrote: > > > What is about the logic creating a shadow root (if supported), and creating > > the > > > <style> element, etc. Wouldn't all of this be dead code after this change, > and > > > therefore should be removed as well? > > > > I have removed it from include.preload.js in this patch. > > How could I miss that, my bad. The changes look good to me. > > Geo, can you try out this patch (note that you have to apply > https://codereview.adblockplus.org/29893555/ first) and test it a bit on > Microsoft Edge? Mostly, making sure that element hiding generally still works: > https://testpages.adblockplus.org/en/filters/element-hiding I've applied both patches, but after this one, element hiding doesn't work on Edge anymore.
On 2018/09/27 17:33:59, geo wrote: > I've applied both patches, but after this one, element hiding doesn't work on > Edge anymore. Do you get any error (either in the background page or in the content page)?
On 2018/09/27 17:43:46, Sebastian Noack wrote: > On 2018/09/27 17:33:59, geo wrote: > > I've applied both patches, but after this one, element hiding doesn't work on > > Edge anymore. > > Do you get any error (either in the background page or in the content page)? Nothing, no errors.
Manish, just in case did you test it on older versions of Chrome and Firefox that don't support user stylesheets either? Geo, could you try if tabs.insertCSS() generally works (and specifically when a frameId is given) on Microsoft Edge?
On 2018/09/27 18:24:01, geo wrote:
> On 2018/09/27 17:43:46, Sebastian Noack wrote:
> > On 2018/09/27 17:33:59, geo wrote:
> > > I've applied both patches, but after this one, element hiding doesn't work
> on
> > > Edge anymore.
> >
> > Do you get any error (either in the background page or in the content page)?
>
> Nothing, no errors.
It must be because we suppress all errors.
The `addStyleSheet` function in `lib/contentFiltering.js`, this is where there
might be an error. Can you modify the code to print out the error instead of
ignoring it and see what the error is?
Something like:
...
// See error handling notes in the catch block.
promise.catch(console.error);
}
catch (error)
{
console.error(error);
...
On 2018/09/27 21:30:40, Manish Jethani wrote:
> On 2018/09/27 18:24:01, geo wrote:
> > On 2018/09/27 17:43:46, Sebastian Noack wrote:
> > > On 2018/09/27 17:33:59, geo wrote:
> > > > I've applied both patches, but after this one, element hiding doesn't
work
> > on
> > > > Edge anymore.
> > >
> > > Do you get any error (either in the background page or in the content
page)?
> >
> > Nothing, no errors.
>
> It must be because we suppress all errors.
>
> The `addStyleSheet` function in `lib/contentFiltering.js`, this is where there
> might be an error. Can you modify the code to print out the error instead of
> ignoring it and see what the error is?
>
> Something like:
>
> ...
>
> // See error handling notes in the catch block.
> promise.catch(console.error);
> }
> catch (error)
> {
> console.error(error);
> ...
Also, we're checking for a specific error message:
// If the error is about the "cssOrigin" option, this is an older version
// of Chromium (65 and below) or Firefox (52 and below) that does not
// support user style sheets.
if (/\bcssOrigin\b/.test(error.message))
userStyleSheetsSupported = false;
The message might be different on Edge, in that case we'll want to see what the
message is.
On 2018/09/27 21:32:08, Manish Jethani wrote:
> On 2018/09/27 21:30:40, Manish Jethani wrote:
> > On 2018/09/27 18:24:01, geo wrote:
> > > On 2018/09/27 17:43:46, Sebastian Noack wrote:
> > > > On 2018/09/27 17:33:59, geo wrote:
> > > > > I've applied both patches, but after this one, element hiding doesn't
> work
> > > on
> > > > > Edge anymore.
> > > >
> > > > Do you get any error (either in the background page or in the content
> page)?
> > >
> > > Nothing, no errors.
> >
> > It must be because we suppress all errors.
> >
> > The `addStyleSheet` function in `lib/contentFiltering.js`, this is where
there
> > might be an error. Can you modify the code to print out the error instead of
> > ignoring it and see what the error is?
> >
> > Something like:
> >
> > ...
> >
> > // See error handling notes in the catch block.
> > promise.catch(console.error);
> > }
> > catch (error)
> > {
> > console.error(error);
> > ...
>
> Also, we're checking for a specific error message:
>
> // If the error is about the "cssOrigin" option, this is an older version
> // of Chromium (65 and below) or Firefox (52 and below) that does not
> // support user style sheets.
> if (/\bcssOrigin\b/.test(error.message))
> userStyleSheetsSupported = false;
>
> The message might be different on Edge, in that case we'll want to see what
the
> message is.
So, the error is just a string, " Error: Invalid value for argument 2. Property
'frameId': Unexpected property, Property 'cssOrigin': Unexpected property."
Something like this works, but I don't know the full implications of that code.
`
let details = {
code: styleSheet,
matchAboutBlank: true,
runAt: "document_start"
};
if (info.platform != "edgehtml")
details.frameId = frameId;
.....
if (userStyleSheetsSupported &&
(/\bcssOrigin\b/.test(error.message) || /\bcssOrigin\b/.test(error)))
{
userStyleSheetsSupported = false;
return addStyleSheet(tabId, frameId, styleSheet);
}
`
On 2018/09/27 23:18:45, geo wrote:
> So, the error is just a string, " Error: Invalid value for argument 2.
Property
> 'frameId': Unexpected property, Property 'cssOrigin': Unexpected property."
>
> Something like this works, but I don't know the full implications of that
code.
> `
> let details = {
> code: styleSheet,
> matchAboutBlank: true,
> runAt: "document_start"
> };
> if (info.platform != "edgehtml")
> details.frameId = frameId;
> .....
> if (userStyleSheetsSupported &&
> (/\bcssOrigin\b/.test(error.message) || /\bcssOrigin\b/.test(error)))
> {
> userStyleSheetsSupported = false;
>
> return addStyleSheet(tabId, frameId, styleSheet);
> }
> `
Thanks for figuring that out! Not using cssOrigin (in order to request a user
stylesheet) on platforms that don't support it is fine. But if we cannot even
target a particular frame this will severely break things. So I guess we have to
stick to injecting the stylesheet through a content script.
Geo, can you file a Microsoft Edge bug (if there isn't any yet) for the lack of
support of frameId in tabs.insertCSS()?
Manish, do you think in regard of #6957 it would be enough to remove the Shadow
DOM functionality (which isn't used on Microsoft Edge anyway) in order to have
all browsers benefit from that optimization? After all, we can still inject the
(same) pre-computed CSS (if we ignore the Shadow DOM scenario) but would have to
pass it to the content script.
On 2018/09/27 23:34:51, Sebastian Noack wrote: > Manish, do you think in regard of #6957 it would be enough to remove the Shadow > DOM functionality (which isn't used on Microsoft Edge anyway) in order to have > all browsers benefit from that optimization? After all, we can still inject the > (same) pre-computed CSS (if we ignore the Shadow DOM scenario) but would have to > pass it to the content script. Yes, I think just removing the shadow DOM code should be fine. We are already passing the list of selectors in this case, now we will pass a style sheet instead (selectors are only really needed if the developer tools are open).
On 2018/09/28 07:13:28, Manish Jethani wrote: > On 2018/09/27 23:34:51, Sebastian Noack wrote: > > > Manish, do you think in regard of #6957 it would be enough to remove the > Shadow > > DOM functionality (which isn't used on Microsoft Edge anyway) in order to have > > all browsers benefit from that optimization? After all, we can still inject > the > > (same) pre-computed CSS (if we ignore the Shadow DOM scenario) but would have > to > > pass it to the content script. > > Yes, I think just removing the shadow DOM code should be fine. We are already > passing the list of selectors in this case, now we will pass a style sheet > instead (selectors are only really needed if the developer tools are open). So here's the plan: I'll test on the oldest version of Chrome and Firefox that we support; if author style sheets via insertCSS works, we'll stick with what we have here for those platforms. On Edge, we will definitely stick with the old way. Does this sound OK?
On 2018/09/28 07:16:26, Manish Jethani wrote: > So here's the plan: I'll test on the oldest version of Chrome and Firefox that > we support; if author style sheets via insertCSS works, we'll stick with what we > have here for those platforms. On Edge, we will definitely stick with the old > way. > > Does this sound OK? I'd rather keep the number of the code paths small. So if we have to keep injecting the author stylesheet through the content script for Microsoft Edge anyway, I'd keep doing the same for older Chrome and Firefox versions.
On 2018/09/28 10:02:27, Sebastian Noack wrote: > On 2018/09/28 07:16:26, Manish Jethani wrote: > > So here's the plan: I'll test on the oldest version of Chrome and Firefox that > > we support; if author style sheets via insertCSS works, we'll stick with what > we > > have here for those platforms. On Edge, we will definitely stick with the old > > way. > > > > Does this sound OK? > > I'd rather keep the number of the code paths small. So if we have to keep > injecting the author stylesheet through the content script for Microsoft Edge > anyway, I'd keep doing the same for older Chrome and Firefox versions. Sounds alright to me. In that case, we just remove the shadow DOM code.
On 2018/09/28 10:07:52, Manish Jethani wrote: > On 2018/09/28 10:02:27, Sebastian Noack wrote: > > On 2018/09/28 07:16:26, Manish Jethani wrote: > > > So here's the plan: I'll test on the oldest version of Chrome and Firefox > that > > > we support; if author style sheets via insertCSS works, we'll stick with > what > > we > > > have here for those platforms. On Edge, we will definitely stick with the > old > > > way. > > > > > > Does this sound OK? > > > > I'd rather keep the number of the code paths small. So if we have to keep > > injecting the author stylesheet through the content script for Microsoft Edge > > anyway, I'd keep doing the same for older Chrome and Firefox versions. > > Sounds alright to me. In that case, we just remove the shadow DOM code. I realize that it's not just the shadow DOM. We would pass the style sheet instead of the list of selectors to the content script and this would mean some changes to the content script related to that. At the same time, there no reason to remove the shadow DOM now just for this change: we could just leave it in. But I also agree that we should remove the shadow DOM, it's just totally unrelated to this change here though. I have filed a separate issue: https://issues.adblockplus.org/ticket/6998
Patch Set 2: Updated implementation This one is based on https://codereview.adblockplus.org/29894564/ We can refactor a bit, happy to discuss.
https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js#... include.preload.js:465: for (let i = 0; i < selectors.length; i += this.selectorGroupSize) What is this code still good for if stylesheets are now generated by the background page?
https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js#... include.preload.js:465: for (let i = 0; i < selectors.length; i += this.selectorGroupSize) On 2018/09/28 15:33:41, Sebastian Noack wrote: > What is this code still good for if stylesheets are now generated by the > background page? This is for element collapsing. If this.inline is set (no user style sheets) and an element needs to be collapsed, it comes here. We could do .one of two things here: (1) don't do element collapsing on platforms that don't support user style sheets, or (2) send the selectors to the background page anyway, but ask it to return the generated style sheet back so we just inject it here.
https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js#... include.preload.js:465: for (let i = 0; i < selectors.length; i += this.selectorGroupSize) On 2018/09/28 16:04:44, Manish Jethani wrote: > On 2018/09/28 15:33:41, Sebastian Noack wrote: > > What is this code still good for if stylesheets are now generated by the > > background page? > > This is for element collapsing. If this.inline is set (no user style sheets) and > an element needs to be collapsed, it comes here. > > We could do .one of two things here: (1) don't do element collapsing on > platforms that don't support user style sheets, or (2) send the selectors to the > background page anyway, but ask it to return the generated style sheet back so > we just inject it here. Please correct me if I'm wrong, but wouldn't it simplify the logic (on the expense of a redundant round trip to the background page but that is fine if it's just for platforms that don't support user stylesheets), if we send the selector back to the background page (just like we do on platforms that support user stylesheets), and then have the generated CSS sent back to the content script (just like we do for element hiding)?
https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js#... include.preload.js:465: for (let i = 0; i < selectors.length; i += this.selectorGroupSize) On 2018/09/28 16:09:06, Sebastian Noack wrote: > On 2018/09/28 16:04:44, Manish Jethani wrote: > > On 2018/09/28 15:33:41, Sebastian Noack wrote: > > > What is this code still good for if stylesheets are now generated by the > > > background page? > > > > This is for element collapsing. If this.inline is set (no user style sheets) > and > > an element needs to be collapsed, it comes here. > > > > We could do .one of two things here: (1) don't do element collapsing on > > platforms that don't support user style sheets, or (2) send the selectors to > the > > background page anyway, but ask it to return the generated style sheet back so > > we just inject it here. > > Please correct me if I'm wrong, but wouldn't it simplify the logic (on the > expense of a redundant round trip to the background page but that is fine if > it's just for platforms that don't support user stylesheets), if we send the > selector back to the background page (just like we do on platforms that support > user stylesheets), and then have the generated CSS sent back to the content > script (just like we do for element hiding)? Yes, I agree. What's more is that it's going to be one selector, and the returned style sheet will also have one selector. We can simply append it to the existing style sheet, if any, in the content script.
https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js#... include.preload.js:465: for (let i = 0; i < selectors.length; i += this.selectorGroupSize) On 2018/09/28 16:12:51, Manish Jethani wrote: > On 2018/09/28 16:09:06, Sebastian Noack wrote: > > On 2018/09/28 16:04:44, Manish Jethani wrote: > > > On 2018/09/28 15:33:41, Sebastian Noack wrote: > > > > What is this code still good for if stylesheets are now generated by the > > > > background page? > > > > > > This is for element collapsing. If this.inline is set (no user style sheets) > > and > > > an element needs to be collapsed, it comes here. > > > > > > We could do .one of two things here: (1) don't do element collapsing on > > > platforms that don't support user style sheets, or (2) send the selectors to > > the > > > background page anyway, but ask it to return the generated style sheet back > so > > > we just inject it here. > > > > Please correct me if I'm wrong, but wouldn't it simplify the logic (on the > > expense of a redundant round trip to the background page but that is fine if > > it's just for platforms that don't support user stylesheets), if we send the > > selector back to the background page (just like we do on platforms that > support > > user stylesheets), and then have the generated CSS sent back to the content > > script (just like we do for element hiding)? > > Yes, I agree. What's more is that it's going to be one selector, and the > returned style sheet will also have one selector. We can simply append it to the > existing style sheet, if any, in the content script. Sounds good. Mind implementing those changes?
Patch Set 3: Rebase Patch Set 4: Remove individual rule injection https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29894581/include.preload.js#... include.preload.js:465: for (let i = 0; i < selectors.length; i += this.selectorGroupSize) On 2018/09/28 16:32:50, Sebastian Noack wrote: > On 2018/09/28 16:12:51, Manish Jethani wrote: > > On 2018/09/28 16:09:06, Sebastian Noack wrote: > > > On 2018/09/28 16:04:44, Manish Jethani wrote: > > > > On 2018/09/28 15:33:41, Sebastian Noack wrote: > > > > > What is this code still good for if stylesheets are now generated by the > > > > > background page? > > > > > > > > This is for element collapsing. If this.inline is set (no user style > sheets) > > > and > > > > an element needs to be collapsed, it comes here. > > > > > > > > We could do .one of two things here: (1) don't do element collapsing on > > > > platforms that don't support user style sheets, or (2) send the selectors > to > > > the > > > > background page anyway, but ask it to return the generated style sheet > back > > so > > > > we just inject it here. > > > > > > Please correct me if I'm wrong, but wouldn't it simplify the logic (on the > > > expense of a redundant round trip to the background page but that is fine if > > > it's just for platforms that don't support user stylesheets), if we send the > > > selector back to the background page (just like we do on platforms that > > support > > > user stylesheets), and then have the generated CSS sent back to the content > > > script (just like we do for element hiding)? > > > > Yes, I agree. What's more is that it's going to be one selector, and the > > returned style sheet will also have one selector. We can simply append it to > the > > existing style sheet, if any, in the content script. > > Sounds good. Mind implementing those changes? I thought about this a little more. It's always going to be one selector that needs to be injected at a time. We don't have to go to the background page this, we can just generate a style sheet like so: selectors.join(", ") + " {display: none !important;}\n" That's all. Please see the latest patch.
Patch Set 5: Use generateStyleSheetForDomain from core This one includes a dependency update. https://codereview.adblockplus.org/29893559/diff/29894607/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29893559/diff/29894607/lib/contentFilterin... lib/contentFiltering.js:231: styleSheet = ElemHide.generateStyleSheetForDomain(docDomain, specificOnly, If the third argument is false here, style sheet generation is super fast, but no selectors will be returned.
I still kinda feel it would be simpler if we message the background page (regardless whether user stylesheets are supported) and then send a message back to the content script if no user stylesheets are supported with the CSS generated by core to be injected (regardless whether we are doing element hiding or element collapsing). Alternatively, we could always collapse elements through the style property if user style sheets aren't supported. On Microsoft Edge we already do that anyway, since CSS.escape() isn't supported (see line 138 in include.preload.js as of current next).
https://codereview.adblockplus.org/29893559/diff/29894607/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29893559/diff/29894607/include.preload.js#... include.preload.js:403: () => {}, This argument should be removed now, and the dependency updated accordingly.
https://codereview.adblockplus.org/29893559/diff/29894607/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29893559/diff/29894607/include.preload.js#... include.preload.js:403: () => {}, On 2018/09/29 02:07:20, Sebastian Noack wrote: > This argument should be removed now, and the dependency updated accordingly. Acknowledged. I'll update this patch on Monday.
Patch Set 6: Update dependency, remove first argument to ElemHideEmulation constructor Patch Set 7: Generate collapsing style sheet also in background page
Patch Set 8: Fix comment wrapping
https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:401: Nit: The blank line here no longer seems to help the readability. https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:489: this.tracer.addSelectors(selectors, filters); It seems ElementHidingTracer.addSelectors() is never called with a second argument now and the related logic can be removed now. https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:428: style.textContent += styleSheet; 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.
https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:489: this.tracer.addSelectors(selectors, filters); On 2018/09/29 14:36:13, Sebastian Noack wrote: > It seems ElementHidingTracer.addSelectors() is never called with a second > argument now and the related logic can be removed now. ElementHidingTracer is a whole pandora's box. If it never gets any filters, we should clean up all the related code. I think that would have to be a separate change. https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:428: style.textContent += styleSheet; 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. Also it would be a bit odd to user `insertRule` for appending and `textContent` for replacement. Let's leave it like it is and revisit if it's a performance issue.
https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:489: this.tracer.addSelectors(selectors, filters); On 2018/09/29 15:58:48, Manish Jethani wrote: > On 2018/09/29 14:36:13, Sebastian Noack wrote: > > It seems ElementHidingTracer.addSelectors() is never called with a second > > argument now and the related logic can be removed now. > > ElementHidingTracer is a whole pandora's box. If it never gets any filters, we > should clean up all the related code. I think that would have to be a separate > change. 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. https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:428: style.textContent += styleSheet; 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. > 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).
https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:489: this.tracer.addSelectors(selectors, filters); On 2018/09/29 16:39:30, Sebastian Noack wrote: > [...] > > 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. OK, but since I'm not the author of these changes (nor have I been through them in detail yet), and since they really are somewhat unrelated to just generating the style sheets in the background page, maybe it should be a separate patch? https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:428: style.textContent += styleSheet; 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.
https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:489: this.tracer.addSelectors(selectors, filters); On 2018/09/29 17:28:04, Manish Jethani wrote: > On 2018/09/29 16:39:30, Sebastian Noack wrote: > > [...] > > > > 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. > > OK, but since I'm not the author of these changes (nor have I been through them > in detail yet), and since they really are somewhat unrelated to just generating > the style sheets in the background page, maybe it should be a separate patch? Well, your adapting for core changes here that make it redundant to pass filters along with the selectors. So I think I can expect you to remove any related redundant code, and the changes I'm asking for here are trivial. https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:428: style.textContent += styleSheet; 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).
https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:428: style.textContent += styleSheet; 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.
Patch Set 9: Apply patch for ElementHidingTracer https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:489: this.tracer.addSelectors(selectors, filters); On 2018/09/29 16:39:30, Sebastian Noack wrote: > - checkNodes(nodes, pairs) > + checkNodes(nodes, selectors) > { > let selectors = []; The local `selectors` variable here needs to be renamed. I've renamed it to `effectiveSelectors`. Otherwise no other changes.
https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:428: style.textContent += styleSheet; 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.
https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:428: style.textContent += styleSheet; 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.
https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:428: style.textContent += styleSheet; 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?
https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29893559/diff/29895563/include.preload.js#... include.preload.js:428: style.textContent += styleSheet; On 2018/10/01 14:24:02, Manish Jethani wrote: > 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? Sounds good!
Patch Set 10: Insert individual rules
https://codereview.adblockplus.org/29893559/diff/29898573/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29893559/diff/29898573/include.preload.js#... include.preload.js:401: Nit: Not sure if this blank line still helps readability. https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... lib/contentFiltering.js:249: response.rules = [...rulesFromStyleSheet(styleSheet.code)]; Why do we have to copy this array? https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... lib/contentFiltering.js:250: else if (trace) Doesn't the content scripts needs the original selectors (in order for the hit logger to find the respective filters) if tracing is active, regardless of the "inline" flag?
https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... lib/contentFiltering.js:249: response.rules = [...rulesFromStyleSheet(styleSheet.code)]; On 2018/10/02 17:14:02, Sebastian Noack wrote: > Why do we have to copy this array? Ah, just figured it's a generator. I think I personally, might rather use Array.from(), or does the rest-operator perform better?
https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... lib/contentFiltering.js:249: response.rules = [...rulesFromStyleSheet(styleSheet.code)]; On 2018/10/02 17:15:38, Sebastian Noack wrote: > On 2018/10/02 17:14:02, Sebastian Noack wrote: > > Why do we have to copy this array? > > Ah, just figured it's a generator. I think I personally, might rather use > Array.from(), or does the rest-operator perform better? Yes, the spread operator is significantly faster (actually it's much, much faster) [1] There's also a difference: the spread operator simply iterates over the "iterable" (defined as an object that has a Symbol.iterator method), whereas Array.from also additionally supports "array-like" objects (objects with a length property and so on). [1]: https://jsperf.com/set-iterator-vs-foreach/4
https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... lib/contentFiltering.js:249: response.rules = [...rulesFromStyleSheet(styleSheet.code)]; On 2018/10/02 17:23:24, Manish Jethani wrote: > On 2018/10/02 17:15:38, Sebastian Noack wrote: > > On 2018/10/02 17:14:02, Sebastian Noack wrote: > > > Why do we have to copy this array? > > > > Ah, just figured it's a generator. I think I personally, might rather use > > Array.from(), or does the rest-operator perform better? > > Yes, the spread operator is significantly faster (actually it's much, much > faster) [1] > > There's also a difference: the spread operator simply iterates over the > "iterable" (defined as an object that has a Symbol.iterator method), whereas > Array.from also additionally supports "array-like" objects (objects with a > length property and so on). > > [1]: https://jsperf.com/set-iterator-vs-foreach/4 I'd take that benchmark with a grain of salt, since it's testing against a Set object (which it the spread operator might be optimized for), not a generic generator. But for the case here, I guess, it doesn't matter too much.
Patch Set 11: Pass selectors if and only if trace is true https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... File lib/contentFiltering.js (right): https://codereview.adblockplus.org/29893559/diff/29898573/lib/contentFilterin... lib/contentFiltering.js:250: else if (trace) On 2018/10/02 17:14:02, Sebastian Noack wrote: > Doesn't the content scripts needs the original selectors (in order for the hit > logger to find the respective filters) if tracing is active, regardless of the > "inline" flag? Ah, you're right. Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
