Summary: | Make chromium/webkit/glue/FormFieldValues use the WebKit API | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yaar Schnitman <yaar> | ||||||||
Component: | WebKit Misc. | Assignee: | Yaar Schnitman <yaar> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, fishd | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Yaar Schnitman
2009-11-18 15:19:41 PST
Created attachment 43467 [details]
form_field_value
Comment on attachment 43467 [details] form_field_value > +++ b/WebKit/chromium/public/WebFormElement.h > + WEBKIT_API void getNamedElements(const WebString&, WebVector<WebNode>&); > + WEBKIT_API void getInputElements(WebVector<WebInputElement>&) const; not relevant to this patch, but... getNamedElements seems unfortunately named. at least, it seems like the output type should be WebVector<WebElement> instead of WebVector<WebNode>. also, it seems that getNamedElements should be const. perhaps we can make some changes in a follow-up patch. > +++ b/WebKit/chromium/public/WebInputElement.h ... > + enum InputType { ... > + Url, s/Url/URL/ each letter of an acronym should be uniformly capitalized. > + WEBKIT_API bool isEnabledFormControl() const; > + WEBKIT_API InputType inputType() const; nit: can we just use "type" as the name of this method and Type instead of InputType as the name of the enum? Since the enum is defined within a class named WebInputElement, it seems redundant for its name to include the term "Input". i don't see the implementation of a number of these methods: > + WEBKIT_API WebString formControlType() const; > + WEBKIT_API void setActivatedSubmit(bool); can you add some comment explaining what setActivatedSubmit does? > + WEBKIT_API void setValue(const WebString& value); > + WEBKIT_API WebString value() const; > + WEBKIT_API void setAutofilled(bool); > + WEBKIT_API void dispatchFormControlChangeEvent(); > + WEBKIT_API void setSelectionRange(size_t, size_t); WebRange uses int instead of size_t for ranges. perhaps these should use int as well. > + WEBKIT_API WebString name() const; nit: please insert a new line above the comment block. > + // Returns the name that should be used for the specified |element| when > + // storing autofill data. This is either the field name or its id, an empty > + // string if it has no name and no id. > + WEBKIT_API WebString nameForAutofill() const; > +++ b/WebKit/chromium/public/WebNode.h > - WebFrame* frame(); > + WebFrame* frame() const; ^^^ you should sync, i've already landed this change > +++ b/WebKit/chromium/src/DOMUtilitiesPrivate.cpp > -String nameOfInputElement(HTMLInputElement* element) it looks like this is still referenced by the chromium repository. i wouldn't get rid of it until the chromium repository stops referencing this method. how about just adding a comment indicating that it is deprecated for now? that way the two-sided patching is simpler. > +WebString WebInputElement::nameForAutofill() const > +{ > + ^^^ nit: remove the extra new line Created attachment 43476 [details]
form_field_value
Feedback incorporated. Except: In reply to comment #2) > getNamedElements seems unfortunately named. at least, it seems like the output > type should be WebVector<WebElement> instead of WebVector<WebNode>. also, it > seems that getNamedElements should be const. perhaps we can make some changes > in a follow-up patch. > Its a thin wrapper around HTMLInputElement->getNamedElements, which by itself returns a vector of nodes, not elements. That getNamedElements is also not const. So the fix should be done in webcore too. > > + WEBKIT_API InputType inputType() const; > > nit: can we just use "type" as the name of this method and Type instead > of InputType as the name of the enum? Since the enum is defined within > a class named WebInputElement, it seems redundant for its name to include > the term "Input". I don't want to diverge from webcore which uses inputType too. There might one day also be Node::NodeTypes so I think it will be better if we stick with InputType and inputType to prevent collisions. Comment on attachment 43476 [details] form_field_value > +++ b/WebKit/chromium/src/WebInputElement.cpp ... > +} > + > + > } // namespace WebKit ^^^ nit: one too many new lines there. R=me w/ that nit fixed Created attachment 43534 [details]
form_field_value
Comment on attachment 43534 [details] form_field_value Clearing flags on attachment: 43534 Committed r51224: <http://trac.webkit.org/changeset/51224> All reviewed patches have been landed. Closing bug. |