[webkit-reviews] review denied: [Bug 115610] Add Support for mspace element : [Attachment 203758] Patch V5 - quater

Wed Jun 19 14:02:15 PDT 2013

chris fleizach <cfleizach at apple.com> has denied Frédéric Wang
<fred.wang at free.fr>'s request for review:
Bug 115610: Add Support for mspace element
https://bugs.webkit.org/show_bug.cgi?id=115610

Attachment 203758: Patch V5 - quater
https://bugs.webkit.org/attachment.cgi?id=203758&action=review

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

You should separate the value parsing from the MathMLSpace patch into two patch
I think, so you can address the line thickness issue separately.

Thanks

> LayoutTests/mathml/presentation/mfrac-linethickness-expected-mismatch.html:4
> +    <title>mfrac linethickness</title>

what is this testing? there's nothing related to mfrac in here

> LayoutTests/mathml/presentation/mspace-expected.html:1
> +<!DOCTYPE html>

Your layout tests probably shouldn't be suffixed with -expected, because that's
the suffix for the expected results.

I also don't see any results from running these layout tests...

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:-1
> -ï»¿<?xml version="1.0" encoding="utf-8"?>

inccorrect addition here?

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:317
> +    return false;

It seems a shame this is specific to MathML and not re-usable. I see code in
CSSPrimitiveValue that handles some of these cases it appears

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:322
> +    int i = 0;

you should choose a better name for this variable, plus it looks like it should
be a float anyway