[webkit-reviews] review denied: [Bug 58591] RenderDetailsMarker should belong to shadow element. : [Attachment 89816] Windows build fix

[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 58591: RenderDetailsMarker should belong to shadow element.
https://bugs.webkit.org/show_bug.cgi?id=58591

Attachment 89816: Windows build fix
https://bugs.webkit.org/attachment.cgi?id=89816&action=review

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

overall, looks great. Except I think you need to really simplify the plumbing
for rendering of children vs. shadow tree. We are getting rid of this soon
anyway.

> Source/WebCore/dom/Node.h:96
> +enum ShadowBoundaryType {
> +    ShadowBoundaryLight,
> +    ShadowBoundaryShadowExclusive,
> +    ShadowBoundaryShadowBeforeLight,
> +    ShadowBoundaryShadowAfterLight
> +};

Why do we have so many types? These are all hacks that we will eventually
remove, and we only use two of these types.

Also, this needs better name. The enum describes how shadow nodes are
renderered in relation to 
child nodes. So maybe ShadowRenderersPositions { BeforeChildren, AfterChildren
}?

> Source/WebCore/dom/Node.h:217
> +    virtual ShadowBoundaryType shadowBoundaryType() const { return
ShadowBoundaryLight; }

I think this is backwards. We should just have a virtual method on a host
element, saying whether or not children should be rendered (false by default).
Then let HTMLSummaryElement set it to true. No need to tweak ShadowRoot at all.


> Source/WebCore/html/HTMLSummaryElement.cpp:60
> +HTMLDetailsElement* HTMLSummaryElement::hostDetails() const

should be detailsElement.

> Source/WebCore/html/shadow/DetailsControlElements.cpp:51
> +    if (result)
> +	   result->setAnimatableStyle(style);

Don't need this. Style is set just after createRenderer in
Node::createRendererAndStyle.

> Source/WebCore/html/shadow/DetailsControlElements.cpp:66
> +HTMLSummaryElement* DetailsMarkerControl::hostSummary()

summaryElement.

> Source/WebCore/html/shadow/DetailsControlElements.h:31
> +#ifndef DetailsControlElements_h

Should just be DetailsMarkerControl. Don't follow the example of
MediaControlElements -- it's an antipattern. All these classes should be split
out into separate files.