[webkit-reviews] review denied: [Bug 55987] input type=email and multiple is not compliant : [Attachment 98685] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kentaro Hara <haraken at google.com>'s
request for review:
Bug 55987: input type=email and multiple is not compliant
https://bugs.webkit.org/show_bug.cgi?id=55987

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

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

> LayoutTests/fast/forms/ValidityState-typeMismatch-email-expected.txt:6
> +PASS "something at something.com" is a correct valid email address. It was
sanitized to "something at something.com".

The test result readability is not good.
How about
 - Remove 'correct' and 'incorrect'
   We show 'PASS' or 'FAIL'.  It's enough.
 - Show sanitized value only if the expected sanitized value is not identical
to the input value or a sanitization test fails.

> LayoutTests/fast/forms/resources/ValidityState-typeMismatch-email.js:21
>  // VALID

It's good to print such headings.  You can
  debug('Valid single addresses:');
or something.

> LayoutTests/fast/forms/resources/ValidityState-typeMismatch-email.js:22
> +emailCheck("something at something.com", "something at something.com", false,
false);

Boolean parameters are not good for readability.
I'd like to do:

var expectValid = false;
var expectInvalid = true;
var multiple = true;

emailCheck("something at something.com", "something at something.com", expectValid);
// We can omit the trailing false parameter.
emailCheck(" a at p.com,b at p.com", "a at p.com,b at p.com", expectInvalid, multiple);

> Source/JavaScriptCore/wtf/ASCIICType.h:147
> +    // White spaces in the HTML specification.
> +    inline bool isASCIISpaceExceptVT(char c) { return c == ' ' || (c <= 0xD
&& c >= 0x9 && c != 0xB); }
> +    inline bool isASCIISpaceExceptVT(unsigned short c) { return c == ' ' ||
(c <= 0xD && c >= 0x9 && c != 0xB); }
> +#if !COMPILER(MSVC) || defined(_NATIVE_WCHAR_T_DEFINED)
> +    inline bool isASCIISpaceExceptVT(wchar_t c) { return c == ' ' || (c <=
0xD && c >= 0x9 && c != 0xB); }
> +#endif
> +    inline bool isASCIISpaceExceptVT(int c) { return c == ' ' || (c <= 0xD
&& c >= 0x9 && c != 0xB); }
> +    inline bool isASCIISpaceExceptVT(unsigned c) { return c == ' ' || (c <=
0xD && c >= 0x9 && c != 0xB); }
> +
> +    inline bool isASCIILineBreak(char c) { return c == 0xA || c == 0xD; }
> +    inline bool isASCIILineBreak(unsigned short c) { return c == 0xA || c ==
0xD; }
> +#if !COMPILER(MSVC) || defined(_NATIVE_WCHAR_T_DEFINED)
> +    inline bool isASCIILineBreak(wchar_t c) { return c == 0xA || c == 0xD; }

> +#endif
> +    inline bool isASCIILineBreak(int c) { return c == 0xA || c == 0xD; }
> +    inline bool isASCIILineBreak(unsigned c) { return c == 0xA || c == 0xD;
}
> +

These functions are HTML-specific, and not useful for JavaScriptCode.  So I
think they should be in WebCore/html/parser/HTMLParserIdioms.{cpp,h}.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:351
> +    // skip white space from start

Usually comments should be sentences.  "// Skip white space from start."
Anyway, this comment seems not helpful.  So remove this comment please.

> Source/JavaScriptCore/wtf/text/StringImpl.h:294
> +    PassRefPtr<StringImpl> stripWhiteSpaceExceptVT();
> +    PassRefPtr<StringImpl> stripLineBreaks();

ditto. Move them to HTMLParserIdioms.

> Source/WebCore/html/EmailInputType.cpp:97
> +String EmailInputType::convertFromVisibleValue(const String& visibleValue)
const
> +{
> +    return visibleValue;
> +}
> +

It's ok to remove this function.

> Source/WebCore/html/EmailInputType.cpp:111
> +    String strippedValue;
> +    for (unsigned i = 0; i < addresses.size(); ++i) {
> +	   if (i > 0)
> +	       strippedValue += String(",");
> +	   strippedValue += addresses[i].stripWhiteSpaceExceptVT();
> +    }
> +    return strippedValue;

You had better use wtf/textStirngBuilder.