Bug 36562 - [Chromium] Implement WebFormControlElement and WebSelectElement
Summary: [Chromium] Implement WebFormControlElement and WebSelectElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-24 15:29 PDT by James Hawkins
Modified: 2010-03-26 12:41 PDT (History)
3 users (show)

See Also:


Attachments
Patch (9.54 KB, patch)
2010-03-24 15:36 PDT, James Hawkins
no flags Details | Formatted Diff | Diff
Patch (22.36 KB, patch)
2010-03-24 16:14 PDT, James Hawkins
no flags Details | Formatted Diff | Diff
Patch (22.24 KB, patch)
2010-03-24 16:27 PDT, James Hawkins
no flags Details | Formatted Diff | Diff
Patch (22.36 KB, patch)
2010-03-25 15:37 PDT, James Hawkins
no flags Details | Formatted Diff | Diff
Patch (22.31 KB, patch)
2010-03-25 15:58 PDT, James Hawkins
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Hawkins 2010-03-24 15:29:01 PDT
Patch will follow.
Comment 1 James Hawkins 2010-03-24 15:36:57 PDT
Created attachment 51560 [details]
Patch
Comment 2 WebKit Review Bot 2010-03-24 15:38:19 PDT
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.
Comment 3 James Hawkins 2010-03-24 15:41:24 PDT
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.
Comment 4 WebKit Review Bot 2010-03-24 16:06:13 PDT
Attachment 51560 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1284011
Comment 5 James Hawkins 2010-03-24 16:14:16 PDT
Created attachment 51563 [details]
Patch
Comment 6 WebKit Review Bot 2010-03-24 16:21:05 PDT
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.
Comment 7 James Hawkins 2010-03-24 16:27:54 PDT
Created attachment 51564 [details]
Patch
Comment 8 WebKit Review Bot 2010-03-24 16:32:28 PDT
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.
Comment 9 Darin Fisher (:fishd, Google) 2010-03-25 15:20:07 PDT
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?
Comment 10 James Hawkins 2010-03-25 15:36:08 PDT
(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.
Comment 11 James Hawkins 2010-03-25 15:37:29 PDT
Created attachment 51684 [details]
Patch
Comment 12 WebKit Review Bot 2010-03-25 15:43:34 PDT
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.
Comment 13 WebKit Review Bot 2010-03-25 15:53:17 PDT
Attachment 51684 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1337006
Comment 14 James Hawkins 2010-03-25 15:58:46 PDT
Created attachment 51686 [details]
Patch
Comment 15 WebKit Review Bot 2010-03-25 16:04:54 PDT
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.
Comment 16 Darin Fisher (:fishd, Google) 2010-03-25 23:16:17 PDT
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
Comment 17 James Hawkins 2010-03-26 12:41:09 PDT
Committed r56637: <http://trac.webkit.org/changeset/56637>