Summary: | [Chromium] Implement WebFormControlElement and WebSelectElement | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Hawkins <jhawkins> | ||||||||||||
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | dglazkov, fishd, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
James Hawkins
2010-03-24 15:29:01 PDT
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> |