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

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> 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 220508: Patch
https://bugs.webkit.org/attachment.cgi?id=220508&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220508&action=review


> Source/WebCore/dom/Element.h:473
> +#endif

we probably don't want to put this in Element. The other MathML elements are
referenced like this in MathMLElement.h

> Source/WebCore/mathml/MathMLMencloseElement.cpp:3
> + * Copyright (C) 2013 The MathJax Consortium.

if this is a new file why does it have a 2010 copyright?

> Source/WebCore/mathml/MathMLMencloseElement.cpp:70
> +    if (m_IsRadicalValue && (!firstElementChild()))

the parens around firstElementChild() seem unnecessary
can you add a comment why you need to add a child when you finish parsing

what do the 0's stand for in addChild()? if they're pointers they should be
nullptr

> Source/WebCore/mathml/MathMLMencloseElement.cpp:80
> +	       int attrvaluesSize = m_NotationValues.size();

size() returns a size_t

> Source/WebCore/mathml/MathMLMencloseElement.cpp:131
> +	   fontSize = font.width(run);

does font.width() return an int? my guess would be that it'd return an unsigned


> Source/WebCore/mathml/MathMLMencloseElement.h:2
> + * Copyright (C) 2010 Alex Milowski (alex at milowski.com). All rights
reserved.

ditto about copyright

> Source/WebCore/mathml/MathMLMencloseElement.h:39
> +    Vector<String>& getNotationValue() { return m_NotationValues; }

i think you can drop the "get" prefix and name it notationValues() const

> Source/WebCore/mathml/MathMLMencloseElement.h:40
> +    bool isRadical() { return m_IsRadicalValue; }

should be a const

> Source/WebCore/mathml/MathMLMencloseElement.h:52
> +    Vector<String> m_NotationValues;

this should be m_notationValues

> Source/WebCore/mathml/MathMLMencloseElement.h:53
> +    bool m_IsRadicalValue;

should be m_isRadicalValue

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:50
> +    // For radical notation creating an anonymous RenderMathMLSquareRoot.

this comment doesn't explain why you're doing this

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:56
> +	       toRenderElement(firstChild())->addChild(newChild, beforeChild &&
beforeChild->parent() == firstChild() ? beforeChild : 0);

0 should be nullptr

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:90
> +    Vector<String>& notationVal = menclose->getNotationValue();

var name should be notationValues

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:91
> +    int notationvalSize = notationVal.size();

size_t instead of int

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:93
> +	   isDefaultLongDiv = true;

seems like this should be written as

bool isDefaultLongDiv = !notatationalValueSize;

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:137
> +bool RenderMathMLMenclose::isValidNotationValue(Vector<String>& attr, int
attrSize) const

passing in attrSize seems unnecessary since you're already passing in attr,
which can easily find the size of itself

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:139
> +    for (int i = 0;i < attrSize; i++) {

bad spacing

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:140
> +	   if (attr[i] == "updiagonalstrike" || attr[i] == "downdiagonalstrike"
|| attr[i] == "horizontalstrike" || attr[i] == "verticalstrike"

since you only seem to do work in the above method when the attr name matches
one of these, it seems like this check is superfluous. Anything not matching
one of these would cause anything to happen

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:43
> +    RenderMathMLSquareRoot* newSRoot = new
RenderMathMLSquareRoot(toElement(*node),
RenderStyle::createAnonymousStyleWithDisplay(&parent->style(), FLEX));

variable name should be something like squareRoot, or root

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:39
> +    static RenderMathMLSquareRoot* createAnonymousWithParentRenderer(const
RenderObject*);

I have a feeling this should return a RenderPtr

> LayoutTests/mathml/presentation/menclose-notation-attribute-set1.html:42
> +				isSuccessfullyParsed();

bad indentation