[Webkit-unassigned] [Bug 23499] Add WML specific logic for WML <input>

[bugzilla-daemon at webkit.org]

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


zimmermann at kde.org changed:

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




------- Comment #2 from zimmermann at kde.org  2009-01-23 08:33 PDT -------
(From update of attachment 26967)
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.


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