Summary: | More cleanup of MathML operator parsing | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
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, bfulgham, commit-queue, dbarton, esprehn+autocc, glenn, jfernandez, kondapallykalyan, rego | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 160301 | ||||||||||
Bug Blocks: | 156537, 160339 | ||||||||||
Attachments: |
|
Description
Frédéric Wang (:fredw)
2016-07-29 03:20:57 PDT
Created attachment 284858 [details] Patch (applies after bugs 160245, 160239, 160190 and 160301) Comment on attachment 284858 [details] Patch (applies after bugs 160245, 160239, 160190 and 160301) View in context: https://bugs.webkit.org/attachment.cgi?id=284858&action=review > Source/WebCore/mathml/MathMLOperatorElement.cpp:163 > + return m_properties.flags & flag; The caller already knows how to mask off the flag and in fact has code to do that in the non-dirty case. So the parse function should probably just set the flags and not try to return a boolean. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:73 > + UChar text = textContent(); The local variable name should be "character", not "text", I think. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:74 > + return 0x2061 <= text && text <= 0x2064; A function like this needs a comment to indicate why the range U+2061-U+2064 is correct, otherwise the code is mysterious. In particular, what guarantees these are the only 4 invisible operators. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:89 > + if (leadingSpace < 0) > + leadingSpace = 0; > + return leadingSpace; Typically we like to write this as an expression rather than an if. If it was an integer it would be: return std::max(0, leadingSpace); Maybe we can find a similar idiom for LayoutUnit? > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:94 > + // FIXME: Negative trailing spaces must be implemented (https://webkit.org/b/124830). Missing space here, incorrect indentation. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:152 > + aspect = float(minSizeValue) / size; We usually try to steer clear of function call style type casting for scalars. Here I would suggest: aspect = minSizeValue.toFloat() / size; > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:156 > + aspect = float(maxSizeValue) / size; Ditto. > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:81 > // The following operators are invisible: U+2061 FUNCTION APPLICATION, U+2062 INVISIBLE TIMES, U+2063 INVISIBLE SEPARATOR, U+2064 INVISIBLE PLUS. This comment needs to move in so it’s with the isInvisibleOperator function body; does not belong in the header. Created attachment 285107 [details] Patch (applies after bug 160190 and bug 160301) Created attachment 285126 [details]
Patch for EWS testing
Committed r204038: <http://trac.webkit.org/changeset/204038> |