[Webkit-unassigned] [Bug 120058] Add an MathMLSelectElement class to implement <maction> and <semantics>

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #11 from Darin Adler <darin at apple.com>  2013-09-16 15:12:02 PST ---
(From update of attachment 211770)
View in context: https://bugs.webkit.org/attachment.cgi?id=211770&action=review

Patch looks generally good, but there are multiple mistakes that should be fixed.

> Source/WebCore/mathml/MathMLSelectElement.cpp:85
> +        String actiontype = getAttribute(MathMLNames::actiontypeAttr);

Type should be const AtomicString& rather than String. Should call fastGetAttribute instead of getAttribute.

> Source/WebCore/mathml/MathMLSelectElement.cpp:86
> +        if (actiontype != "tooltip" && actiontype != "statusline") {

Is this intentionally case sensitive? I’d like to see test coverage for this; many attribute values are non-case-sensitive.

> Source/WebCore/mathml/MathMLSelectElement.cpp:98
> +            bool ok = true;
> +            int selection = (hasAttribute(MathMLNames::selectionAttr) ? getAttribute(MathMLNames::selectionAttr).toInt(&ok) : 1);
> +            if (ok && selection >= 1) {
> +                Element* child = newSelectedChild;
> +                for (int i = 1; child; child = child->nextElementSibling()) {
> +                    newSelectedChild = child;
> +                    if (i == selection)
> +                        break;
> +                    i++;
> +                }
> +            }

There need to be more test cases. I don’t see any test cases that cover a selection attribute that has, say, leading whitespace or any of the many other cases. It’s possible this function should be skipping whitespace or using toStrictInt rather than toInt but it’s test cases that would make that clear. I also don’t see any test cases that demonstrate that a value for selection larger than the number of children means that the last child is selected, even though that's what the code does.

This logic is too complicated. There are many cases that are handled identically but have different code paths, for example, lack of an attribute, attribute that is empty string, attribute that is not parseable as an int are all treated the same in unnecessarily different ways. And the loop is needlessly strange the way it mixes the integer part with the pointer part. Here’s how I would write it:

    int selection = fastGetAttribute(MathMLNames::selectionAttr).toInt();
    for (int i = 1; i < selection; ++i) {
        Element* nextChild = newSelectedChild->nextElementSibling();
        if (!nextChild)
            break;
        newSelectedChild = nextChild;
    }

> Source/WebCore/mathml/MathMLSelectElement.cpp:105
> +    if (m_SelectedChild != newSelectedChild) {

I think that early return would be a better way to write this. Return here if they are equal so the rest of the code is not nested.

> Source/WebCore/mathml/MathMLSelectElement.h:39
> +protected:
> +    MathMLSelectElement(const QualifiedName& tagName, Document&);

Why protected instead of private?

> Source/WebCore/mathml/MathMLSelectElement.h:42
> +    virtual RenderObject* createRenderer(RenderArena*, RenderStyle*);

Needs to be marked OVERRIDE.

> Source/WebCore/mathml/MathMLSelectElement.h:44
> +    bool childShouldCreateRenderer(const Node*) const;

Needs to be marked virtual and OVERRIDE.

> Source/WebCore/mathml/MathMLSelectElement.h:48
> +    void finishParsingChildren() OVERRIDE;
> +    void childrenChanged(const ChildChange&) OVERRIDE;
> +    void attributeChanged(const QualifiedName&, const AtomicString&, AttributeModificationReason = ModifiedDirectly) OVERRIDE;

Should be marked virtual as well as OVERRIDE.

> Source/WebCore/mathml/MathMLSelectElement.h:51
> +    Element* m_SelectedChild;

The "S" should not be capitalized. It should be m_selectedChild.

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