[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


yichao.yin at torchmobile.com.cn changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yichao.yin at torchmobile.com.c
                   |                            |n




------- Comment #5 from yichao.yin at torchmobile.com.cn  2009-01-23 18:54 PDT -------
(In reply to comment #2)

> 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" :-)

Just for saving several bytes :). Okay, I will name it more readable

> > + 
> > +    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];
> > +                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; }

No need, "if (i + 1 != MaskLen)" is used to check this since we search the "*"
from left to right. If there are more 
than one "*f" or "nf", the if statement will be true and break the for loop
with isValid = false.


> > +                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();

Yes, that's better, will fix it.

> > +        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

You are right, I should add more test cases for it.

Thanks a lot for your good comments. I will update the patch following them
ASAP.


-- 
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.