Bug 156906

Summary: Minor refactoring in RenderMathMLOperator
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, commit-queue, dbarton, esprehn+autocc, glenn, kondapallykalyan, mrobinson, rego
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 152244, 156913, 156921, 156950    
Attachments:
Description Flags
Patch
none
Patch mrobinson: review+, mrobinson: commit-queue-

Description Frédéric Wang (:fredw) 2016-04-22 04:59:28 PDT
I'm extracting more changes from bug 152244 that were suggested by Manuel Rego
Comment 1 Frédéric Wang (:fredw) 2016-04-22 05:06:25 PDT
Created attachment 277045 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2016-04-22 06:10:13 PDT
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.
Comment 3 Frédéric Wang (:fredw) 2016-04-22 06:48:25 PDT
Created attachment 277059 [details]
Patch
Comment 4 Martin Robinson 2016-04-23 12:33:06 PDT
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.
Comment 5 Frédéric Wang (:fredw) 2016-04-25 00:04:28 PDT
Committed r199978: <http://trac.webkit.org/changeset/199978>