Bug 58914

Summary: Content of <summary> should be forwarded through the shadow DOM
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Hajime Morrita <morrita>
Severity: Normal CC: abarth, dglazkov, eric, luiz, mdelaney7, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 56967, 56973    
Description Flags
WIP Take2
WIP Take3 - Added tests, almost ready.
Patch dglazkov: review+

Description Hajime Morrita 2011-04-19 12:02:08 PDT
We are going to introduce <content> like mechanics to handle this.
Comment 1 Hajime Morrita 2011-04-20 14:36:41 PDT
Created attachment 90418 [details]
Comment 2 Hajime Morrita 2011-04-20 15:49:51 PDT
Created attachment 90428 [details]
WIP Take2
Comment 3 Hajime Morrita 2011-04-20 18:37:14 PDT
Created attachment 90469 [details]
WIP Take3 - Added tests, almost ready.
Comment 4 Hajime Morrita 2011-04-20 18:51:16 PDT
Created attachment 90471 [details]
Comment 5 Hajime Morrita 2011-04-20 19:00:54 PDT
Created attachment 90476 [details]
Comment 6 Dimitri Glazkov (Google) 2011-04-20 20:47:23 PDT
Comment on attachment 90476 [details]

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

> Source/WebCore/ChangeLog:8
> +        - Introduced ShadowContentElement which hosts forwarded childrem of <summary>


> Source/WebCore/ChangeLog:11
> +        - The parent look up is also aware of node forwarding. If the visual parent node has

look up -> lookup

> Source/WebCore/ChangeLog:12
> +          a shadow root, the node is possibly forwarde to ShadowContentElement

forwarde -> forwarded

> Source/WebCore/dom/NodeVisualParentLookupResult.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

At least some of the code here comes from Node.cpp, so we should probably clone the copyrights.

> Source/WebCore/dom/NodeVisualParentLookupResult.h:34
> +class NodeVisualParentLookupResult {

The name is awkward. The point of the class is to determine the parent node for rendering purposes and whether a renderer for it is necessary. So perhaps NodeRendererFinder? Or something like that.

> Source/WebCore/dom/NodeVisualParentLookupResult.h:51
> +    ContainerNode* found() const { return m_found; }


> Source/WebCore/dom/ShadowRoot.cpp:101
> +void ShadowRoot::didHostChildrenChanged()


> Source/WebCore/dom/ShadowRoot.cpp:119
> +    for (Node* n = firstChild(); n; n = n->traverseNextNode(this)) {
> +        // FIXME: This should be replaced with tag-name checking once <content> is ready.
> +        // See also http://webkit.org/b/56973
> +        if (n->isShadowBoundary())
> +            return toContainerNode(n);
> +    }

This really bugs me, but I don't know if my idea would work. It seems that instead of re-traversing this all the time during child renderer creation, we should store the created renderers in a map, and then pull from that map as we create the renderer for <content> element.
Comment 7 Dimitri Glazkov (Google) 2011-04-20 20:51:48 PDT
Comment 8 Dimitri Glazkov (Google) 2011-04-20 20:52:09 PDT
(In reply to comment #7)
> Also,

... please adjust the title in the ChangeLog.
Comment 9 Dimitri Glazkov (Google) 2011-04-20 20:58:16 PDT
Comment on attachment 90476 [details]

Let discuss this in person tomorrow.
Comment 10 Dimitri Glazkov (Google) 2011-04-21 08:54:31 PDT
Comment on attachment 90476 [details]

I slept on it and I am now generally okay with the approach as a first stab. I would still like to explore the role of <content> being more active:
1) light children attach first, but all of their renderers are stashed in to a map
2) shadow DOM attaches next
3) any time a <content> element is found, it looks in the map, collects matching renderers, and inserts them into the render tree in its place.

Also, keep the new class in Node.cpp for now. We can split it out later. NodeRenderParentDetector seems like a good name.
Comment 11 Hajime Morrita 2011-04-21 09:52:07 PDT
Committed r84511: <http://trac.webkit.org/changeset/84511>
Comment 12 Matthew Delaney 2011-04-21 09:57:06 PDT
Committed r84512: <http://trac.webkit.org/changeset/84512>
Comment 13 WebKit Review Bot 2011-04-21 10:34:20 PDT
http://trac.webkit.org/changeset/84511 might have broken Qt Linux Release
The following tests are not passing:
Comment 14 WebKit Review Bot 2011-04-21 10:34:25 PDT
http://trac.webkit.org/changeset/84512 might have broken Qt Linux Release
The following tests are not passing: