Bug 159624 - Move parsing of mfrac attributes into a MathMLFractionElementClass
Summary: Move parsing of mfrac attributes into a MathMLFractionElementClass
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 156792
Blocks: 156536 159876
  Show dependency treegraph
 
Reported: 2016-07-11 07:19 PDT by Frédéric Wang (:fredw)
Modified: 2016-07-18 04:43 PDT (History)
5 users (show)

See Also:


Attachments
Patch (24.82 KB, patch)
2016-07-15 06:31 PDT, Frédéric Wang (:fredw)
bfulgham: review-
Details | Formatted Diff | Diff
Patch (29.57 KB, patch)
2016-07-15 09:51 PDT, Frédéric Wang (:fredw)
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2016-07-11 07:19:12 PDT
This is bug 156536 for the following attributes:

- linethickness (MathMLLength, see bug 156792)
- numalign
- denomalign
Comment 1 Frédéric Wang (:fredw) 2016-07-15 06:31:04 PDT
Created attachment 283758 [details]
Patch
Comment 2 Brent Fulgham 2016-07-15 09:22:44 PDT
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 3 Frédéric Wang (:fredw) 2016-07-15 09:47:07 PDT
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.
Comment 4 Frédéric Wang (:fredw) 2016-07-15 09:51:02 PDT
Created attachment 283768 [details]
Patch

Let's see the results with the changes to XCode project file.
Comment 5 WebKit Commit Bot 2016-07-15 09:55:39 PDT
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 6 Frédéric Wang (:fredw) 2016-07-15 10:26:16 PDT
Comment on attachment 283768 [details]
Patch

Note about the style error: I have to fix the extra whitespace I added.
Comment 7 Brent Fulgham 2016-07-15 11:13:14 PDT
Comment on attachment 283768 [details]
Patch

Please fix the style error. Otherwise, r=me.
Comment 8 Frédéric Wang (:fredw) 2016-07-15 11:25:43 PDT
Committed r203285: <http://trac.webkit.org/changeset/203285>