Bug 156906 - Minor refactoring in RenderMathMLOperator
Summary: Minor refactoring in RenderMathMLOperator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on:
Blocks: 152244 156913 156921 156950
  Show dependency treegraph
 
Reported: 2016-04-22 04:59 PDT by Frédéric Wang (:fredw)
Modified: 2016-04-25 00:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.82 KB, patch)
2016-04-22 05:06 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (7.43 KB, patch)
2016-04-22 06:48 PDT, Frédéric Wang (:fredw)
mrobinson: review+
mrobinson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>