[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