Bug 159624 – Move parsing of mfrac attributes into a MathMLFractionElementClass

• Component: MathML
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

This is bug 156536 for the following attributes:

- linethickness (MathMLLength, see bug 156792)
- numalign
- denomalign

[Frédéric Wang (:fredw)]

Created attachment 283758 [details]
Patch

[Brent Fulgham]

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!!

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 283768 [details]
Patch

Let's see the results with the changes to XCode project file.

[WebKit Commit Bot]

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.

[Frédéric Wang (:fredw)]

Comment on attachment 283768 [details]
Patch

Note about the style error: I have to fix the extra whitespace I added.

[Brent Fulgham]

Comment on attachment 283768 [details]
Patch

Please fix the style error. Otherwise, r=me.

[Frédéric Wang (:fredw)]

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