Bug 156906 – Minor refactoring in RenderMathMLOperator

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Frédéric Wang (:fredw)]

I'm extracting more changes from bug 152244 that were suggested by Manuel Rego

[Frédéric Wang (:fredw)]

Created attachment 277045 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 277045 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277045&action=review

LGTM, just a minor suggestion.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:441
> -            if (state == 1) {
> +            if (expected == Start) {
>                  // We ignore left/bottom piece and multiple successive extenders.
> -                state = 2;
> -            } else if (state == 3) {
> +                expected = ExtenderBetweenStartAndMiddle;
> +            } else if (expected == Middle) {
>                  // We ignore middle piece and multiple successive extenders.
> -                state = 4;
> -            } else if (state >= 5)
> +                expected = ExtenderBetweenMiddleAndEnd;
> +            } else if (expected >= End)

Maybe you can rewrite this with a switch.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:464
> -        if (state == 1) {
> +        if (expected == Start) {
>              // We copy the left/bottom part.
>              bottom.glyph = part.glyph;
> -            state = 2;
> +            expected = ExtenderBetweenStartAndMiddle;
>              continue;
>          }
>  
> -        if (state == 2 || state == 3) {
> +        if (expected == ExtenderBetweenStartAndMiddle || expected == Middle) {
>              // We copy the middle part.
>              middle.glyph = part.glyph;
> -            state = 4;
> +            expected = ExtenderBetweenMiddleAndEnd;
>              continue;
>          }
>  
> -        if (state == 4 || state == 5) {
> +        if (expected == ExtenderBetweenMiddleAndEnd || expected == End) {
>              // We copy the right/top part.
>              top.glyph = part.glyph;
> -            state = 6;
> +            expected = None;
>          }

Ditto.

[Frédéric Wang (:fredw)]

Created attachment 277059 [details]
Patch

[Martin Robinson]

Comment on attachment 277059 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277059&action=review

This is a *really good* change that makes the code a lot more readable. I have a couple small nits, so maybe you can take care of them before landing this.

> Source/WebCore/ChangeLog:8
> +        No new tests, this is only minor refactoring that does not change the code.

It might be more accurate to say "behavior" here instead of "code." The code is, in fact, changing. :)

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:418
> +    PartType expected = Start;

I have a preference for expectedPartType just for the sake of clarity.

[Frédéric Wang (:fredw)]

Committed r199978: <http://trac.webkit.org/changeset/199978>