[Webkit-unassigned] [Bug 118053] Parsing of MathML length

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=118053


chris fleizach <cfleizach at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #205475|review?                     |review-
               Flag|                            |




--- Comment #2 from chris fleizach <cfleizach at apple.com>  2013-06-26 12:48:01 PST ---
(From update of attachment 205475)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.