Bug 36562 – [Chromium] Implement WebFormControlElement and WebSelectElement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[James Hawkins]

Patch will follow.

[James Hawkins]

Created attachment 51560 [details]
Patch

[WebKit Review Bot]

Attachment 51560 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/public/WebInputElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebInputElement.h:48:  More than one command on the same line  [whitespace/newline] [4]
WebKit/chromium/public/WebFormElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Hawkins]

I'm ignoring the style errors, as the new code matches the old.  Let me know if
I should update the entire file to fix the style errors.

[WebKit Review Bot]

Attachment 51560 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1284011

[James Hawkins]

Created attachment 51563 [details]
Patch

[WebKit Review Bot]

Attachment 51563 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/public/WebFormControlElement.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebFormControlElement.h:49:  More than one command on the same line  [whitespace/newline] [4]
WebKit/chromium/public/WebFormElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebSelectElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebSelectElement.h:48:  More than one command on the same line  [whitespace/newline] [4]
WebKit/chromium/public/WebInputElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebInputElement.h:48:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 7 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Hawkins]

Created attachment 51564 [details]
Patch

[WebKit Review Bot]

Attachment 51564 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/public/WebFormControlElement.h:49:  More than one command on the same line  [whitespace/newline] [4]
WebKit/chromium/public/WebFormElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebSelectElement.h:48:  More than one command on the same line  [whitespace/newline] [4]
WebKit/chromium/public/WebInputElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebInputElement.h:48:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 51564 [details]
Patch

> 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?

[James Hawkins]

(In reply to comment #9)
> (From update of attachment 51564 [details])
> > 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.
> 

Yes, I agree with you.  I've made this change.

> > 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")
> 

Done.

> > +    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
> 

Done.

> > 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")
> 

Done.

> > 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")
> 

Done.

> 
> > +
> > +    WEBKIT_API void setValue(const WebString& value);
> 
> nit: webkit style says to not name the parameter here since the name
> doesn't add information.
> 

This is the style rule I really need to ingrain in my head.  Done.

> 
> > Index: WebKit/chromium/src/WebSelectElement.cpp
> 
> > +void WebSelectElement::setAutofilled(bool)
> > +{
> > +
> > +}
> 
> ^^^ should this have an implementation?

I removed the method, as I'm not sure it's necessary, nor do I know how it would be implemented (in the UX sense) if it were.

[James Hawkins]

Created attachment 51684 [details]
Patch

[WebKit Review Bot]

Attachment 51684 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/public/WebFormControlElement.h:49:  More than one command on the same line  [whitespace/newline] [4]
WebKit/chromium/public/WebFormElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebSelectElement.h:48:  More than one command on the same line  [whitespace/newline] [4]
WebKit/chromium/public/WebInputElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebInputElement.h:48:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 51684 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1337006

[James Hawkins]

Created attachment 51686 [details]
Patch

[WebKit Review Bot]

Attachment 51686 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/public/WebFormControlElement.h:49:  More than one command on the same line  [whitespace/newline] [4]
WebKit/chromium/public/WebFormElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebSelectElement.h:48:  More than one command on the same line  [whitespace/newline] [4]
WebKit/chromium/public/WebInputElement.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebInputElement.h:48:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 51686 [details]
Patch

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

The style bot is saying that this should be rewritten as:

    WebFormControlElement& operator=(const WebFormControlElement& e)
    {
        WebElement::assign(e);
        return *this;
    }

R=me otherwise

[James Hawkins]

Committed r56637: <http://trac.webkit.org/changeset/56637>