[Webkit-unassigned] [Bug 43629] Use correct minus glyphs in MathML operators

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43629


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #63813|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2010-08-29 12:05:19 PST ---
(From update of attachment 63813)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.