[webkit-reviews] review granted: [Bug 219177] zero boundingRect pointer in Font::platformBoundsForGlyph() to CTFontGetBoundingRectsForGlyphs() : [Attachment 417721] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Julian Gonzalez
<julian_a_gonzalez at apple.com>'s request for review:
Bug 219177: zero boundingRect pointer in Font::platformBoundsForGlyph() to
CTFontGetBoundingRectsForGlyphs()
https://bugs.webkit.org/show_bug.cgi?id=219177

Attachment 417721: Patch

https://bugs.webkit.org/attachment.cgi?id=417721&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 417721
  --> https://bugs.webkit.org/attachment.cgi?id=417721
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417721&action=review

>>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:703
>>> +	 boundingBox = CTFontGetBoundingRectsForGlyphs(m_platformData.ctFont(),
platformData().orientation() == FontOrientation::Vertical ?
kCTFontOrientationVertical : kCTFontOrientationHorizontal, &glyph, &zeroRect,
1);
>> 
>> After more research I now realize this is an *out* argument from
CTFontGetBoundingRectsForGlyphs. So it must not be named "emptyRect" or
"zeroRect", since it will get a value and will not be empty or zero! Instead we
should pass nullptr. The CTFontGetBoundingRectsForGlyphs documentation for this
arguments says "Can be NULL, in which case only the overall bounding rect is
calculated." I suggest we use nullptr.
> 
> Darin, the reason for 218812 was that passing nullptr is not as performant as
passing stack-allocated memory, because a heap allocation is performed in that
case.
> This is what Ryosuke was getting at when he wrote:
> 
> "So the person who maintains this function told us that passing nullptr will
result in heap memory allocation so it's slightly more efficient to pass in a
rect."

Oops, forgot that. Then we should not name this "zero rect" but otherwise this
is great.