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

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Luiz Agostini <luiz at webkit.org>'s
request for review:
Bug 51071: RenderDetails: layout the first <summary> element child of a
<details> element
https://bugs.webkit.org/show_bug.cgi?id=51071

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80928&action=review

> Source/WebCore/rendering/RenderDetails.cpp:109
> +	   m_interactiveArea = IntRect(m_mainSummary->logicalLeft(),
m_mainSummary->logicalTop(), m_mainSummary->logicalWidth(),
m_mainSummary->logicalHeight());

This seems hard-coded to horizontal text.

> Source/WebCore/rendering/RenderDetailsMarker.cpp:107
> +static Path getCannonicalPath(bool isOpen, bool isLtr)

This should be getCanonicalPath.  It also seems like this appearance may be
hardcoded assuming a particular orientation (horizontal).  Have you tested this
element in vertical text mode (and/or decided what it's going to do in that
mode)?

> Source/WebCore/rendering/RenderObject.cpp:135
> +    if (node->hasTagName(summaryTag))
> +	   return new (arena) RenderSummary(node);

Since this is tied directly to a specific DOM element, don't put this in the
generic RenderObject creation method.  That's reserved only for objects that
have display types.  Assuming there is a DOM subclass for HTMLSummaryElement,
put the creation there.

> Source/WebCore/rendering/RenderSummary.cpp:86
> +RenderObject* RenderSummary::getParentOfFirstLineBox(RenderBlock* curr)

This looks like a duplication of the RenderListItem function, even down to the
odd quirks mode behavior for lists.  I'm not sure why you'd want to propagate
list item quirks into a brand new HTML5 element.