Bug 43629 - Use correct minus glyphs in MathML operators
Summary: Use correct minus glyphs in MathML operators
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-06 10:55 PDT by François Sausset
Modified: 2010-09-03 03:34 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.84 KB, patch)
2010-08-06 11:10 PDT, François Sausset
eric: review-
Details | Formatted Diff | Diff
Revised patch (3.22 KB, patch)
2010-08-06 15:12 PDT, François Sausset
no flags Details | Formatted Diff | Diff
Revised patch (4.08 KB, patch)
2010-08-07 03:48 PDT, François Sausset
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Revised patch (62.72 KB, patch)
2010-09-02 04:28 PDT, François Sausset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description François Sausset 2010-08-06 10:55:37 PDT
MathML mo elements should render "hyphen-minus" as "minus sign" (Unicode names).
Comment 1 François Sausset 2010-08-06 11:10:32 PDT
Created attachment 63743 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-08-06 13:15:10 PDT
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.
Comment 3 François Sausset 2010-08-06 15:12:14 PDT
Created attachment 63766 [details]
Revised patch

Code simplification.
Comment 4 Alexey Proskuryakov 2010-08-07 01:59:32 PDT
Comment on attachment 63766 [details]
Revised patch

+        return 0x2212;

Ideally, this should be in CharacterNames.h.
Comment 5 François Sausset 2010-08-07 03:48:34 PDT
Created attachment 63813 [details]
Revised patch

Use CharacterNames.h for better readability.
Comment 6 Kenneth Rohde Christiansen 2010-08-09 08:16:24 PDT
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?
Comment 7 François Sausset 2010-08-09 09:14:45 PDT
(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?
Comment 8 Darin Adler 2010-08-29 12:05:19 PDT
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.
Comment 9 François Sausset 2010-09-02 04:28:13 PDT
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.
Comment 10 Darin Adler 2010-09-02 07:57:04 PDT
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.
Comment 11 WebKit Commit Bot 2010-09-02 20:45:40 PDT
Comment on attachment 66352 [details]
Revised patch

Clearing flags on attachment: 66352

Committed r66708: <http://trac.webkit.org/changeset/66708>
Comment 12 WebKit Commit Bot 2010-09-02 20:45:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Review Bot 2010-09-02 21:30:36 PDT
http://trac.webkit.org/changeset/66708 might have broken GTK Linux 64-bit Debug
Comment 14 Philippe Normand 2010-09-03 00:14:16 PDT
(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?
Comment 15 Philippe Normand 2010-09-03 00:46:03 PDT
(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
Comment 16 François Sausset 2010-09-03 03:15:03 PDT
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?
Comment 17 Philippe Normand 2010-09-03 03:25:37 PDT
(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)
Comment 18 François Sausset 2010-09-03 03:34:08 PDT
OK, but I think I have not the rights to use webkit-patch rebaseline as I'm not a committer.