[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 #3 from zimmermann at kde.org  2009-01-23 09:02 PDT -------
(From update of attachment 26967)
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.

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

> +void WMLInputElement::init()
> +{
> +    String nameVar = name();
> +    String varValue;
> +    WMLPageState* pageSate = wmlPageStateForDocument(document()); 
> +    ASSERT(pageSate);
> +    if (!nameVar.isEmpty())
> +        varValue = pageSate->getVariable(nameVar);
> +
> +    if (varValue.isEmpty() || !isConformedToInputMask(varValue)) {
> +        String val = value();
> +        if (isConformedToInputMask(val))
> +            varValue = val;
> +        else
> +            varValue = "";
> + 
> +        pageSate->storeVariable(nameVar, varValue);
> +    }
> +    setValue(varValue);
> + 
> +    if (getAttribute(WMLNames::emptyokAttr).isEmpty()) {
You can use if (!hasAttribute(WMLNames::emptyokAttr)) here.

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

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

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

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

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

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

Okay, really need to leave now :-)
Have fun!


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