[webkit-reviews] review granted: [Bug 34733] :invalid only works when input is in a form : [Attachment 52648] Proposed patch (rev.3)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 34733: :invalid only works when input is in a form
https://bugs.webkit.org/show_bug.cgi?id=34733

Attachment 52648: Proposed patch (rev.3)
https://bugs.webkit.org/attachment.cgi?id=52648&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    , m_willValidate(false)
> +    , m_willValidateInitialized(false)
> +    , m_willValidate(true)

I am not sure that "initialized" is the right word here. Since you called the
function "recalc" maybe you should reverse the sense and call it
"m_willValidateNeedsRecalc" or "m_needsWillValidateRecalc"? Or maybe this is
needed only during construction. Maybe we can make m_willValidateInitialized be
a debug-only concept?

>      // FIXME: Check if the control does not have a datalist element as an
ancestor.
> -    return m_form && m_hasName && !m_disabled && !m_readOnly;
> +    return !m_disabled && !m_readOnly;

It would be good to make that existing FIXME clearer and more specific. Check
that, and do what? Why check it?

>      bool newWillValidate = recalcWillValidate();
> -    if (m_willValidate == newWillValidate)
> +    if (m_willValidateInitialized && m_willValidate == newWillValidate)
>	   return;
> +    m_willValidateInitialized = true;

This seems a little strange. Do we really need to recalc immediately here?
There at least needs to be a comment explaining why this doesn't just set
m_willValidateInitialized to false and return.

r=me