[webkit-reviews] review denied: [Bug 85729] Add support for menclose element : [Attachment 212899] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Sep 28 12:17:04 PDT 2013


Sam Weinig <sam at webkit.org> has denied gur.trio at gmail.com's request for review:
Bug 85729: Add support for menclose element
https://bugs.webkit.org/show_bug.cgi?id=85729

Attachment 212899: Patch
https://bugs.webkit.org/attachment.cgi?id=212899&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=212899&action=review


> Source/WebCore/mathml/MathMLInlineContainerElement.cpp:87
> +    if (hasLocalName(mencloseTag))
> +	   return new (arena) RenderMathMLMenclose(this);

The normal model is override createRenderer in the derived class.  You should
probably do that here.

> Source/WebCore/mathml/MathMLMencloseElement.cpp:47
> +// for notation = roundedbox we set the border radius (px)
> +static const String borderRadius()
> +{
> +    DEFINE_STATIC_LOCAL(const String, radius, (ASCIILiteral("5px")));
> +    return radius;
> +}

I don't think this is necessary.  Just use ASCIILiteral("5px")) where you call
borderRadius().

> Source/WebCore/mathml/MathMLMencloseElement.cpp:144
> +    String padding;
> +    int fontSize = 0;
> +    String closingBrace = ")";
> +    TextRun run(closingBrace.impl(), closingBrace.length());
> +    Node* node = parentNode();
> +    if (node && node->renderer() && node->renderer()->style()) {
> +	   const Font& font = node->renderer()->style()->font();
> +	   fontSize = font.width(run);
> +	   padding.append(String::number(fontSize));
> +	   padding.append("px");
> +    }
> +    return padding;

This should use StringBuilder.

> Source/WebCore/mathml/MathMLMencloseElement.h:35
> +class MathMLMencloseElement : public MathMLInlineContainerElement {

If no one ever inherits from this class, you should mark it FINAL.

> Source/WebCore/mathml/MathMLMencloseElement.h:40
> +    virtual bool isPresentationAttribute(const QualifiedName&) const
OVERRIDE;
> +    virtual void collectStyleForPresentationAttribute(const QualifiedName&,
const AtomicString&, MutableStylePropertySet*) OVERRIDE;
> +    virtual void finishParsingChildren() OVERRIDE;

These can probably be private.

> Source/WebCore/mathml/MathMLMencloseElement.h:42
> +    Vector<String> getNotationValue() {return mNotationValues;}
> +    bool isRadical() {return mIsRadicalValue;}

You need spaces after the  { and before the }.

> Source/WebCore/mathml/MathMLMencloseElement.h:45
> +    MathMLMencloseElement(const QualifiedName& , Document&);

Extraneous space after const QualifiedName&.

> Source/WebCore/mathml/MathMLMencloseElement.h:49
> +    Vector<String> mNotationValues;
> +    bool mIsRadicalValue;

The style in WebCore is m_name, not mName.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:49
> +	   MathMLMencloseElement* menclose =
static_cast<MathMLMencloseElement*>(element());

You should add and use a toMathMLMencloseElement() which asserts the type.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:66
> +    if (element()) {

When will there be no Element? If it is possible, you should probably check it
for null in RenderMathMLMenclose::addChild as well.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:67
> +	   MathMLMencloseElement* menclose =
static_cast<MathMLMencloseElement*>(element());

You should add and use a toMathMLMencloseElement() which asserts the type.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:95
> +		   if (equalIgnoringCase(notationVal[i], "updiagonalstrike"))
> +		       info.context->drawLine(IntPoint(left, (top+boxHeight)),
IntPoint((left+boxWidth), top));
> +		   else if (equalIgnoringCase(notationVal[i],
"downdiagonalstrike"))
> +		       info.context->drawLine(IntPoint(left, top),
IntPoint((left+boxWidth), (top+boxHeight)));
> +		   else if (equalIgnoringCase(notationVal[i],
"verticalstrike"))
> +		       info.context->drawLine(IntPoint((left+halfboxWidth),
top), IntPoint((left+halfboxWidth), (top+boxHeight)));
> +		   else if (equalIgnoringCase(notationVal[i],
"horizontalstrike"))
> +		       info.context->drawLine(IntPoint(left,
(top+halfboxHeight)), IntPoint((left+boxWidth), (top+halfboxHeight)));

You need spaces around your +s and you don't really need all the extra
parenthesis.

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:35
> +// Render sqrt(base), using radical notation.

Is this comment right?

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:42
> +    virtual void paint(PaintInfo&, const LayoutPoint&) OVERRIDE;

Does this need to be protected? Can it be private?

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46
> +    bool paintdependentAttribute(Vector<String> attr, int attrSize) const;

This should probably take a const Vector<String>& to avoid a copy.  The name is
also odd and should probably use camelcase.


More information about the webkit-reviews mailing list