[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





------- Comment #6 from yichao.yin at torchmobile.com.cn  2009-01-23 19:34 PDT -------
(In reply to comment #3)
> (From update of attachment 26967 [review])
> Review, part 2:
> > @@ -215,6 +235,9 @@ void WMLInputElement::attach()
> > +    // Here we initialize the WML <input> element
> > +    init();
> I'm not sure if it's desired to call init() from attach().
> How about calling it from 'finishedParsingChildren()' ?
> The drawback using attach() would be that I could force the renderer to be
> attached, detach and re-attached.
> This way we'd store the variable state twice, that's not what we want I fear.

You are right, finishParsingChildren() is a better place to do this. 


> > +        else if (renderer() && !isConformedToInputMask(textEvent->data()[0], static_cast<RenderTextControl*>(renderer())->text().length() + 1))
> > +            // If the inputed char doesn't conform to the input mask, stop handling 
> > +            return;
> > +    }
> Why length() + 1 here?

the second parameter of isConformedToInputMask(UChar inChar, unsigned
inputCharCount, bool isInputedByUsr) is used to specify the character number
user has inputed, including current one, which has not been stored in
RenderTextControl::text().


> > + 
> > +    if (getAttribute(WMLNames::emptyokAttr).isEmpty()) {
> You can use if (!hasAttribute(WMLNames::emptyokAttr)) here.

Good, better solution. will fix.


> > +        if (m_formatMask.isEmpty() || 
> > +            // check if the format codes is a "*f" code
> > +           (m_formatMask.length() == 2 && m_formatMask[0] == '*' && s_formatCodes.find(m_formatMask[1]) != -1))
> > +            m_isEmptyOk = true;
> > +    }
> This only takes care of the 'f' format code, what about the others?

That piece of code is to check the input mask with value of "*f" only.  so we
need to check the length and whole string.

> > +bool WMLInputElement::isConformedToInputMask(const String& inputChars)
> > +{
> > +    bool ok = true;
> > +    for (unsigned i = 0; i < inputChars.length(); ++i) {
> > +        ok = isConformedToInputMask(inputChars[i], i + 1, false);
> > +        if (!ok)
> > +            break;
> > +    }
> You can rewrite as follows, to save a line:
> if (ok = isConformedToInputMask(inputChars[i], i + 1, false))
>     continue;

Exactly, Thanks. :)

> > +bool WMLInputElement::isConformedToInputMask(UChar inChar, unsigned inputCharCounts, bool isInputedByUsr)
> s/isInputedByUsr/isUserInput/
> s/inputCharCounts/inputCharCount/
> > +{
> > +    bool ok = true;
> > +    if (m_formatMask.isEmpty())
> > +        return ok;
> Just 'return true' here, and save the 'ok' variable declaration. Move it below
> 'UChar mask = ..' where it's atually needed.

Good, will fix.

> > +    unsigned idxOfMask = 0;
> s/idxOfMask/maskIndex/
> > +    if (isInputedByUsr) {
> > +        int cursorPos = 0;
> > +        if (renderer())
> > +            cursorPos = static_cast<RenderTextControl*>(renderer())->selectionStart(); 
> > +        else
> > +            cursorPos = m_data.cachedSelectionStart();
> > +
> > +        idxOfMask = cursorPosToIdxOfMask(cursorPos);
> > +    } else
> > +        idxOfMask = cursorPosToIdxOfMask(inputCharCounts - 1);
> s/cursorPosToIdxOfMask/cursorPositionToMaskIndex/
> > +
> > +    UChar mask = m_formatMask[idxOfMask];
> You should be able to use switch (mask) here - should read more cleanly:

Sounds better. Agreed.


> > +    // match the inputed character with input mask
> > +    if (mask == 'A')
> > +        ok = !WTF::isASCIIDigit(inChar) && !WTF::isASCIILower(inChar) && WTF::isASCIIPrintable(inChar);
> > +    else if (mask == 'a')
> > +        ok = !WTF::isASCIIDigit(inChar) && !WTF::isASCIIUpper(inChar) && WTF::isASCIIPrintable(inChar);
> > +    else if (mask == 'N')
> > +        ok = WTF::isASCIIDigit(inChar);
> > +    else if (mask == 'n')
> > +        ok = !WTF::isASCIIAlpha(inChar) && WTF::isASCIIPrintable(inChar);
> > +    else if (mask == 'X')
> > +        ok = !WTF::isASCIILower(inChar) && WTF::isASCIIPrintable(inChar);
> > +    else if (mask == 'x')
> > +        ok = !WTF::isASCIIUpper(inChar) && WTF::isASCIIPrintable(inChar);
> > +    else if (mask == 'M')
> > +        ok = WTF::isASCIIPrintable(inChar);
> > +    else if (mask == 'm')
> > +        ok = WTF::isASCIIPrintable(inChar);
> > +    else
> > +        ok = (mask == inChar);
> > +
> > +    return ok;
> > +}
> > +
> > +unsigned WMLInputElement::cursorPosToIdxOfMask(unsigned cursorPos)
> > +{
> > +    UChar mask;
> > +    int idx = -1;
> > +    do {
> > +        mask = m_formatMask[++idx];
> > +        if (mask == '\\')
> > +            idx++;
> > +        else if (mask == '*' || (WTF::isASCIIDigit(mask) && mask != '0')) {
> > +            idx = m_formatMask.length() - 1;
> > +            break;
> > +        }
> > +    } while (cursorPos--);
> > + 
> > +    return idx;
> > +}
> s/idx++/++idx/
> s/idx/index/

Will fix.

> >  }
> >  
> >  #endif
> > Index: WebCore/wml/WMLInputElement.h
> > ===================================================================
> > --- WebCore/wml/WMLInputElement.h	(revision 40084)
> > +++ WebCore/wml/WMLInputElement.h	(working copy)
> > @@ -89,10 +89,21 @@ public:
> >      virtual void willMoveToNewOwnerDocument();
> >      virtual void didMoveToNewOwnerDocument();
> >  
> > +    bool isConformedToInputMask(const String&);
> > +    bool isConformedToInputMask(UChar, unsigned, bool isInputedByUsr = true);
> > +
> > +    static const String s_formatCodes;
> I'm not sure wheter it's wise to declare a static const String.
> What you could do is add a private function: "static const AtomicString&
> formatCodes();"
> const AtomicString& WMLInputElement::formatCodes()
> {
>     DEFINE_STATIC_LOCAL(AtomicString, codes, ("AaNnXxMm"));
>     return codes;
> }


Good idea! It should be more like Webkit-style code.

Thanks a lot to let me learn more.


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