Summary: | Move parsing of mfrac attributes into a MathMLFractionElementClass | ||||||||
---|---|---|---|---|---|---|---|---|---|
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, rego, svillar | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 156792 | ||||||||
Bug Blocks: | 156536, 159876 | ||||||||
Attachments: |
|
Description
Frédéric Wang (:fredw)
2016-07-11 07:19:12 PDT
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> |