Bug 43629 – Use correct minus glyphs in MathML operators

• Component: MathML
• Status: RESOLVED FIXED

[François Sausset]

MathML mo elements should render "hyphen-minus" as "minus sign" (Unicode names).

[François Sausset]

Created attachment 63743 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 63743 [details]
Patch

This seems like this wants to be an inline funcion in some MathML header which can be used in both places here.

UChar fixMinusSign(UChar) or similar.

[François Sausset]

Created attachment 63766 [details]
Revised patch

Code simplification.

[Alexey Proskuryakov]

Comment on attachment 63766 [details]
Revised patch

+        return 0x2212;

Ideally, this should be in CharacterNames.h.

[François Sausset]

Created attachment 63813 [details]
Revised patch

Use CharacterNames.h for better readability.

[Kenneth Rohde Christiansen]

Comment on attachment 63813 [details]
Revised patch

Uh, is there no better way of doing this? and will there be more cases like this in the future?

[François Sausset]

(In reply to comment #6)
> (From update of attachment 63813 [details])
> Uh, is there no better way of doing this? and will there be more cases like this in the future?

To my knowledge, this is the only glyph that needs such a conversion. But, I'm perhaps wrong.

What are you finding not very clean? The loop on the characters?

[Darin Adler]

Comment on attachment 63813 [details]
Revised patch

Looks like something good to do!

Bug fixes require regression tests, so please add a regression test.

I don’t like the name of the fixMinusSign function. The function replaces hyphenMinus with minusSign, but that's only correct in contexts where we know it's a minus sign and not a hyphen. Is there some better name for the function that expressions that more clearly? Maybe convertHyphenToMinusSign is a better name.

>        m_stretchHeight(0),
>        m_operator(operatorChar)
>  {
> +    // Modify the minus glyph ("hyphen-minus" to "minus sign").
> +    m_operator = fixMinusSign(m_operator);
>  }

If the fixMinusSign has a good enough function name, then there is no need for the comment. Also, comments should answer the question "why", not the question "what", since the code can speak for itself about what it does. The comments need to explain twhat the code does not, the reason why. I would write it like this:

    , m_operator(fixMinusSign(operatorChar))

And not have a separate line. But this depends on the name of fixMinusSign being clear. We still might need a why comment. Where does the hyphen come from? Why doesn’t that code just produce a minus sign? Why is it this class’s responsibility to change it. Those are the questions the comment could answer.


> -            if (Element* mo = static_cast<Element*>(node()))
> -                text = new (renderArena()) RenderText(node(), StringImpl::create(mo->textContent().characters(), mo->textContent().length()));
> +            if (Element* mo = static_cast<Element*>(node())) {
> +                UChar* chars;
> +                unsigned charLength = mo->textContent().length();
> +                chars = new UChar[charLength];
> +                // Modify minus glyphs ("hyphen-minus" to "minus sign").
> +                for (unsigned i = 0; i < charLength; i++)
> +                    chars[i] = fixMinusSign(mo->textContent().characters()[i]);
> +                text = new (renderArena()) RenderText(node(), StringImpl::create(chars, charLength));
> +                delete[] chars;
> +            }

This is not the right style.

It's unnecessarily inefficient to always allocate an array of UChar on the heap each time we do this. Also, we frown on explicit new and delete since they are easy to use wrong and we are trying to eliminate them from the code.

The code here already inefficient because it calls the textContent function twice, creating two strings, and then destroying both of them, and then creating a new StringImpl that copies all the characters even when there is no need to do so. A much more efficient way to write this is:

    if (Element* mo = static_cast<Element*>(node()))
        text = new (renderArena()) RenderText(node(), mo->textContent().replace(hyphenMinus, minusSign).impl());

review- because of the lack of the regression test and because we should make at least some of the changes I suggest above.

[François Sausset]

Created attachment 66352 [details]
Revised patch

Proposed changes have been integrated.

About comments, they were here for the first version of the patch. They were indeed useless in the last version of the patch.
I added a "why" comment in the convertHyphenMinusToMinusSign function.

I was not aware of the replace() method. Thanks for pointing it to me.
Code is much cleaner now.

[Darin Adler]

Comment on attachment 66352 [details]
Revised patch

> +inline UChar convertHyphenMinusToMinusSign(UChar glyph)
> +{
> +    // When rendered as a mathematical operator, minus glyph should be larger.
> +    if (glyph == hyphenMinus)
> +        return minusSign;
>      
> +    return glyph;
>  }

I don't think this needs to be in the RenderMathMLOperator.h file. It could just as well be private in the .cpp file.

Thanks for making this more of a "why" comment. It still is slightly oblique, but I can't figure out an easy way to make it more direct.

Otherwise, patch seems fine.

I’ll set both the review flag and the commit-queue flag, but if you would like to make the function private, you could clear the commit-queue one.

[WebKit Commit Bot]

Comment on attachment 66352 [details]
Revised patch

Clearing flags on attachment: 66352

Committed r66708: <http://trac.webkit.org/changeset/66708>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66708 might have broken GTK Linux 64-bit Debug

[Philippe Normand]

(In reply to comment #13)
> http://trac.webkit.org/changeset/66708 might have broken GTK Linux 64-bit Debug

All 3 GTK slaves.
Is a rebaseline needed or not?

[Philippe Normand]

(In reply to comment #14)
> (In reply to comment #13)
> > http://trac.webkit.org/changeset/66708 might have broken GTK Linux 64-bit Debug
> 
> All 3 GTK slaves.
> Is a rebaseline needed or not?

I rebaselined the test, since it was updated and the mac results as well. See r66718

[François Sausset]

Thanks.

Tests indeed needed a rebaseline.

However, I was not aware that mathml tests results were available on GTK.
In the future, how can I do, as I'm developing on a mac?

[Philippe Normand]

(In reply to comment #16)
> Thanks.
> 
> Tests indeed needed a rebaseline.
> 
> However, I was not aware that mathml tests results were available on GTK.
> In the future, how can I do, as I'm developing on a mac?

Well it'd be nice (but not sure realistic) to have try-bots running the layout tests with the r? patch so in theory you could get the new baselines from them ;)

I think what people do currently is either check the LayoutTests directory for platform-specific baselines or land, put the trees on fire and get the baselines from the buildbot (which is what i did with webkit-patch rebaseline)

[François Sausset]

OK, but I think I have not the rights to use webkit-patch rebaseline as I'm not a committer.