[webkit-reviews] review denied: [Bug 31650] Make chromium/webkit/glue/FormFieldValues use the WebKit API : [Attachment 43467] form_field_value

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Yaar Schnitman
<yaar at chromium.org>'s request for review:
Bug 31650: Make chromium/webkit/glue/FormFieldValues use the WebKit API
https://bugs.webkit.org/show_bug.cgi?id=31650

Attachment 43467: form_field_value
https://bugs.webkit.org/attachment.cgi?id=43467&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ 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