[webkit-reviews] review denied: [Bug 35317] [Chromium] Implement WebLabelElement : [Attachment 49346] [Chromium] Implement WebLabelElement [try2]

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied James Hawkins
<jhawkins at google.com>'s request for review:
Bug 35317: [Chromium] Implement WebLabelElement
https://bugs.webkit.org/show_bug.cgi?id=35317

Attachment 49346: [Chromium] Implement WebLabelElement [try2]
https://bugs.webkit.org/attachment.cgi?id=49346&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebKit/chromium/public/WebLabelElement.h

> +    WebLabelElement(const WebLabelElement& n) : WebElement(n) { }
> +
> +    WebLabelElement& operator=(const WebLabelElement& n)
> +    {
> +	   WebElement::assign(n);
> +	   return *this;
> +    }
> +
> +    WEBKIT_API void assign(const WebLabelElement& n) {
WebElement::assign(n); }

^^^ nit:  s/n/e/ perhaps?  where "e" stands for element, but "n" doesn't seem
to
be related to the type name WebLabelElement.


> +#if WEBKIT_IMPLEMENTATION
> +    WebLabelElement(const WTF::PassRefPtr<WebCore::HTMLLabelElement>&);
> +    WebLabelElement& operator=(const
WTF::PassRefPtr<WebCore::HTMLLabelElement>&);
> +    operator WTF::PassRefPtr<WebCore::HTMLLabelElement>() const;
> +#endif
> +
> +    WEBKIT_API WebElement correspondingControl();
> +};

^^^ nit: please put the WEBKIT_IMPLEMENTATION section at the end of the class
to
help make the actual public API a bit more visible.


> Index: WebKit/chromium/src/WebLabelElement.cpp

> +WebLabelElement::WebLabelElement(const WTF::PassRefPtr<HTMLLabelElement>&
elem)
> +    : WebElement(elem)
> +{
> +}
> +
> +WebLabelElement& WebLabelElement::operator=(const
WTF::PassRefPtr<HTMLLabelElement>& elem)

nit: no need to mention WTF:: since there is a global 'using namespace WTF'