[webkit-reviews] review granted: [Bug 22348] -webkit-autofill pseudo style not always respected : [Attachment 25286] Patch, changelog

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 22348: -webkit-autofill pseudo style not always respected
https://bugs.webkit.org/show_bug.cgi?id=22348

Attachment 25286: Patch, changelog
https://bugs.webkit.org/attachment.cgi?id=25286&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>      virtual bool isControl() const { return false; } // Eventually the
notion of what is a control will be extensible.
>      virtual bool isEnabled() const { return true; }
>      virtual bool isChecked() const { return false; }
> +    virtual bool isAutofilled() const { return false; }
>      virtual bool isIndeterminate() const { return false; }
>      virtual bool isReadOnlyControl() const { return false; }
>      virtual bool isTextControl() const { return false; }

Not sure why you chose this point in the list to add it. Not alphabetical.
Between checked and indeterminate seems a strange choice, since those two are
closely related.

>		   if (e && e->hasTagName(inputTag))
> -		       return static_cast<HTMLInputElement*>(e)->autofilled();
> +		       return
static_cast<HTMLInputElement*>(e)->isAutofilled();

If we're going to make isAutofilled virtual, then we should remove the
hasTagName check here. It's just there as a type check so we can call the
non-virtual function. We could also have put that check inside the code in
canShareStyleWithElement.

> +void HTMLInputElement::setAutofilled(bool b)
> +{
> +    if (b != m_autofilled) {
> +	   m_autofilled = b;
> +	   setChanged();
> +    }
> +}

We normally use early return for something like this rather than nesting both
lines inside an if statement.

r=me as is, but you could consider refining too