Issue 29559743: Issue 5650 - Apply emulation filters to shadow DOMs
|
Created:
Sept. 29, 2017, 11:21 p.m. by Manish Jethani Modified:
June 14, 2018, 5:09 a.m. CC:
kzar Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
Patch Set 1 #
Total comments: 6
Patch Set 2 : Prevent author code from intercepting shadowAttached event #
Total comments: 7
Patch Set 3 : Find shadow roots semi-asynchronously #
Total comments: 4
Patch Set 4 : Remove unused node argument #
Total comments: 11
Patch Set 5 : Terminate loop #Patch Set 6 : Use Node.contains #Patch Set 7 : Take event type as a parameter #
Total comments: 4
Patch Set 8 : Optimize removeRedundantNodes based on node order #
Total comments: 6
Patch Set 9 : Run code only on if Shadow DOM is supported #
Total comments: 5
Patch Set 10 : Use pure generator function to extract added subtrees #MessagesTotal messages: 39
Patch Set 1 For -abp-has and -abp-contains we need to be able to peek into the shadow DOMs. This is one part of the change, the other one is in adblockpluschrome where we override Element.attachShadow and dispatch an "shadowAttached" event. https://codereview.adblockplus.org/29559743/diff/29559744/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29559744/lib/content/elemHid... lib/content/elemHideEmulation.js:46: if (!node.parentElement && !(node.parentNode instanceof ShadowRoot)) BODY has a parent element (HTML), but the topmost element in a shadow DOM doesn't. We still need to add a ":host" prefix in this case, but the topmost element must be included in the CSS selector. https://codereview.adblockplus.org/29559743/diff/29559744/lib/content/elemHid... lib/content/elemHideEmulation.js:311: this.root = document; Now we have a root property that can be a document or a document fragment (shadow root). https://codereview.adblockplus.org/29559743/diff/29559744/lib/content/elemHid... lib/content/elemHideEmulation.js:315: this.shadowEmulations = new WeakMap(); Using WeakMap here, though I suspect the shadow root object may be held on to in other ways (via MutationObserver for example), even after the host element has been removed from the document. I'll look further into this. https://codereview.adblockplus.org/29559743/diff/29559744/lib/content/elemHid... lib/content/elemHideEmulation.js:610: for (let mutation of mutations) We listen for "shadowAttached" on the document, but if the shadow host hasn't been added to the document then the event won't reach us. When new nodes are added, we must find shadow roots within those nodes, recursively. https://codereview.adblockplus.org/29559743/diff/29559744/lib/content/elemHid... lib/content/elemHideEmulation.js:650: if (this.root == this.document) Listen for these events only if this is the main instance.
https://codereview.adblockplus.org/29559743/diff/29559744/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29559744/lib/content/elemHid... lib/content/elemHideEmulation.js:604: for (let child of node.children) We could use a generator and setTimeout here to keep things in the background.
Patch Set 2: Prevent author code from intercepting shadowAttached event https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... lib/content/elemHideEmulation.js:308: function ElemHideEmulation(document, root, addSelectorsFunc, hideElemsFunc) It's best to pass these in the constructor as we need them early. https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... lib/content/elemHideEmulation.js:317: if (this.root == this.document) We have to listen for these events as early as possible, before any of the site's code has had a chance to run. If not, the author code can simply add a listener and call Event.stopPropagation on the event object. We're even listening in capture mode so we get the event first, no matter what. https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... lib/content/elemHideEmulation.js:585: event.stopImmediatePropagation(); Prevent any author code from detecting even the presence of this event.
https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... lib/content/elemHideEmulation.js:585: event.stopImmediatePropagation(); On 2017/09/30 12:29:01, Manish Jethani wrote: > Prevent any author code from detecting even the presence of this event. What will stop page from sending own events with the same name and custom (and potentially malicious) event.target? Why do we even use event with predictable name in the firs place?
https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... lib/content/elemHideEmulation.js:585: event.stopImmediatePropagation(); On 2017/10/02 14:11:55, lainverse wrote: > On 2017/09/30 12:29:01, Manish Jethani wrote: > > Prevent any author code from detecting even the presence of this event. > > What will stop page from sending own events with the same name and custom (and > potentially malicious) event.target? That's a good point, but the event would have to be dispatched off a DOM node for it to be caught by us, and the node would have to have a shadowRoot property. If it does, we only do something if we haven't already seen the same shadow root. > Why do we even use event with predictable name in the firs place? We could namespace it, like "org.adblockplus::shadowAttached", so that there is no unintentional conflict with the author's code, but I don't see why anyone would deliberately degrade their site's performance when it has no actual effect (the event would be a no-op if the target doesn't have a shadow root or if we've already seen the shadow root). But we could just add a random component to the name, I suppose, just to be safe.
https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... lib/content/elemHideEmulation.js:585: event.stopImmediatePropagation(); On 2017/10/02 14:28:11, Manish Jethani wrote: > On 2017/10/02 14:11:55, lainverse wrote: > > On 2017/09/30 12:29:01, Manish Jethani wrote: > > > Prevent any author code from detecting even the presence of this event. > > > > What will stop page from sending own events with the same name and custom (and > > potentially malicious) event.target? > > That's a good point, but the event would have to be dispatched off a DOM node > for it to be caught by us, and the node would have to have a shadowRoot > property. If it does, we only do something if we haven't already seen the same > shadow root. What about a proper DOM element with Proxy with getter in place of shadowRoot property? Even if event name would be random I'd recommend to use actual shadowRoot getter instead of event.target.shadowRoot since any attempt to access anything within it will trigger code in that getter. As I udnerstand event's code might be postponed a bit and site may redefine shadowRoot with proxy or just null.
https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29560555/lib/content/elemHid... lib/content/elemHideEmulation.js:585: event.stopImmediatePropagation(); On 2017/10/02 16:05:39, lainverse wrote: > [...] > > What about a proper DOM element with Proxy with getter in place of shadowRoot > property? Even if event name would be random I'd recommend to use actual > shadowRoot getter instead of event.target.shadowRoot since any attempt to access > anything within it will trigger code in that getter. As I udnerstand event's > code might be postponed a bit and site may redefine shadowRoot with proxy or > just null. A content script has its own copy of these APIs, a site cannot override these. When we access event.target.shadowRoot, for example, our own wrapper for Element.shadowRoot in the page does not get called, because that is a different context.
I'll file an issue with Chromium to allow extensions to access closed shadow roots. Meanwhile, why don't we access the shadow DOM when it's open? When Chromium implements the feature, the same code will just work. It's not just -abp-has and -abp-contains, -abp-properties doesn't apply to elements inside shadow DOMs either.
On 2017/10/14 00:52:37, Manish Jethani wrote: > I'll file an issue with Chromium to allow extensions to access closed shadow > roots. > > Meanwhile, why don't we access the shadow DOM when it's open? When Chromium > implements the feature, the same code will just work. > > It's not just -abp-has and -abp-contains, -abp-properties doesn't apply to > elements inside shadow DOMs either. Good point. We should do that.
This sounds like a good idea.
Patch Set 3: Find shadow roots semi-asynchronously https://codereview.adblockplus.org/29559743/diff/29582707/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29582707/lib/content/elemHid... lib/content/elemHideEmulation.js:149: function removeRedundantNodes(nodes) We have to do this, because often (too often) when the DOM is constructed programmatically, multiple levels of nodes are added at once, then we get multiple mutations at once. We are only interested in the most common ancestors. For example, on the TechCrunch home page I'm getting 31 nodes added. They all have one common ancestor. If we don't remove all the others from the list, we end up checking 261 nodes for shadow roots. On the other hand, when we do remove all but the common ancestor, we do only 31 checks. This algorithm is the most efficient I could come up with. There's a comment in there about an optimization. https://codereview.adblockplus.org/29559743/diff/29582707/lib/content/elemHid... lib/content/elemHideEmulation.js:192: function niceLoop(iterator, callback) This is basically an abstraction of what is happening elsewhere in the code. It iterates over the iterator and calls the callback for each item, taking a break every once in a while. https://codereview.adblockplus.org/29559743/diff/29582707/lib/content/elemHid... lib/content/elemHideEmulation.js:198: for (let next = iterator.next(); !next.done; next = iterator.next()) We cannot simply do "for (let item of iterator) ..." here, it does not work with yield* for some reason. I'm pretty sure it's broken elsewhere in the code as well. I'll take a look. https://codereview.adblockplus.org/29559743/diff/29582707/lib/content/elemHid... lib/content/elemHideEmulation.js:375: this.patterns = []; This needs to be initialized.
Patch Set 4: Remove unused node argument
https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:168: nodeInfo.redundant = true; Why don't you just keep track of redundant nodes in the first place? Or just remove them right away (using a for(;;) loop, updating the loop index), here? https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:203: setTimeout(loop, 0); Don't you have to terminate the loop here? https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:382: this.document.addEventListener("shadowAttached", So I assume you still intend to patch attachShadow() in order to dispatch this event? If we don't have any better idea, we should at least randomize the event name, so that web pages cannot listen to it.
Patch Set 5: Terminate loop https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:168: nodeInfo.redundant = true; On 2017/10/19 00:43:08, Sebastian Noack wrote: > Why don't you just keep track of redundant nodes in the first place? You mean "node.redundant = true" rather than "nodeInfo.redundant = true"? I did not want to modify an object passed in by the browser. > Or jus remove them right away (using a for(;;) loop, updating the loop index), here? As I've tried to explain in the comment, we could just delete the node right away from the Set object instead of setting the redundant property first and then later filtering it out, but then we'd have to walk up the tree for each node. When a node's parent is redundant, the node is redundant too. This is an important optimization given the kind of data we're going to get here. Take this: <div id="a"> <div id="b"> <div id="c"> </div> </div> </div> Now suppose this was built incrementally. We'll get all three nodes. If we remove "b" right away, when we're looking at "c" we'll have to go all the way up to "a" to find out that "c" is redundant. By keeping "b" but simply marking it as redundant, we only climb one level until we know that "c" is redundant. This saves a lot of iterations. It cut execution time in half for the TechCrunch home page where I was testing it. https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:203: setTimeout(loop, 0); On 2017/10/19 00:43:08, Sebastian Noack wrote: > Don't you have to terminate the loop here? Right, I forgot to terminate it. Done. https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:382: this.document.addEventListener("shadowAttached", On 2017/10/19 00:43:08, Sebastian Noack wrote: > So I assume you still intend to patch attachShadow() in order to dispatch this > event? If we don't have any better idea, we should at least randomize the event > name, so that web pages cannot listen to it. Ack. Web pages won't get the event because we're calling Event.stopImmediatePropagation, and we're the first to get the message. But it could happen that web pages message with us by dispatching the event. I'll try to come up with something, probably a randomized event name.
https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:168: nodeInfo.redundant = true; On 2017/10/19 02:13:44, Manish Jethani wrote: > [...] > > Now suppose this was built incrementally. We'll get all three nodes. If we > remove "b" right away, when we're looking at "c" we'll have to go all the way up > to "a" to find out that "c" is redundant. By keeping "b" but simply marking it > as redundant, we only climb one level until we know that "c" is redundant. This > saves a lot of iterations. It cut execution time in half for the TechCrunch home > page where I was testing it. By setting a redundant flag we don't just avoid walking all the way up to the root, we also avoid doing lookups in the Set object.
https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:168: nodeInfo.redundant = true; On 2017/10/19 02:19:06, Manish Jethani wrote: > On 2017/10/19 02:13:44, Manish Jethani wrote: > > [...] > > > > Now suppose this was built incrementally. We'll get all three nodes. If we > > remove "b" right away, when we're looking at "c" we'll have to go all the way > up > > to "a" to find out that "c" is redundant. By keeping "b" but simply marking it > > as redundant, we only climb one level until we know that "c" is redundant. > This > > saves a lot of iterations. It cut execution time in half for the TechCrunch > home > > page where I was testing it. > > By setting a redundant flag we don't just avoid walking all the way up to the > root, we also avoid doing lookups in the Set object. How about using a Set() object containing only redundant nodes, instead an array of objects holding each node and the information whether they are redundant?
https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:168: nodeInfo.redundant = true; On 2017/10/19 03:51:01, Sebastian Noack wrote: > On 2017/10/19 02:19:06, Manish Jethani wrote: > > On 2017/10/19 02:13:44, Manish Jethani wrote: > > > [...] > > > > > > Now suppose this was built incrementally. We'll get all three nodes. If we > > > remove "b" right away, when we're looking at "c" we'll have to go all the > way > > up > > > to "a" to find out that "c" is redundant. By keeping "b" but simply marking > it > > > as redundant, we only climb one level until we know that "c" is redundant. > > This > > > saves a lot of iterations. It cut execution time in half for the TechCrunch > > home > > > page where I was testing it. > > > > By setting a redundant flag we don't just avoid walking all the way up to the > > root, we also avoid doing lookups in the Set object. > > How about using a Set() object containing only redundant nodes, instead an array > of objects holding each node and the information whether they are redundant? How do you do lookups efficiently in an array? Perhaps I'm misunderstanding, could you explain?
Maybe something like this:
// parentNode crawler
function removeRedundantNodes(nodes) {
let link, redundant = [];
for (let node of nodes) {
link = node;
while (link = link.parentNode) {
if (nodes.has(link)) {
redundant.push(node);
break;
}
}
}
redundant.forEach(node => nodes.delete(node));
return nodes;
}
// .contains() over existing list
function removeRedundantNodes(nodes) {
let parent, child, redundant = [];
for (parent of nodes) {
if (redundant.includes(parent))
continue;
for (child of nodes) {
if (parent !== child && parent.contains(child)) {
redundant.push(child);
}
}
}
redundant.forEach(node => nodes.delete(node));
return nodes;
}
Also, if nodes would be an array it shold be possible to use nodes.some(parent
=> parent.contains(node))
// parentNode crawler
function removeRedundantNodes(nodes) {
let link, redundant = [];
for (let node of nodes) {
link = node;
while (link = link.parentNode) {
if (nodes.has(link)) {
redundant.push(node);
break;
}
}
}
redundant.forEach(node => nodes.delete(node));
return nodes;
}
// .contains() over existing list
function removeRedundantNodes(nodes) {
let parent, child, redundant = [];
for (parent of nodes) {
if (redundant.includes(parent))
continue;
for (child of nodes) {
if (parent !== child && parent.contains(child)) {
redundant.push(child);
}
}
}
redundant.forEach(node => nodes.delete(node));
return nodes;
}
// parent.contains(child) when nodes is an Array let removeRedundantNodes = nodes => nodes.filter( node => !nodes.some(parent => parent !== node && parent.contains(node)) );
https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:168: nodeInfo.redundant = true; On 2017/10/19 09:57:14, Manish Jethani wrote: > How do you do lookups efficiently in an array? > > Perhaps I'm misunderstanding, could you explain? I was basically talking about what lainverse is suggesting above. Perhaps, a Set isn't even necessary.
Patch Set 6: Use Node.contains https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29582710/lib/content/elemHid... lib/content/elemHideEmulation.js:168: nodeInfo.redundant = true; On 2017/10/19 18:35:39, Sebastian Noack wrote: > On 2017/10/19 09:57:14, Manish Jethani wrote: > > How do you do lookups efficiently in an array? > > > > Perhaps I'm misunderstanding, could you explain? > > I was basically talking about what lainverse is suggesting above. Perhaps, a Set > isn't even necessary. OK, I did not realize Node.contains was so efficient. Done.
Patch Set 7: Take event type as a parameter https://codereview.adblockplus.org/29559743/diff/29583631/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29583631/lib/content/elemHid... lib/content/elemHideEmulation.js:348: shadowAttachedEventType) This is the event type, it's a parameter now, include.preload.js should pass a value here. https://codereview.adblockplus.org/29559743/diff/29583631/lib/content/elemHid... lib/content/elemHideEmulation.js:368: this.findShadowRoots(this.root.children); If this root is not the document (it's a shadow root), find shadow roots within this root. https://codereview.adblockplus.org/29559743/diff/29583631/lib/content/elemHid... lib/content/elemHideEmulation.js:621: this.findShadowRoots(this.root.children); Find shadow roots on the load event. This is not strictly necessary because we're overriding the Element.attachShadow API before anything else happens on the page. I'm not sure we should keep this, but I've added it for now. https://codereview.adblockplus.org/29559743/diff/29583631/lib/content/elemHid... lib/content/elemHideEmulation.js:654: this.shadowAttachedEventType); Pass this on.
Patch Set 8: Optimize removeRedundantNodes based on node order A node always appears before any of its descendants so we don't have to check the entire list for ancestors.
https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... lib/content/elemHideEmulation.js:679: if (node instanceof Element) Actually if nodes are always added in parent -> child order then why not check does last node in the list contains current one? Something like: node instanceof Element && !elements.some(other => other.contains(node)) Then extractSubtrees won't be needed at all.
https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... lib/content/elemHideEmulation.js:48: let newSelector = node instanceof ShadowRoot ? ":host" : ":root"; In Firefox Nightly `document.querySelector("div") instanceof ShadowRoot` cause an exception "ReferenceError" because ShadowRoot is not defined. In Chrome it returns `false` which is expected (or `true` if it really is)
Patch Set 9: Run code only on if Shadow DOM is supported https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... lib/content/elemHideEmulation.js:48: let newSelector = node instanceof ShadowRoot ? ":host" : ":root"; On 2017/10/20 15:37:57, hub wrote: > In Firefox Nightly > > `document.querySelector("div") instanceof ShadowRoot` cause an exception > "ReferenceError" because ShadowRoot is not defined. > > In Chrome it returns `false` which is expected (or `true` if it really is) Done. https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... lib/content/elemHideEmulation.js:679: if (node instanceof Element) On 2017/10/20 15:37:07, lainverse wrote: > Actually if nodes are always added in parent -> child order then why not check > does last node in the list contains current one? > > Something like: > node instanceof Element && !elements.some(other => other.contains(node)) > > Then extractSubtrees won't be needed at all. That's a very good point! While it is better in theory, I tried it out and it seems to run slightly slower for whatever reason. Maybe it's because extractSubtrees is a pure function and it always runs on the same unmodified array object in both the outer and inner loops. I've left it as it is for now. https://codereview.adblockplus.org/29559743/diff/29584695/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29584695/lib/content/elemHid... lib/content/elemHideEmulation.js:679: if (typeof ShadowRoot != "undefined") Of course we only have to run this code on Chrome.
https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... lib/content/elemHideEmulation.js:679: if (node instanceof Element) > While it is better in theory, I tried it out and it seems to run slightly slower > for whatever reason. Maybe it's because extractSubtrees is a pure function and > it always runs on the same unmodified array object in both the outer and inner > loops. Most likely that's because arrow function were defined within a loop. It may create a new instance every time it runs. Try to move it out of the loop. let elements = []; let isRoot = node => node instanceof Element && !elements.some(other => other.contains(node)); // as I understand 'let node' within a loop creates new one each time let mutation, node; for (mutation of mutations) for (node of mutation.addedNodes) if (isRoot(node)) elements.push(node);
https://codereview.adblockplus.org/29559743/diff/29584695/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29584695/lib/content/elemHid... lib/content/elemHideEmulation.js:679: if (typeof ShadowRoot != "undefined") On 2017/10/20 16:16:42, Manish Jethani wrote: > Of course we only have to run this code on Chrome. Maybe we should set a different mutation observer based on this as the observer is called quite often.
Patch Set 10: Use pure generator function to extract added subtrees https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29584630/lib/content/elemHid... lib/content/elemHideEmulation.js:679: if (node instanceof Element) On 2017/10/20 17:37:36, lainverse wrote: > > While it is better in theory, I tried it out and it seems to run slightly > slower > > for whatever reason. Maybe it's because extractSubtrees is a pure function and > > it always runs on the same unmodified array object in both the outer and inner > > loops. > > Most likely that's because arrow function were defined within a loop. It may > create a new instance every time it runs. Try to move it out of the loop. > > let elements = []; > let isRoot = node => node instanceof Element && !elements.some(other => > other.contains(node)); > // as I understand 'let node' within a loop creates new one each time > let mutation, node; > for (mutation of mutations) > for (node of mutation.addedNodes) > if (isRoot(node)) > elements.push(node); Simply moving the array function out of the loop did not help. But I tried moving it out entirely to make it a pure function with no reference to the surrounding scope, and that helped. Then I moved the entire loop out into its own pure function, that helped more. Finally I thought why not make it a generator, that way it can participate in niceLoop's niceness. Done. https://codereview.adblockplus.org/29559743/diff/29584695/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29584695/lib/content/elemHid... lib/content/elemHideEmulation.js:679: if (typeof ShadowRoot != "undefined") On 2017/10/20 19:12:50, hub wrote: > On 2017/10/20 16:16:42, Manish Jethani wrote: > > Of course we only have to run this code on Chrome. > > Maybe we should set a different mutation observer based on this as the observer > is called quite often. But both the observers would listen for the same DOM mutations, it's not like the work would be split between the two. It would be worse because we would have two observers doing the same work that one could do.
https://codereview.adblockplus.org/29559743/diff/29584695/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29584695/lib/content/elemHid... lib/content/elemHideEmulation.js:679: if (typeof ShadowRoot != "undefined") On 2017/10/22 20:01:35, Manish Jethani wrote: > On 2017/10/20 19:12:50, hub wrote: > > On 2017/10/20 16:16:42, Manish Jethani wrote: > > > Of course we only have to run this code on Chrome. > > > > Maybe we should set a different mutation observer based on this as the > observer > > is called quite often. > > But both the observers would listen for the same DOM mutations, it's not like > the work would be split between the two. It would be worse because we would have > two observers doing the same work that one could do. not saying to observe twice. but when we set up the MutationObserver, we could setup up a different one if the browser supports ShadowRoot. Feels like a micro optimisation, maybe.
https://codereview.adblockplus.org/29559743/diff/29584695/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29559743/diff/29584695/lib/content/elemHid... lib/content/elemHideEmulation.js:679: if (typeof ShadowRoot != "undefined") On 2017/10/23 13:30:34, hub wrote: > On 2017/10/22 20:01:35, Manish Jethani wrote: > > On 2017/10/20 19:12:50, hub wrote: > > > On 2017/10/20 16:16:42, Manish Jethani wrote: > > > > Of course we only have to run this code on Chrome. > > > > > > Maybe we should set a different mutation observer based on this as the > > observer > > > is called quite often. > > > > But both the observers would listen for the same DOM mutations, it's not like > > the work would be split between the two. It would be worse because we would > have > > two observers doing the same work that one could do. > > not saying to observe twice. but when we set up the MutationObserver, we could > setup up a different one if the browser supports ShadowRoot. Feels like a micro > optimisation, maybe. OK, I see what you mean. I think we're going to end up having to add a lot of optimizations to that observe function, and that will involve sharing some code between the shadow roots case and the normal case (e.g. making a list of the newly added subtree roots). On balance it feels like it's probably best to keep it all in one function. If we split the observer, it would make sense to do it based on the mutation type ("childList", "attributes", and "characterData" can each have their own observer), but then again let's see how this code evolves as we add support for user style sheets and so on.
On 2017/10/14 00:52:37, Manish Jethani wrote: > I'll file an issue with Chromium to allow extensions to access closed shadow > roots. Done. https://crbug.com/778816
On 2017/10/26 22:07:03, Manish Jethani wrote: > On 2017/10/14 00:52:37, Manish Jethani wrote: > > I'll file an issue with Chromium to allow extensions to access closed shadow > > roots. > > Done. > > https://crbug.com/778816 I have started work on this: https://crrev.com/c/897291
On 2018/02/02 15:23:02, Manish Jethani wrote: > On 2017/10/26 22:07:03, Manish Jethani wrote: > > On 2017/10/14 00:52:37, Manish Jethani wrote: > > > I'll file an issue with Chromium to allow extensions to access closed shadow > > > roots. > > > > Done. > > > > https://crbug.com/778816 > > I have started work on this: https://crrev.com/c/897291 So the API change got shot down because it's a breaking change. I think that's fair, but now I'm wondering what kind of API should exist to let extensions access closed shadow roots. As far as I am aware, none of the APIs in the chrome.* namespace return DOM nodes, so this is going to be a first.
Shall we go ahead with this change, at least for open shadow DOMs? I don't see why not. But if we disagree then I'll just close this, no point keeping it open for now.
I would have liked to see a proper test for this case.
On 2018/06/14 03:17:39, hub wrote: > I would have liked to see a proper test for this case. I'll add one, thanks. 
|
