Bug 68519 – Refactoring: Move m_inclusions from ShadowInclusionSelector to ShadowRoot

• Status: NEW
• Component: DOM

[Hajime Morrita]

This should de-emphasize ShadowInclusionSelector concept and hopefully make inclusion related code readable.

[Hajime Morrita]

Created attachment 108132 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 108132 [details]
Patch

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;