Bug 50018 – MathML: vertical alignment & bar thickness adjustments of fractions are needed

• Component: MathML
• Status: RESOLVED FIXED

[François Sausset]

The fraction bar thickness should follow the w3c MathML 3 recommendation:
http://www.w3.org/TR/MathML3/chapter3.html#presm.mfrac

The vertical alignment of the fraction bar should be more accurate, specially with operators (+,=,...).

[François Sausset]

Created attachment 74749 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 74749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74749&action=review

Please respond to or  fix these comments before landing.

> WebCore/ChangeLog:8
> +        Adjustments of the fraction bar default thickness and vertical alignment.
> +        https://bugs.webkit.org/show_bug.cgi?id=50018
> +
> +        Test: mathml/presentation/fractions.xhtml

It would be nice if you explained what change you actually made. Like why is this new change more correct?

> WebCore/mathml/RenderMathMLFraction.cpp:145
> +        int adjustForThickness = m_lineThickness > 1 ? static_cast<int>(m_lineThickness / 2) : 1;

Why static_cast<int> and not just int(m_lineThickness / 2) ?

[François Sausset]

(In reply to comment #2)
> It would be nice if you explained what change you actually made. Like why is this new change more correct?
I changed the default value of the bar thickness from "thin" to "medium" according to the MathML 3 recommendation. I thus had to adjust the numeric value of the "thin", "medium", and "thick" thicknesses for a correct rendering.
I also changed the vertical alignment of the bar, to be more reliable with operators like: +,=,...
See the removed FIXME in the patch for more details.

> Why static_cast<int> and not just int(m_lineThickness / 2) ?
There is no good reason. It will be corrected in the revised patch.

[François Sausset]

Created attachment 77806 [details]
Revised patch

[WebKit Commit Bot]

Comment on attachment 77806 [details]
Revised patch

Rejecting attachment 77806 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-attachment', '--no-update', '--non-interactive', 77806]" exit_code: 2
Last 500 characters of output:
to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/mathml/presentation/fractions.xhtml
patching file LayoutTests/platform/mac/mathml/presentation/fractions-expected.checksum
patching file LayoutTests/platform/mac/mathml/presentation/fractions-expected.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7388126

[François Sausset]

Created attachment 78401 [details]
Regenerated patch

The previous patch didn't apply because some MathML related files moved in the tree: WebCore/mathml to WebCore/rendering/mathml.

[WebKit Commit Bot]

Comment on attachment 78401 [details]
Regenerated patch

Clearing flags on attachment: 78401

Committed r75385: <http://trac.webkit.org/changeset/75385>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This broke Leopard build.  I think m_lineThickness should be float instead of double.

[Kenneth Rohde Christiansen]

(In reply to comment #9)
> This broke Leopard build.  I think m_lineThickness should be float instead of double.

Too bad it didn't ran on the bot. I don't have access to my devel machine right now, could you commit the potential fix? That would be great.

[Ryosuke Niwa]

(In reply to comment #10)
> (In reply to comment #9)
> > This broke Leopard build.  I think m_lineThickness should be float instead of double.
> 
> Too bad it didn't ran on the bot. I don't have access to my devel machine right now, could you commit the potential fix? That would be great.

I added the cast to float back in the line 160 for now.  You can change the type of m_lineThickness to float later if you think my suggestion is right.

http://trac.webkit.org/changeset/75394

[François Sausset]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > This broke Leopard build.  I think m_lineThickness should be float instead of double.
> > 
> > Too bad it didn't ran on the bot. I don't have access to my devel machine right now, could you commit the potential fix? That would be great.
> 
> I added the cast to float back in the line 160 for now.  You can change the type of m_lineThickness to float later if you think my suggestion is right.
> 
> http://trac.webkit.org/changeset/75394

Yes, a float is sufficient here for the accuracy and it is better to avoid an unneeded cast.

I'll do the change tomorrow by submitting a new patch.

[WebKit Review Bot]

http://trac.webkit.org/changeset/75385 might have broken GTK Linux 32-bit Release
The following tests are not passing:
fast/dom/Window/window-property-descriptors.html
fast/dom/prototype-inheritance.html
fast/js/global-constructors.html

[François Sausset]

Type change to float made in bug 52201.