[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