Bug 52788 – Any class other than Element should not use setShadowHost()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

Any class other than Element should not use setShadowHost(). 
They should use host->setShadowRoot(this) or host->removeShadowRoot() instead 
because only these APIs guarantee mutual-link between host and root.

[Roland Steiner]

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

[Dominic Cooney]

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.

[Dominic Cooney]

Created attachment 89980 [details]
Patch

[Dominic Cooney]

Comment on attachment 89980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89980&action=review

> Source/WebCore/ChangeLog:1
> [phone]  Dominic Cooney  <[e-mail]>

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.

[Dominic Cooney]

Blocks bug 58703.

[Roland Steiner]

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?

[Dimitri Glazkov (Google)]

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?

[Dominic Cooney]

Created attachment 90125 [details]
Patch

[Dominic Cooney]

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.

[Dominic Cooney]

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

[Dimitri Glazkov (Google)]

Comment on attachment 90125 [details]
Patch

ok

[WebKit Commit Bot]

Comment on attachment 90125 [details]
Patch

Clearing flags on attachment: 90125

Committed r84225: <http://trac.webkit.org/changeset/84225>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(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’
]]

[Daniel Bates]

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().

[Dominic Cooney]

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

[Antti Koivisto]

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.

[Antti Koivisto]

https://bugs.webkit.org/show_bug.cgi?id=69266