[webkit-reviews] review granted: [Bug 128907] Migrate the MathML stretchy code from UChar to Glyph : [Attachment 226493] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Frédéric Wang <fred.wang at free.fr>'s
request for review:
Bug 128907: Migrate the MathML stretchy code from UChar to Glyph
https://bugs.webkit.org/show_bug.cgi?id=128907

Attachment 226493: Patch
https://bugs.webkit.org/attachment.cgi?id=226493&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226493&action=review


r=me but I presume you are going to make some refinements based on Chris’s
comments.

>>> Source/WebCore/platform/graphics/GraphicsContext.h:353
>>> +	     void drawGlyphs(const Font&, const SimpleFontData*, const
GlyphBuffer&, const FloatPoint&, int from = 0, int numGlyphs = 1);
>> 
>> seems like the order of arguments should be the same as in Font.h
> 
> This is to allow default argument values from and numGlyphs. This function
was already defined in Font.h, but for some reason the argument were not in the
same order as drawText. I wonder if I should update all the calls to drawGlyphs
or just give up with trying to have default argument values for
GraphicsContext::drawGlyphs...

When adding a new function, we should use references when possible unless
something can be null. So it should be const SimpleFontData& unless a null
value is legal.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:57
> +static StretchyCharacter stretchyCharacters[14] = {

Why not const?

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1285
>> +FloatRect RenderMathMLOperator::boundsForGlyph(GlyphData data)
> 
> can this be passed as reference

I am not sure that a const GlyphData& is better here. In the past we almost
always passed structures by const& but more recently Anders has said that we
can get good performance copying simple structures as long as they are truly
simple and GlyphData is one of those super-simple structures. Anders, care to
comment?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:73
> +    // FIXME: Ideally (for MATH fonts) we would only need to store the Glyph
indices here.

Why is MATH all capitals here? Is that a specific technical term that is always
all capitals like that?