Bug 52788

Agreed. In addition, neither should be called from the constructor.

Why should neither setShadowRoot/Host be called from the constructor? As part of bug 57813 I am removing setShadowRoot in favor of ensureShadowRoot. So setShadowHost could become a protocol between Element::ensureShadowRoot and ShadowRoot::create. However there are still some uses of setShadowHost with FIXMEs that we need to clean up to make this happen.

Created attachment 89980 [details] Patch

Comment on attachment 89980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89980&action=review > Source/WebCore/ChangeLog:1 > +2011-04-17 Dominic Cooney <dominicc@chromium.org> WIP This divorces SVGShadowRootElement from ShadowRoot. Now ShadowRoot owns the IsShadowRoot flag and shadowHost accessor and SVGShadowRootElement gets an isSVGShadowRoot virtual and svgShadowHost accessor. The divorce is moderately messy because a bunch of special cases for shadows are now two-way tests. Because there is often special-casing for SVG nearby, I wonder if a more aggressive clean-up will yield a cleaner result… Alternatively, we could try replacing SVGShadowRootElement with ShadowRoot. What makes that tricky is that ShadowRoot isn’t an SVGElement, which a lot of SVG-special casing is predicated on.

Blocks bug 58703.

I very much like the clear distinction of SVGShadowRoot vs. "normal" (non-SVG) shadow root in your patch! One question: So far, setShadowHost set/cleared the IsShadowRootFlag - how will this be handled with isSVGShadowRoot? Will only SVG shadow root elements have this flag set, or will it also be set in ShadowRoot instances?

Comment on attachment 89980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89980&action=review Looks great, just a few nits. > Source/WebCore/dom/EventDispatcher.cpp:136 > + if ((*i)->isShadowRoot() || (*i)->isSVGShadowRoot()) { This (and hereon) should be an inline helper function. > Source/WebCore/dom/EventDispatcher.cpp:246 > + ancestor = ancestor->isShadowRoot() ? ancestor->shadowHost() : ancestor->svgShadowHost(); You are querying isShadowRoot twice here. Perhaps optimize the loop a bit?

Created attachment 90125 [details] Patch

Comment on attachment 90125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90125&action=review > Source/WebCore/dom/EventDispatcher.cpp:248 > + bool isSVGShadowRoot = ancestor->isSVGShadowRoot(); This avoids doing the test twice. I tried something more adventurous splitting this into three branches for shadows, SVG shadows and ordinary nodes, but I couldn’t find a clean way to make it work without some (excessive?) duplication. But I could try with fresh eyes tomorrow.

Roland: Ultimately IsShadowRoot flag is only set for ShadowRoot instances (and could be made a virtual as follow-up clean-up.)

Comment on attachment 90125 [details] Patch ok

Comment on attachment 90125 [details] Patch Clearing flags on attachment: 90125 Committed r84225: <http://trac.webkit.org/changeset/84225>

All reviewed patches have been landed. Closing bug.

(In reply to comment #13) > All reviewed patches have been landed. Closing bug. This patch broke the Qt Linux Release minimal build: <http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/23373/steps/compile-webkit/logs/stdio> In particular, the error is: [[ ../../../Source/WebCore/dom/EventDispatcher.cpp: In member function ‘void WebCore::EventDispatcher::ensureEventAncestors(WebCore::Event*)’: ../../../Source/WebCore/dom/EventDispatcher.cpp:252: error: ‘class WebCore::Node’ has no member named ‘svgShadowHost’ ]]

Committed attempt to fix the build in changeset 84226 <http://trac.webkit.org/changeset/84226>. I added an ENABLE(SVG) guard around the call to Node::svgShadowHost() since this method is only defined when building with SVG enabled. I was unsure whether this was the best place to add the guard or to add it around the local variable isSVGShadowRoot, such that isSVGShadowRoot is defined to be false when building with SVG disabled. I decided to place the guard closest to the call that required it, Node::svgShadowHost().

Sorry I broke this. The placement of the #if in r84226 looks good to me.

This added virtual function call (isSVGShadowRoot()) to Node::parentNode(): inline ContainerNode* Node::parentNode() const { - return getFlag(IsShadowRootFlag) ? 0 : parent(); + return getFlag(IsShadowRootFlag) || isSVGShadowRoot() ? 0 : parent(); } This is not acceptable from performance perspective. I noticed the regression from samples.