[webkit-reviews] review denied: [Bug 36562] [Chromium] Implement WebFormControlElement and WebSelectElement : [Attachment 51564] Patch

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebKit/chromium/public/WebElement.h
...
> +	   WEBKIT_API bool isEnabledFormControl() const;

I wonder... could this just be isFormControl()?  Then, on
WebFormControlElement, could we have an isEnabled() method?

I realize that you are just replicating what WebCore does.
Would it be hard to support the interface I'm recommending?
It seems cleaner to me.


> Index: WebKit/chromium/public/WebFormControlElement.h

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

nit: n -> e ("e for element")


> +    WEBKIT_API WebString formControlName() const;
> +    WEBKIT_API WebString formControlType() const;
> +    // Returns the name that should be used for the specified |element| when


nit: insert a new line above this comment


> Index: WebKit/chromium/public/WebInputElement.h
...
> +	   WebInputElement(const WebInputElement& n) : WebFormControlElement(n)
{ }
>  
> +	   WebInputElement& operator=(const WebInputElement& n) {
WebFormControlElement::assign(n); return *this; }
> +	   WEBKIT_API void assign(const WebInputElement& n) {
WebFormControlElement::assign(n); }

nit: n -> e ("e for element")


> Index: WebKit/chromium/public/WebSelectElement.h

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

nit: n -> e ("e for element")


> +
> +    WEBKIT_API void setValue(const WebString& value);

nit: webkit style says to not name the parameter here since the name
doesn't add information.


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

> +void WebSelectElement::setAutofilled(bool)
> +{
> +
> +}

^^^ should this have an implementation?