Bug 31650 – Make chromium/webkit/glue/FormFieldValues use the WebKit API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Yaar Schnitman]

Do some WebKit API changes to help FormFieldValues not used WebCore directly.

[Yaar Schnitman]

Created attachment 43467 [details]
form_field_value

[Darin Fisher (:fishd, Google)]

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

[Yaar Schnitman]

Created attachment 43476 [details]
form_field_value

[Yaar Schnitman]

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.

[Darin Fisher (:fishd, Google)]

Comment on attachment 43476 [details]
form_field_value

> +++ b/WebKit/chromium/src/WebInputElement.cpp
...
> +}
> +    
> +
>  } // namespace WebKit

^^^ nit: one too many new lines there.

[Darin Fisher (:fishd, Google)]

R=me w/ that nit fixed

[Yaar Schnitman]

Created attachment 43534 [details]
form_field_value

[WebKit Commit Bot]

Comment on attachment 43534 [details]
form_field_value

Clearing flags on attachment: 43534

Committed r51224: <http://trac.webkit.org/changeset/51224>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.