Bug 159624

Created attachment 283758 [details] Patch

Comment on attachment 283758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283758&action=review It's great to see the style modifications being removed from the rendering code! However, it looks like some debase lining is needed. R- to correct the test cases, but otherwise this looks great! > Source/WebCore/ChangeLog:13 > + nearest valid value, which is zero. There probably isn't room here to explain properly, but what is the functional difference between an ignored line thickness, and a line thickness of zero? Is there some padding or other rendering effect that happens with zero-thickness that doesn't happen with 'ignored' render elements? > Source/WebCore/ChangeLog:15 > + No new tests, already covered by existing tests. It seems like some rebaselining is needed! > Source/WebCore/mathml/MathMLFractionElement.cpp:53 > + if (m_lineThickness.dirty) { I think this might be easier to follow if you did an early return here: If (!m_lineThickness.dirty) Return m_lineThickness; I would also do this early return above the comments. > Source/WebCore/mathml/MathMLFractionElement.cpp:73 > + if (alignment.dirty) { Again, I think an early return before doing the work of handling left/right, etc., is easier to follow. > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:-55 > - Good! > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:-150 > - updateFromElement(); Good! > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:-197 > - Good!!

Comment on attachment 283758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283758&action=review >> Source/WebCore/ChangeLog:13 >> + nearest valid value, which is zero. > > There probably isn't room here to explain properly, but what is the functional difference between an ignored line thickness, and a line thickness of zero? Is there some padding or other rendering effect that happens with zero-thickness that doesn't happen with 'ignored' render elements? So actually the *attribute* is ignored which means that the default value (generally nonzero) is used. I'll reword to clarify this. That's not really important anyway... >> Source/WebCore/ChangeLog:15 >> + No new tests, already covered by existing tests. > > It seems like some rebaselining is needed! Actually, the failures were due to having forgotten the change to the XCode project file (that's why I cancelled the review request). Normally no rebaseline is needed as the only change is how negative values are treated.

Created attachment 283768 [details] Patch Let's see the results with the changes to XCode project file.

Attachment 283768 [details] did not pass style-queue: ERROR: Source/WebCore/mathml/MathMLFractionElement.cpp:74: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 283768 [details] Patch Note about the style error: I have to fix the extra whitespace I added.

Comment on attachment 283768 [details] Patch Please fix the style error. Otherwise, r=me.

Committed r203285: <http://trac.webkit.org/changeset/203285>