Bug 160336

Summary: More cleanup of MathML operator parsing
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, 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 Flags
Patch (applies after bugs 160245, 160239, 160190 and 160301)
darin: review+
Patch (applies after bug 160190 and bug 160301)
none
Patch for EWS testing none

Frédéric Wang (:fredw)
Reported 2016-07-29 03:20:57 PDT
Some cleanup are possible once the attribute parsing is moved into MathMLOperatorElement.
Attachments
Patch (applies after bugs 160245, 160239, 160190 and 160301) (17.34 KB, patch)
2016-07-29 03:45 PDT, Frédéric Wang (:fredw)
darin: review+
Patch (applies after bug 160190 and bug 160301) (15.32 KB, patch)
2016-08-02 06:59 PDT, Frédéric Wang (:fredw)
no flags
Patch for EWS testing (23.20 KB, patch)
2016-08-02 12:47 PDT, Frédéric Wang (:fredw)
no flags
Frédéric Wang (:fredw)
Comment 1 2016-07-29 03:45:28 PDT
Created attachment 284858 [details] Patch (applies after bugs 160245, 160239, 160190 and 160301)
Darin Adler
Comment 2 2016-07-31 21:33:39 PDT
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.
Frédéric Wang (:fredw)
Comment 3 2016-08-02 06:59:33 PDT
Created attachment 285107 [details] Patch (applies after bug 160190 and bug 160301)
Frédéric Wang (:fredw)
Comment 4 2016-08-02 12:47:02 PDT
Created attachment 285126 [details] Patch for EWS testing
Frédéric Wang (:fredw)
Comment 5 2016-08-02 13:27:33 PDT
Note You need to log in before you can comment on or make changes to this bug.