[webkit-reviews] review granted: [Bug 58914] Content of <summary> should be stayed inside shadow : [Attachment 90476] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has granted MORITA Hajime
<morrita at google.com>'s request for review:
Bug 58914: Content of <summary> should be stayed inside shadow
https://bugs.webkit.org/show_bug.cgi?id=58914

Attachment 90476: Patch
https://bugs.webkit.org/attachment.cgi?id=90476&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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>

childrem->children

> 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; }

parentNodeForRenderingAndStyle?

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

hostChildrenChanged

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