Patch will follow.
Created attachment 51560 [details] Patch
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.
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.
Attachment 51560 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1284011
Created attachment 51563 [details] Patch
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.
Created attachment 51564 [details] Patch
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 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?
(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.
Created attachment 51684 [details] Patch
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.
Attachment 51684 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1337006
Created attachment 51686 [details] Patch
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 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
Committed r56637: <http://trac.webkit.org/changeset/56637>