[webkit-reviews] review denied: [Bug 51071] RenderDetails: the first <summary> element child of a <details> element : [Attachment 76592] patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Luiz Agostini
<luiz at webkit.org>'s request for review:
Bug 51071: RenderDetails: the first <summary> element child of a <details>
element
https://bugs.webkit.org/show_bug.cgi?id=51071

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76592&action=review

> WebCore/ChangeLog:5
> +	   RenderDetails: the first <summary> element child of a <details>
element

I do not understand this description. It is like something is missing.

Maybe: Render the first summary element who is a child of a details element? or
similar.

> WebCore/rendering/RenderDetails.cpp:44
> +RenderBox* RenderDetails::findSummary() const

Maybe firstSummary()? It sounds from the change log like there can be more

> WebCore/rendering/RenderDetails.cpp:46
> +    for (RenderObject* summary = firstChild(); summary; summary =
summary->nextSibling()) {

I guess this 'summary' is really potentialSummary? I guess calling it child
instead would be better. What is used elsewhere?

> WebCore/rendering/RenderDetails.cpp:53
> +RenderObject* RenderDetails::layoutSpecialExcludedChild(bool
relayoutChildren)

>From the function name im not sure what exactly it does. It is the child that
is special? or are you doing a special layout?

> WebCore/rendering/RenderDetails.cpp:56
> +    if (summary) {

an early return seems better