Currently getElementById() doesn't work with ShadowRoot which is attached to orphan host. But it should work.
At the first point, it might be OK to search everything, I think. We could enhance the performance later.
Created attachment 177932 [details] Modify insertedInto
This version has performance regression by 2~3% on Dromaeo/dom-modify.html 35.98 runs/s -> 35.10 runs/s I don't think this is acceptable. Let me consider another approach.
Created attachment 177946 [details] Patch
Comment on attachment 177946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177946&action=review > Source/WebCore/dom/TreeScope.cpp:103 > + if (Element* element = m_elementsById ? m_elementsById->getElementById(elementId.impl(), this) : 0) Can we have this in ShadowRoot.cpp ? It would be cleaner.
Comment on attachment 177946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177946&action=review >> Source/WebCore/dom/TreeScope.cpp:103 >> + if (Element* element = m_elementsById ? m_elementsById->getElementById(elementId.impl(), this) : 0) > > Can we have this in ShadowRoot.cpp ? > It would be cleaner. I think m_elementsById should not be exposed to ShadowRoot. The other things can be moved to ShadowRoot.cpp.
> > It would be cleaner. > > I think m_elementsById should not be exposed to ShadowRoot. The other things can be moved to ShadowRoot.cpp. Cannot we just call TreeScope::getElementById() for non-orphan case?
(In reply to comment #7) > > > It would be cleaner. > > > > I think m_elementsById should not be exposed to ShadowRoot. The other things can be moved to ShadowRoot.cpp. > > Cannot we just call TreeScope::getElementById() for non-orphan case? Ah, you mean we should create ShadowRoot::getElementById(), right? That makes sense.
Created attachment 177970 [details] Patch
Comment on attachment 177970 [details] Patch Attachment 177970 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15181031
Hmm... instead of exposing the symbol, I would like to kill Internals.getElementByIdShadowRoot. It's nonsense now.
Comment on attachment 177970 [details] Patch Could you add test like this? - Append child during the shadow is in the document, - then remove the host from document, - and finallly exercise against that now-orphan shadow root.
(In reply to comment #12) > (From update of attachment 177970 [details]) > Could you add test like this? > > - Append child during the shadow is in the document, > - then remove the host from document, > - and finallly exercise against that now-orphan shadow root. OK.
Comment on attachment 177970 [details] Patch Attachment 177970 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15175180
Created attachment 177982 [details] Patch
This patch can be built after https://bugs.webkit.org/show_bug.cgi?id=104241 is resolved.
Comment on attachment 177982 [details] Patch Attachment 177982 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15154820
Comment on attachment 177982 [details] Patch Attachment 177982 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15144929
Created attachment 178102 [details] Patch
(In reply to comment #3) > This version has performance regression by 2~3% on Dromaeo/dom-modify.html > > 35.98 runs/s -> 35.10 runs/s > > I don't think this is acceptable. Was this caused by Bug 87034?
(In reply to comment #20) > (In reply to comment #3) > > This version has performance regression by 2~3% on Dromaeo/dom-modify.html > > > > 35.98 runs/s -> 35.10 runs/s > > > > I don't think this is acceptable. > > Was this caused by Bug 87034? I'm not 100% sure, but since insertedInto() is very host function, I'm thinking that adding if-condition can regress dom-modify performance. At least, I tested the first version of the patch after Bug 87034 has been resolved. So I don't think such regression is mainly caused by the slowness of treeScope(). # If treeScope() is still too slow, I'll change my mind.
This seems like a workaround for a bigger issue: the inDocument is not expressive enough to communicate the state of the tree. In terms of shadow trees, an element in that tree should always seem inDocument, even if it's hosted by an orphan element. However, this means that we may have a situation that the root is inDocument, but the host is not. That's bad. Customizing getElementById for ShadowRoot is just masking this issue. Let's fix the real problem :)
(In reply to comment #22) > This seems like a workaround for a bigger issue: the inDocument is not expressive enough to communicate the state of the tree. > > In terms of shadow trees, an element in that tree should always seem inDocument, even if it's hosted by an orphan element. However, this means that we may have a situation that the root is inDocument, but the host is not. That's bad. > > Customizing getElementById for ShadowRoot is just masking this issue. Let's fix the real problem :) I agree this, but the correct fix will take longer. My suggestion here is that we could have workaround now and fix it since making Shadow DOM available without making this completely broken will be a disaster. We could possibly change title of Bug 104223 for let the problem visible.
My current plan is to introduce InShadowTreeFlag and make have a method isInTreeScope() (name should be reconsidered) which returns "inDocument() || isInShadowTree()" (actually we use bit-and to return it). Also, I'll move setInDocument()/clearInDocument() from insertedInto()/removedFrom() to ContainerNodeAlgorithm. I'm thinking this doesn't impact performance
Created attachment 179193 [details] WIP
Comment on attachment 179193 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=179193&action=review > Source/WebCore/dom/ContainerNodeAlgorithms.h:63 > + void notifyDescendantRemovedFromDocument(ContainerNode*, Node::TreeScopeTypes); You could define another enum here for clarity. It's really confusing to see the flag name to be cleared here. Let's give more intention revealing name.
The last WIP patch regresses performance by 0.5%. (37.71runs/s -> 37.45 runs/s, Dromaeo/dom-modify.html) It would be acceptable, but let me try to improve more.
Comment on attachment 179193 [details] WIP Attachment 179193 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15321098 New failing tests: fast/forms/radio/radio-group.html
Created attachment 179204 [details] WIP
This does not regress performance anymore. I would like make code cleaner.
It seems this still regress Parser/html5-full-render.html performance. Ohya
Created attachment 179234 [details] WIP
Comment on attachment 179234 [details] WIP Attachment 179234 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15322171 New failing tests: fast/forms/radio/radio-group.html
Comment on attachment 179234 [details] WIP Attachment 179234 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15311216
Comment on attachment 179234 [details] WIP Attachment 179234 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15323212 New failing tests: fast/forms/radio/radio-group.html
Created attachment 179423 [details] Patch
Created attachment 179424 [details] Performance Results (first 2 is without this patch, last 2 is with this patch)
Created attachment 179425 [details] Patch
Comment on attachment 179425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179425&action=review It's surprising to see this doesn't hurt perf. But well, that happens. Wait for bots beging green. > Source/WebCore/dom/Node.cpp:2098 > + if (parentNode() && parentNode()->isInShadowTree()) You can use parentOrHostNode() here. It will be a bit faster.
(In reply to comment #39) > (From update of attachment 179425 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179425&action=review > > It's surprising to see this doesn't hurt perf. But well, that happens. Wait for bots beging green. > > > Source/WebCore/dom/Node.cpp:2098 > > + if (parentNode() && parentNode()->isInShadowTree()) > > You can use parentOrHostNode() here. It will be a bit faster. Good point. Thanks!
Created attachment 179428 [details] Patch for landing
Comment on attachment 179428 [details] Patch for landing Clearing flags on attachment: 179428 Committed r137731: <http://trac.webkit.org/changeset/137731>
All reviewed patches have been landed. Closing bug.