RESOLVED FIXED 43629
Use correct minus glyphs in MathML operators
https://bugs.webkit.org/show_bug.cgi?id=43629
Summary Use correct minus glyphs in MathML operators
François Sausset
Reported 2010-08-06 10:55:37 PDT
MathML mo elements should render "hyphen-minus" as "minus sign" (Unicode names).
Attachments
Patch (2.84 KB, patch)
2010-08-06 11:10 PDT, François Sausset
eric: review-
Revised patch (3.22 KB, patch)
2010-08-06 15:12 PDT, François Sausset
no flags
Revised patch (4.08 KB, patch)
2010-08-07 03:48 PDT, François Sausset
darin: review-
darin: commit-queue-
Revised patch (62.72 KB, patch)
2010-09-02 04:28 PDT, François Sausset
no flags
François Sausset
Comment 1 2010-08-06 11:10:32 PDT
Eric Seidel (no email)
Comment 2 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.
François Sausset
Comment 3 2010-08-06 15:12:14 PDT
Created attachment 63766 [details] Revised patch Code simplification.
Alexey Proskuryakov
Comment 4 2010-08-07 01:59:32 PDT
Comment on attachment 63766 [details] Revised patch + return 0x2212; Ideally, this should be in CharacterNames.h.
François Sausset
Comment 5 2010-08-07 03:48:34 PDT
Created attachment 63813 [details] Revised patch Use CharacterNames.h for better readability.
Kenneth Rohde Christiansen
Comment 6 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?
François Sausset
Comment 7 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?
Darin Adler
Comment 8 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.
François Sausset
Comment 9 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.
Darin Adler
Comment 10 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.
WebKit Commit Bot
Comment 11 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>
WebKit Commit Bot
Comment 12 2010-09-02 20:45:45 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 13 2010-09-02 21:30:36 PDT
http://trac.webkit.org/changeset/66708 might have broken GTK Linux 64-bit Debug
Philippe Normand
Comment 14 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?
Philippe Normand
Comment 15 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
François Sausset
Comment 16 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?
Philippe Normand
Comment 17 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)
François Sausset
Comment 18 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.
Note You need to log in before you can comment on or make changes to this bug.