[webkit-reviews] review denied: [Bug 23499] Add WML specific logic for WML <input> : [Attachment 26967] initial patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied yichao.yin
<yichao.yin at torchmobile.com.cn>'s request for review:
Bug 23499: Add WML specific logic for WML <input>
https://bugs.webkit.org/show_bug.cgi?id=23499

Attachment 26967: initial patch
https://bugs.webkit.org/attachment.cgi?id=26967&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hey Yichao, nice work - especially the new testcase :-)
Though I think it will need to handle a lot more 'corner cases' especially in
the input mask validation. I'll show my issues using the code below:

I don't have time for a full review, only talking about validateInputMask()
now:

> +String WMLInputElement::validateInputMask(const String& inputMask)
> +{
> +    bool isValid = true;
> +    bool hasWildcard = false;
> +    unsigned escapeCharCnts = 0;
> +    unsigned maskLen = inputMask.length();
> +    UChar formatCode;
Naming, why not use "maskLength" and "escapeCharCount" :-)

> + 
> +    for (unsigned i = 0; i < maskLen; ++i) {
> +	   formatCode = inputMask[i];
> +	   if (s_formatCodes.find(formatCode) == -1) {
> +	       if (formatCode == '*' || (WTF::isASCIIDigit(formatCode) &&
formatCode != '0')) {
> +		   // validate codes which ends with '*f' or 'nf'
> +		   formatCode = inputMask[++i];
What happens if we're at i == maskLen - 1, then inputMask[maskLen] would crash,
no?

> +		   if ((i + 1 != maskLen) || s_formatCodes.find(formatCode) ==
-1) {
> +		       isValid = false;
> +		       break;
> +		   }

Okay this takes care of only allowing the "*f' or "nf" to appear at the end of
the string, fine.
The WML spec says these format codes are only allowed to appear _once_ though.
So maybe you could add the following, before the last 'if' above:
if (hasWildcard) { isValid = false; break; }

> +		   hasWildcard = true;
> +	       } else if (formatCode == '\\') {
> +		   //skip over the next mask character
> +		   i++;
> +		   escapeCharCnts++;
> +	       } else {
> +		   isValid = false;
> +		   break;
> +	       }
> +	   }
> +    }
> + 
> +    // calculate the number of characters allowed to be entered by input
mask
> +    if (isValid) {

Use early exit here: if (!isValid) return String();

> +	   m_numOfCharsAllowedByMask = maskLen;
> + 
> +	   if (escapeCharCnts)
> +	       m_numOfCharsAllowedByMask -= escapeCharCnts;
> + 
> +	   if (hasWildcard) {
> +	       formatCode = inputMask[maskLen - 2];
> +	       if (formatCode == '*')
> +		   m_numOfCharsAllowedByMask = m_data.maxLength();
> +	       else {
> +		   unsigned leftLen = String(&formatCode).toInt();
> +		   m_numOfCharsAllowedByMask = leftLen +
m_numOfCharsAllowedByMask - 2;
> +	       }
> +	   }
> +
> +	   return inputMask;
> +    }
> + 
> +    return String();
> +}

Okay, this method is mostly correct, but all of the corner cases need to be
tested in your wml/input-formal.html test:
- 'nf' / '*f' statements in the beginning/middle of the input mask (should
fail)
- 'nf' statements, with n>9, should fail
- invalid mask characters 'YyKkQq', should be tested in all variations
(combined with valid mask chars, as well)
- 'escape characters' should be tested

I hope that's it for now :-)
I'll continue the review tonight.