Bug 31650

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 Flags
form_field_value
none
form_field_value
none
form_field_value none

Description Yaar Schnitman 2009-11-18 15:19:41 PST
Do some WebKit API changes to help FormFieldValues not used WebCore directly.
Comment 1 Yaar Schnitman 2009-11-18 15:21:48 PST
Created attachment 43467 [details]
form_field_value
Comment 2 Darin Fisher (:fishd, Google) 2009-11-18 15:39:41 PST
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
Comment 3 Yaar Schnitman 2009-11-18 17:59:03 PST
Created attachment 43476 [details]
form_field_value
Comment 4 Yaar Schnitman 2009-11-18 18:00:06 PST
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 5 Darin Fisher (:fishd, Google) 2009-11-19 16:07:46 PST
Comment on attachment 43476 [details]
form_field_value

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

^^^ nit: one too many new lines there.
Comment 6 Darin Fisher (:fishd, Google) 2009-11-19 16:08:02 PST
R=me w/ that nit fixed
Comment 7 Yaar Schnitman 2009-11-19 16:46:00 PST
Created attachment 43534 [details]
form_field_value
Comment 8 WebKit Commit Bot 2009-11-19 17:09:59 PST
Comment on attachment 43534 [details]
form_field_value

Clearing flags on attachment: 43534

Committed r51224: <http://trac.webkit.org/changeset/51224>
Comment 9 WebKit Commit Bot 2009-11-19 17:10:11 PST
All reviewed patches have been landed.  Closing bug.