Bug 52788 - Any class other than Element should not use setShadowHost()
Summary: Any class other than Element should not use setShadowHost()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 52905 59005
Blocks: 48698 52963 54179 58703
  Show dependency treegraph
 
Reported: 2011-01-19 23:40 PST by Hajime Morrita
Modified: 2011-10-03 09:39 PDT (History)
9 users (show)

See Also:


Attachments
Patch (13.58 KB, patch)
2011-04-17 18:48 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (14.08 KB, patch)
2011-04-18 18:39 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-01-19 23:40:02 PST
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.
Comment 1 Roland Steiner 2011-02-07 00:42:01 PST
Agreed. In addition, neither should be called from the constructor.
Comment 2 Dominic Cooney 2011-04-07 13:11:04 PDT
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.
Comment 3 Dominic Cooney 2011-04-17 18:48:54 PDT
Created attachment 89980 [details]
Patch
Comment 4 Dominic Cooney 2011-04-17 18:53:49 PDT
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.
Comment 5 Dominic Cooney 2011-04-18 14:24:10 PDT
Blocks bug 58703.
Comment 6 Roland Steiner 2011-04-18 14:54:12 PDT
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 7 Dimitri Glazkov (Google) 2011-04-18 15:27:00 PDT
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?
Comment 8 Dominic Cooney 2011-04-18 18:39:33 PDT
Created attachment 90125 [details]
Patch
Comment 9 Dominic Cooney 2011-04-18 18:45:44 PDT
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.
Comment 10 Dominic Cooney 2011-04-18 18:47:05 PDT
Roland: Ultimately IsShadowRoot flag is only set for ShadowRoot instances (and could be made a virtual as follow-up clean-up.)
Comment 11 Dimitri Glazkov (Google) 2011-04-18 19:42:10 PDT
Comment on attachment 90125 [details]
Patch

ok
Comment 12 WebKit Commit Bot 2011-04-18 22:44:37 PDT
Comment on attachment 90125 [details]
Patch

Clearing flags on attachment: 90125

Committed r84225: <http://trac.webkit.org/changeset/84225>
Comment 13 WebKit Commit Bot 2011-04-18 22:44:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Daniel Bates 2011-04-19 00:23:10 PDT
(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’
]]
Comment 15 Daniel Bates 2011-04-19 00:31:26 PDT
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().
Comment 16 Dominic Cooney 2011-04-19 00:35:40 PDT
Sorry I broke this. The placement of the #if in r84226 looks good to me.
Comment 17 Antti Koivisto 2011-10-03 09:31:45 PDT
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.
Comment 18 Antti Koivisto 2011-10-03 09:39:42 PDT
https://bugs.webkit.org/show_bug.cgi?id=69266