[webkit-reviews] review denied: [Bug 118053] Parsing of MathML length : [Attachment 205475] Patch V1

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied Frédéric Wang
<fred.wang at free.fr>'s request for review:
Bug 118053: Parsing of MathML length
https://bugs.webkit.org/show_bug.cgi?id=118053

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

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


thanks!

> LayoutTests/ChangeLog:12
> +	   * mathml/presentation/mfrac-linethickness1-expected-mismatch.html:
Added.

none of the tests have expected results

> LayoutTests/mathml/presentation/mfrac-linethickness1-expected-mismatch.html:8

> +  <div style="position: absolute; top: 0; left: 0; width: 500px; height:
150px; background: blue;"></div>

what is this testing? there is nothing about math in this test

> LayoutTests/mathml/presentation/mfrac-linethickness1.html:11
> +	 <mfrac linethickness="200px">

these thickness tests should be in one line thickness test. i don't think we
need separate files for each one

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:243
> +    // see if the negative sign is there

full sentence required for comment

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:256
> +	       return false; // two dots encountered

comments should go on the same line. you should put this comment above the if
line and make it a full sentence

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:257
> +	   if (c == '.') {

one line if statements shouldn't use brackets

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:261
> +	       // some authors leave blanks before the unit, but that shouldn't


comments should start with capitalized letters like full sentences

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:322
> +bool RenderMathMLBlock::ParseNamedSpace(const String& string, float&
lengthValue, bool allowNegative)

length should probably be the return value, and there should be a parameter to
confirm if it succeeded, like toIntStrict()

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:324
> +    float length = 0.;

don't append . for floating point literals
see "Floating point literals"
http://www.webkit.org/coding/coding-style.html

> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:73
> +    bool ParseNamedSpace(const String&, float&, bool allowNegative = true);

i think both of these can be functions that are outside the class block. They
don't really belong as instance methods of the class
also the names should start with lowercase

it looks like ParseNamedSpace is only used inside ParseLength… that method
should be static inside the class file

> Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:82
> +	   ParseLength(thickness, m_lineThickness, false);

you might want to check if the conversion succeeded before setting the value