Bug 160336

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>