[webkit-reviews] review denied: [Bug 50309] HTML5 <details> and <summary> initial implementation : [Attachment 75427] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Luiz Agostini <luiz at webkit.org>'s
request for review:
Bug 50309: HTML5 <details> and <summary> initial implementation
https://bugs.webkit.org/show_bug.cgi?id=50309

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=75427&action=review

I don’t want to see these renderer classes added until we are sure we need
them. Are you certain you will need all three?

> WebCore/html/HTMLDetailsElement.cpp:55
> +bool HTMLDetailsElement::open() const
> +{
> +    return !getAttribute(openAttr).isNull();
> +}
> +
> +void HTMLDetailsElement::setOpen(bool value)
> +{
> +    setAttribute(openAttr, value ? "" : 0);
> +}

These functions are not needed.

> WebCore/html/HTMLDetailsElement.h:31
> +    static PassRefPtr<HTMLDetailsElement> create(Document* document);

We should not add this function unless we have a callsite that needs it.

> WebCore/html/HTMLDetailsElement.h:34
> +    bool open() const;
> +    void setOpen(bool);

These functions are not needed.

> WebCore/html/HTMLDetailsElement.idl:23
> +	   attribute boolean open;

This should use [Reflect] so we don’t need open and canOpen functions in the
C++ class.

> WebCore/html/HTMLTagNames.in:118
> -summary interfaceName=HTMLElement
> +summary

This change is incorrect. See the HTML5 specification. It specifically says the
DOM interface uses HTMLElement. The class HTMLSummaryElement is not needed.