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

Delta Between Two Patch Sets: include.preload.js

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

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

Right Patch Set: Pass selectors if and only if trace is true Created Oct. 2, 2018, 7:07 p.m.

Left:
Right:

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

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 /*	1 /*

2 * This file is part of Adblock Plus <https://adblockplus.org/>,	2 * This file is part of Adblock Plus <https://adblockplus.org/>,

3 * Copyright (C) 2006-present eyeo GmbH	3 * Copyright (C) 2006-present eyeo GmbH

4 *	4 *

5 * Adblock Plus is free software: you can redistribute it and/or modify	5 * Adblock Plus is free software: you can redistribute it and/or modify

6 * it under the terms of the GNU General Public License version 3 as	6 * it under the terms of the GNU General Public License version 3 as

7 * published by the Free Software Foundation.	7 * published by the Free Software Foundation.

8 *	8 *

9 * Adblock Plus is distributed in the hope that it will be useful,	9 * Adblock Plus is distributed in the hope that it will be useful,

10 * but WITHOUT ANY WARRANTY; without even the implied warranty of	10 * but WITHOUT ANY WARRANTY; without even the implied warranty of

(...skipping 240 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
251 this.timeout = null;	251 this.timeout = null;

252 this.observer = new MutationObserver(this.observe.bind(this));	252 this.observer = new MutationObserver(this.observe.bind(this));

253 this.trace = this.trace.bind(this);	253 this.trace = this.trace.bind(this);

254	254

255 if (document.readyState == "loading")	255 if (document.readyState == "loading")

256 document.addEventListener("DOMContentLoaded", this.trace);	256 document.addEventListener("DOMContentLoaded", this.trace);

257 else	257 else

258 this.trace();	258 this.trace();

259 }	259 }

260 ElementHidingTracer.prototype = {	260 ElementHidingTracer.prototype = {

261 addSelectors(selectors, filters)	261 addSelectors(selectors)

262 {	262 {

263 let pairs = selectors.map((sel, i) => [sel, filters && filters[i]]);

264

265 if (document.readyState != "loading")	263 if (document.readyState != "loading")

266 this.checkNodes([document], pairs);	264 this.checkNodes([document], selectors);

267	265

268 this.selectors.push(...pairs);	266 this.selectors.push(...selectors);

269 },	267 },

270	268

271 checkNodes(nodes, pairs)	269 checkNodes(nodes, selectors)

272 {	270 {

273 let selectors = [];	271 let effectiveSelectors = [];

274 let filters = [];	272

275	273 for (let selector of selectors)

276 for (let [selector, filter] of pairs)

277 {	274 {

278 nodes: for (let node of nodes)	275 nodes: for (let node of nodes)

279 {	276 {

280 for (let element of node.querySelectorAll(selector))	277 for (let element of node.querySelectorAll(selector))

281 {	278 {

282 // Only consider selectors that actually have an effect on the	279 // Only consider selectors that actually have an effect on the

283 // computed styles, and aren't overridden by rules with higher	280 // computed styles, and aren't overridden by rules with higher

284 // priority, or haven't been circumvented in a different way.	281 // priority, or haven't been circumvented in a different way.

285 if (getComputedStyle(element).display == "none")	282 if (getComputedStyle(element).display == "none")

286 {	283 {

287 // For regular element hiding, we don't know the exact filter,	284 effectiveSelectors.push(selector);

288 // but the background page can find it with the given selector.

289 // In case of element hiding emulation, the generated selector

290 // we got here is different from the selector part of the filter,

291 // but in this case we can send the whole filter text instead.

292 if (filter)

293 filters.push(filter);

294 else

295 selectors.push(selector);

296

297 break nodes;	285 break nodes;

298 }	286 }

299 }	287 }

300 }	288 }

301 }	289 }

302	290

303 if (selectors.length > 0 \|\| filters.length > 0)	291 if (effectiveSelectors.length > 0)

304 {	292 {

305 browser.runtime.sendMessage({	293 browser.runtime.sendMessage({

306 type: "hitLogger.traceElemHide",	294 type: "hitLogger.traceElemHide",

307 selectors, filters	295 selectors: effectiveSelectors,

	296 filters: []

308 });	297 });

309 }	298 }

310 },	299 },

311	300

312 onTimeout()	301 onTimeout()

313 {	302 {

314 this.checkNodes(this.changedNodes, this.selectors);	303 this.checkNodes(this.changedNodes, this.selectors);

315 this.changedNodes = [];	304 this.changedNodes = [];

316 this.timeout = null;	305 this.timeout = null;

317 },	306 },

(...skipping 76 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
394 };	383 };

395	384

396 function ContentFiltering()	385 function ContentFiltering()

397 {	386 {

398 this.styles = new Map();	387 this.styles = new Map();

399 this.tracer = null;	388 this.tracer = null;

400	389

401 this.elemHideEmulation = new ElemHideEmulation(this.hideElements.bind(this));	390 this.elemHideEmulation = new ElemHideEmulation(this.hideElements.bind(this));

402 }	391 }

403 ContentFiltering.prototype = {	392 ContentFiltering.prototype = {

404 addStyleSheetInline(styleSheet, groupName = "standard", appendOnly = false)	393 addRulesInline(rules, groupName = "standard", appendOnly = false)

405 {	394 {

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

	396

	397 if (style && !appendOnly)

	398 {

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

	400 style.sheet.deleteRule(0);

	401 }

	402

	403 if (rules.length == 0)

	404 return;

407	405

408 if (!style)	406 if (!style)

409 {	407 {

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

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

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

413 // new one.	411 // new one.

414 style = document.createElement("style");	412 style = document.createElement("style");

415 (document.head \|\| document.documentElement).appendChild(style);	413 (document.head \|\| document.documentElement).appendChild(style);

416	414

417 // It can happen that the frame already navigated to a different	415 // It can happen that the frame already navigated to a different

418 // document while we were waiting for the background page to respond.	416 // document while we were waiting for the background page to respond.

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

420 // <style> element.	418 // <style> element.

421 if (!style.sheet)	419 if (!style.sheet)

422 return;	420 return;

423	421

424 this.styles.set(groupName, style);	422 this.styles.set(groupName, style);

425 }	423 }

426	424

427 if (appendOnly)	425 for (let rule of rules)

428 style.textContent += styleSheet;	426 style.sheet.insertRule(rule, style.sheet.cssRules.length);
Sebastian Noack 2018/09/29 14:36:13 I didn't benchmark it, but I'd assume using insert 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. Manish Jethani 2018/09/29 15:58:48 The problem with using `insertRule` here: we don't Show quoted text 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. Sebastian Noack 2018/09/29 16:39:30 How about making the function in core return an ar Show quoted text 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. Show quoted text > 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). Manish Jethani 2018/09/29 17:28:04 That's not really an option, it would destroy the Show quoted text 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. Show quoted text > > 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. Sebastian Noack 2018/09/29 21:01:49 How many ms would that add to the bill if core wil Show quoted text 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 Show quoted text 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/ Show quoted text > 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. Sebastian Noack 2018/09/30 14:19:24 This doesn't seem to only be about element collaps Show quoted text 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 Show quoted text 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 Show quoted text 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! Show quoted text 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!
429 else

430 style.textContent = styleSheet;

431 },	427 },

432	428

433 addSelectors(selectors, groupName = "standard", appendOnly = false)	429 addSelectors(selectors, groupName = "standard", appendOnly = false)

434 {	430 {

435 browser.runtime.sendMessage({	431 browser.runtime.sendMessage({

436 type: "content.injectSelectors",	432 type: "content.injectSelectors",

437 selectors,	433 selectors,

438 groupName,	434 groupName,

439 appendOnly	435 appendOnly

440 },	436 },

441 styleSheet =>	437 rules =>

442 {	438 {

443 if (styleSheet)	439 if (rules)

444 {	440 {

445 // Insert the style sheet inline if we have been instructed by the	441 // Insert the rules inline if we have been instructed by the background

446 // background page to do so. This is rarely the case, except on	442 // page to do so. This is rarely the case, except on platforms that do

447 // platforms that do not support user stylesheets via the	443 // not support user stylesheets via the browser.tabs.insertCSS API

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

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

450 // the code below. See issue #5090.	446 // the code below. See issue #5090.

451 // Related Chrome and Firefox issues:	447 // Related Chrome and Firefox issues:

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

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

454 this.addStyleSheetInline(styleSheet, groupName, appendOnly);	450 this.addRulesInline(rules, groupName, appendOnly);

455 }	451 }

456 });	452 });

457 },	453 },

458	454

459 hideElements(elements, filters)	455 hideElements(elements, filters)

460 {	456 {

461 for (let element of elements)	457 for (let element of elements)

462 hideElement(element);	458 hideElement(element);

463	459

464 if (this.tracer)	460 if (this.tracer)

(...skipping 15 matching lines...) Expand all Loading...
480 response =>	476 response =>

481 {	477 {

482 if (this.tracer)	478 if (this.tracer)

483 this.tracer.disconnect();	479 this.tracer.disconnect();

484 this.tracer = null;	480 this.tracer = null;

485	481

486 if (response.trace)	482 if (response.trace)

487 this.tracer = new ElementHidingTracer();	483 this.tracer = new ElementHidingTracer();

488	484

489 if (response.inline)	485 if (response.inline)

490 this.addStyleSheetInline(response.styleSheet.code);	486 this.addRulesInline(response.rules);

491	487

492 if (this.tracer)	488 if (this.tracer)

493 this.tracer.addSelectors(response.styleSheet.selectors);	489 this.tracer.addSelectors(response.selectors);

494	490

495 this.elemHideEmulation.apply(response.emulatedPatterns);	491 this.elemHideEmulation.apply(response.emulatedPatterns);

496 });	492 });

497 }	493 }

498 };	494 };

499	495

500 if (document instanceof HTMLDocument)	496 if (document instanceof HTMLDocument)

501 {	497 {

502 checkSitekey();	498 checkSitekey();

503	499

(...skipping 10 matching lines...) Expand all Loading...
514 let element = event.target;	510 let element = event.target;

515 if (/^i?frame$/.test(element.localName))	511 if (/^i?frame$/.test(element.localName))

516 checkCollapse(element);	512 checkCollapse(element);

517 }, true);	513 }, true);

518 }	514 }

519	515

520 window.checkCollapse = checkCollapse;	516 window.checkCollapse = checkCollapse;

521 window.contentFiltering = contentFiltering;	517 window.contentFiltering = contentFiltering;

522 window.typeMap = typeMap;	518 window.typeMap = typeMap;

523 window.getURLsFromElement = getURLsFromElement;	519 window.getURLsFromElement = getURLsFromElement;

LEFT	RIGHT