[webkit-reviews] review denied: [Bug 68519] Refactoring: Move m_inclusions from ShadowInclusionSelector to ShadowRoot : [Attachment 108132] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 68519: Refactoring: Move m_inclusions from ShadowInclusionSelector to
ShadowRoot
https://bugs.webkit.org/show_bug.cgi?id=68519

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=108132&action=review


I like the direction, but this patch is too large and it's difficult to grasp
the exact nature of each change. Can we perhaps split it up and start with
simple renames?

> Source/WebCore/ChangeLog:13
> +	     No one other than ShadowContentElement doesn't care about
"selection" concept anymore.

that's a double negative. Did you mean to say "No one other than
ShadowContentElement cares about ..."?

> Source/WebCore/dom/NodeRenderingContext.h:80
>	   AttachContentLight,

This guy needs a lot of clarifying too.

> Source/WebCore/dom/NodeRenderingContext.h:81
> +	   AttachContentIncluded,

The name sounds a bit better, but we still need explanation. Perhaps a comment
above the enum to explain which phase means what?

> Source/WebCore/dom/ShadowInclusionSelector.cpp:127
> +    Vector<RefPtr<Node> >* selectableNodes = m_scope->selectableNodes();

Handing out a pointer to a member like this looks scary. Can this logic be
somehow encapsulated in ShadowInclusionScope?

> Source/WebCore/dom/ShadowRoot.cpp:162
> +    if (!m_inclusions)
> +	   return 0;
> +    return m_inclusions->find(node);

return m_inclusions ? m_inclusions->find(node) : 0;