Bug 156906

Summary:

Minor refactoring in RenderMathMLOperator

Product:

WebKit

Reporter:

Frédéric Wang (:fredw) <fred.wang>

Component:

MathML

Assignee:

Frédéric Wang (:fredw) <fred.wang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alex, commit-queue, dbarton, esprehn+autocc, glenn, kondapallykalyan, mrobinson, rego

Priority:

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-

Frédéric Wang (:fredw)

Reported 2016-04-22 04:59:28 PDT

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

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Frédéric Wang (:fredw)

Comment 1 2016-04-22 05:06:25 PDT

Created attachment 277045 [details] Patch

Manuel Rego Casasnovas

Comment 2 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.

Frédéric Wang (:fredw)

Comment 3 2016-04-22 06:48:25 PDT

Created attachment 277059 [details] Patch

Martin Robinson

Comment 4 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.

Frédéric Wang (:fredw)

Comment 5 2016-04-25 00:04:28 PDT

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

Note You need to log in before you can comment on or make changes to this bug.