[Webkit-unassigned] [Bug 55987] input type=email and multiple is not compliant

[bugzilla-daemon at webkit.org]

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


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #98685|review?                     |review-
               Flag|                            |




--- Comment #9 from Kent Tamura <tkent at chromium.org>  2011-06-27 02:50:37 PST ---
(From update of attachment 98685)
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.

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