[webkit-reviews] review granted: [Bug 42472] Implement MathML mfenced element : [Attachment 63595] Revised patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Aug 5 11:28:23 PDT 2010
Kenneth Rohde Christiansen <kenneth at webkit.org> has granted François Sausset
<sausset at gmail.com>'s request for review:
Bug 42472: Implement MathML mfenced element
https://bugs.webkit.org/show_bug.cgi?id=42472
Attachment 63595: Revised patch
https://bugs.webkit.org/attachment.cgi?id=63595&action=review
------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
WebCore/mathml/RenderMathMLFenced.cpp:42
+ enum Braces {OpeningBraceChar = 0x28, ClosingBraceChar = 0x29};
I would add a space before Opening or put in on a new line.
WebCore/mathml/RenderMathMLFenced.cpp:46
+ , m_open(OpeningBraceChar)
What does 'open' mean? We normally name things like "isOpen" instead.
WebCore/mathml/RenderMathMLFenced.cpp:55
+ // FIXME: Handle open/close values with more than one character (they
should be treated like text)
Misses dot at the end :-)
WebCore/mathml/RenderMathMLFenced.cpp:72
+ // The separator defaults to a single comma
Here as well
WebCore/mathml/RenderMathMLFenced.cpp:92
+ RenderObject* openFence = new (renderArena())
RenderMathMLOperator(node(), m_open);
Why did you need the m_open? Can't you use the enum directtly instead?
WebCore/mathml/RenderMathMLFenced.cpp:100
+ void RenderMathMLFenced::addChild(RenderObject* child, RenderObject* )
Remove space after *
WebCore/mathml/RenderMathMLFenced.cpp:107
+ unsigned int count = 0;
count of?
WebCore/mathml/RenderMathMLFenced.cpp:113
+ if (count>1) {
Needs spaces around >
WebCore/mathml/RenderMathMLFenced.cpp:130
+
Remove this newline
WebCore/mathml/RenderMathMLFenced.cpp:141
+
And this one
WebCore/mathml/RenderMathMLFenced.cpp:163
+
And this one
WebCore/mathml/RenderMathMLOperator.cpp:115
+ // FIXME: use fractions of style()->fontSize() for proper zooming/resizing
Add dot at end
More information about the webkit-reviews
mailing list