[webkit-reviews] review denied: [Bug 50617] ValidityState's exposed functions should check if willValidate() is true before all : [Attachment 76205] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Dai Mikurube
<dmikurube at google.com>'s request for review:
Bug 50617: ValidityState's exposed functions should check if willValidate() is
true before all
https://bugs.webkit.org/show_bug.cgi?id=50617

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76205&action=review

> LayoutTests/fast/forms/ValidityState-patternMismatch.html:66
> -/><input id="empty-pattern" type="text" pattern="" value="Lorem Ipsum"
> -/><input id="disabled" pattern="[0-9][A-Z]{3}" value="00AA" disabled />
> +
> +<input id="simple" pattern="[0-9][A-Z]{3}" value="0AAA"/>
> +
> +<input id="no-pattern-and-no-value" />
> +
> +<input id="ip-address" type="text"
pattern="\b(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0
-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[
0-9][0-9]?)\b" value="127.0.0.1" />
> +<input id="email-address" type="text"
pattern="[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-
z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?"
value="someone at somewhere.com" />
> +<input id="wrong-email-address" type="text"
pattern="[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-
z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?"
value="Wrong!"/>
> +
> +<input id="match-1" type="text"
pattern="\b(word1|word2|word3)(?:\W+\w+){1,6}?\W+(word1|word2|word3)\b"
value="word1 near word2" />
> +<input id="match-2" type="text" pattern=".{5,}" value="12345" />
> +<input id="match-3" type="text" pattern=".{5,}" value="*(@$!" />
> +<input id="match-4" type="text" pattern=".{5,}" value="*(@^$!" />
> +<input id="match-5" type="text" pattern="0|1|2|3|4|5|6|7|8|" value="3" />
> +<input id="match-6" type="text" pattern="^(foo).*" value="foo"/>
> +<input id="match-7" type="text" pattern="^(foo).*" value="fooo"/>
> +<input id="match-8" type="text" pattern="\w" value="f"/>
> +<input id="match-9" type="text" pattern="\." value="."/>
> +<input id="match-10" type="text" pattern="\)\(" value=")("/>
> +<input id="match-11" type="text" pattern="ab" value="ab"/>
> +<input id="match-12" type="text" pattern="^ab" value="ab"/>
> +<input id="match-13" type="text" pattern="^\(" value="("/>
> +<input id="match-14" type="text" pattern="...\)\(..." value="aaa)(bbb"/>
> +<input id="match-15" type="text" pattern="^" value=""/>
> +<input id="match-16" type="text" pattern="$" value=""/>
> +<input id="match-17" type="text" pattern="foobar" value=""/>
> +
> +<input id="wrong-gray-or-grey" type="text" pattern="gr[ae]y"
value="Wrong!"/>
> +<input id="gray" type="text" pattern="gr[ae]y" value="gray"/>
> +<input id="grey" type="text" pattern="gr[ae]y" value="grey"/>
> +<input id="empty-gray-or-grey" type="text" pattern="gr[ae]y" value=""/>
> +
> +<input id="mismatch-1" type="text" pattern="((4\.[0-3])|(2\.[0-3]))"
value="Something 4.0"/>
> +<input id="mismatch-2" type="text" pattern="\w" value="*"/>
> +<input id="mismatch-3" type="email" pattern="[0-9]" value="something"/>
> +<input id="mismatch-4" type="text" pattern=".{5,}" value="1234" />
> +<input id="mismatch-5" type="text" pattern=".{5,}" value="*)$!" />
> +<input id="mismatch-6" type="text" pattern=".{5,}" value="(^$!" />
> +<input id="mismatch-7" type="text" pattern="0|1|2|3|4|5|6|7|8|" value="a" />

> +<input id="mismatch-8" type="text" pattern="^(foo).*" value="a foo"/>
> +<input id="mismatch-9" type="text" pattern="\w" value="134"/>
> +<input id="mismatch-10" type="text" pattern="\." value="\."/>
> +<input id="mismatch-11" type="text" pattern="\)\(" value=") ("/>
> +<input id="mismatch-12" type="text" pattern="ab" value="a"/>
> +<input id="mismatch-13" type="text" pattern="ab" value="b"/>
> +<input id="mismatch-14" type="text" pattern="^ab" value="abc"/>
> +<input id="mismatch-15" type="text" pattern="^\(" value="(something"/>
> +<input id="mismatch-16" type="text" pattern="\)\\" value="something)\"/>
> +<input id="mismatch-17" type="text" pattern="...\)\([b]{3}"
value="adf)(bbbTEST"/>
> +<input id="mismatch-18" type="text" pattern="foo\\" value="food"/>
> +<input id="mismatch-19" type="text" pattern="^" value="wrong"/>
> +<input id="mismatch-20" type="text" pattern="$" value="wrong"/>
> +
> +<input id="empty-pattern" type="text" pattern="" value="Lorem Ipsum"/>
> +
> +<input id="disabled" pattern="[0-9][A-Z]{3}" value="00AA" disabled />
> +

You don't need to change this block, do you?

> WebCore/ChangeLog:9
> +	   ValidityState's exposed functions should check if willValidate() is
true before all
> +	   https://bugs.webkit.org/show_bug.cgi?id=50617
> +
> +	   Added checking willValidate() to exposed functions.
> +

We should mention the submit button behavior change.

> WebCore/ChangeLog:12
> +	   (WebCore::HTMLButtonElement::parseMappedAttribute): Added calling
setNeedsWillValidateCheck().
> +	   (WebCore::HTMLButtonElement::recalcWillValidate): Added.

It's obvious you added them.  You should write why you needed to add them.

> WebCore/ChangeLog:15
> +	   (WebCore::SubmitInputType::supportsValidation): Removed.

You should write the reason.

> WebCore/html/HTMLButtonElement.cpp:188
> +    return !disabled() && !readOnly();

We should use HTMLFormControlElement::recalcWillValidate().
  return m_type == SUBMIT && HTMLFormControlElement::recalcWillValidate();

> WebCore/html/ValidityState.cpp:209
> +    if (!element->willValidate())
> +	   return false;
> +
> +    return !m_customErrorMessage.isEmpty();

nit: We can write "return element->willValidate() &&
!m_customErrorMessage.isEmpty();"