[webkit-reviews] review granted: [Bug 120058] Add an MathMLSelectElement class to implement <maction> and <semantics> : [Attachment 212337] Patch V7 - bis

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 23 10:07:07 PDT 2013

Darin Adler <darin at apple.com> has granted Frédéric Wang <fred.wang at free.fr>'s
request for review:
Bug 120058: Add an MathMLSelectElement class to implement <maction> and

Attachment 212337: Patch V7 - bis

------- Additional Comments from Darin Adler <darin at apple.com>
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.

More information about the webkit-reviews mailing list