[Webkit-unassigned] [Bug 36562] [Chromium] Implement WebFormControlElement and WebSelectElement

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36562





--- Comment #10 from James Hawkins <jhawkins at chromium.org>  2010-03-25 15:36:08 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.