I'm extracting more changes from bug 152244 that were suggested by Manuel Rego
Created attachment 277045 [details] Patch
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.
Created attachment 277059 [details] Patch
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.
Committed r199978: <http://trac.webkit.org/changeset/199978>