[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 #212337|review?                     |review+
               Flag|                            |




--- Comment #20 from Darin Adler <darin at apple.com>  2013-09-23 10:06:10 PST ---
(From update of attachment 212337)
View in context: https://bugs.webkit.org/attachment.cgi?id=212337&action=review

> Source/WebCore/mathml/MathMLSelectElement.cpp:40
> +    : MathMLInlineContainerElement(tagName, document), m_selectedChild(0)

Should be nullptr instead of 0. We need to update our coding style guideline document to say this.

> Source/WebCore/mathml/MathMLSelectElement.cpp:56
> +    return (child->isElementNode() && toElement(child) == m_selectedChild);

No need for the typecast. The compiler understands the relationship between the types. This should just read:

    return child == m_selectedChild;

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

Do these need to be case sensitive? Does the test case check that they should be case sensitive? I guess I asked this before.

> Source/WebCore/mathml/MathMLSelectElement.cpp:89
> +            if (newSelectedChild) {

Why not put this "if (newSelectedChild)" check around even the call to hasLocalName and save a lot of work? I see no reason to do all that stuff if there are no children.

> Source/WebCore/mathml/MathMLSelectElement.cpp:107
> +    if (m_selectedChild && m_selectedChild->renderer())
> +        Style::detachRenderTree(*m_selectedChild);

This seems surprisingly manual and low level. Explicitly deleting the renderer like this is surprising. I’d normally expect this to be done during the later style recalculation process rather than explicitly and immediately.

> Source/WebCore/mathml/MathMLSelectElement.h:34
> +class MathMLSelectElement : public MathMLInlineContainerElement {

Should mark this class FINAL.

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