[webkit-reviews] review denied: [Bug 80392] [Forms] Introduce LabelableElement to share "labels" attribute implementation : [Attachment 130347] Patch 4

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 80392: [Forms] Introduce LabelableElement to share "labels" attribute
implementation
https://bugs.webkit.org/show_bug.cgi?id=80392

Attachment 130347: Patch 4
https://bugs.webkit.org/attachment.cgi?id=130347&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130347&action=review


> Source/WebCore/ChangeLog:11
> +	   This patch introduces new class LabelableElement for base class of
> +	   HTMLFormControlElement and moves isLabelable method to HTMLElement.
> +
> +	   No new tests are required. No behavior changes.

You should explain a reason why you want to introduce LabelableElement in
ChangeLog.

> Source/WebCore/html/HTMLElement.h:96
> +    virtual bool isLabelable() const { return false; }
> +

You don't need to define isLabelable() in HTMLElement.	Defining it in
LabelableElement is enough.

> Source/WebCore/html/HTMLFormControlElement.cpp:-470
> -bool HTMLFormControlElement::isLabelable() const

Because you remove some code, you can remove some #include in this file.

> Source/WebCore/html/HTMLOutputElement.h:61
> +    virtual bool isLabelable() const OVERRIDE { return true; }

The original code doesn't support HTMLOutputElement.  This is a behavior change
and you need to write a test.

> Source/WebCore/html/LabelableElement.h:38
> +// LabelableElement is the default implementation of labels attribute.

This comment is confusing.  Usually an element is not an implementation of an
element, not an attribute.
The comment should be something like the following:

LabelableElement represents "labelable element" defined in the HTML
specification, and provides the implementation of the "labels" attribute.

> Source/WebCore/html/LabelableElement.h:48
> +} // namespace

The commet is confusing.  This doesn't close the anonymous namespace.
Please remove the comment, or make it "namespace WebCore".