Bug 219177 - zero and rename boundingRect pointer in Font::platformBoundsForGlyph() to CTFontGetBoundingRectsForGlyphs()
Summary: zero and rename boundingRect pointer in Font::platformBoundsForGlyph() to CTF...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-19 13:32 PST by Julian Gonzalez
Modified: 2021-01-21 14:56 PST (History)
3 users (show)

See Also:


Attachments
Patch (2.73 KB, patch)
2020-11-19 14:10 PST, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (2.81 KB, patch)
2021-01-15 12:41 PST, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (2.82 KB, patch)
2021-01-15 14:50 PST, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (2.77 KB, patch)
2021-01-21 13:42 PST, Julian Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Gonzalez 2020-11-19 13:32:06 PST
Follow up to 218812 - zero out the CGRect structures on the stack from that change, so as not to pass uninitialized memory to CTFontGetBoundingRectsForGlyphs().

<rdar://problem/71547171>
Comment 1 Julian Gonzalez 2020-11-19 14:10:35 PST
Created attachment 414618 [details]
Patch
Comment 2 Darin Adler 2020-11-19 15:36:25 PST
Comment on attachment 414618 [details]
Patch

Good to have the code match what the change log says. And it was my comment that led to this change.

But why does this need to be a zero rect rather than uninitialized? And why can’t we pass nullptr instead?

A little confused about that.
Comment 3 Ryosuke Niwa 2020-11-19 16:29:34 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 414618 [details]
> Patch
> 
> Good to have the code match what the change log says. And it was my comment
> that led to this change.

Yeah, we should mention this in the change log.

> But why does this need to be a zero rect rather than uninitialized? And why
> can’t we pass nullptr instead?

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.
Comment 4 Julian Gonzalez 2021-01-15 12:41:23 PST
Created attachment 417721 [details]
Patch
Comment 5 Darin Adler 2021-01-15 13:02:20 PST
Comment on attachment 417721 [details]
Patch

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

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:703
> +    CGRect zeroRect = { };
> +    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.

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:748
> +        CGRect zeroRect = { };
> +        if (!CGRectIsEmpty(CTFontGetBoundingRectsForGlyphs(platformFont, kCTFontOrientationDefault, &lowercaseAGlyph, &zeroRect, 1)))

Ditto.
Comment 6 Julian Gonzalez 2021-01-15 13:37:03 PST
(In reply to Darin Adler from comment #5)
> Comment on attachment 417721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417721&action=review
> 
> > Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:703
> > +    CGRect zeroRect = { };
> > +    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."

> > Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:748
> > +        CGRect zeroRect = { };
> > +        if (!CGRectIsEmpty(CTFontGetBoundingRectsForGlyphs(platformFont, kCTFontOrientationDefault, &lowercaseAGlyph, &zeroRect, 1)))
> 
> Ditto.

If we want to ignore the recommendation and just use nullptr, then we should just revert 218812.
Comment 7 Darin Adler 2021-01-15 13:48:31 PST
Comment on attachment 417721 [details]
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.
Comment 8 Julian Gonzalez 2021-01-15 13:59:12 PST
(In reply to Darin Adler from comment #7)
> Comment on attachment 417721 [details]
> 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.

Sounds good. How does 'dummyRect' sound? (possibly with a comment?)
Comment 9 Darin Adler 2021-01-15 14:01:11 PST
Comment on attachment 417721 [details]
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.
> 
> Sounds good. How does 'dummyRect' sound? (possibly with a comment?)

That’s fine. I might also suggest "ignoredRect".
Comment 10 Julian Gonzalez 2021-01-15 14:50:38 PST
Created attachment 417743 [details]
Patch
Comment 11 Darin Adler 2021-01-15 14:57:46 PST
Comment on attachment 417743 [details]
Patch

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

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:702
> +    CGRect ignoredRect = { };

No reason for the "= { }". The name emptyRect confused me into thinking it was needed. So the only thing wrong was the name, the code was totally fine. This patch is OK as is, but also one that just ranges would be fine. And the bug could be retitled since this is just naming clarity, no need to zero anything.
Comment 12 Darin Adler 2021-01-15 14:58:10 PST
Comment on attachment 417743 [details]
Patch

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

>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:702
>> +    CGRect ignoredRect = { };
> 
> No reason for the "= { }". The name emptyRect confused me into thinking it was needed. So the only thing wrong was the name, the code was totally fine. This patch is OK as is, but also one that just ranges would be fine. And the bug could be retitled since this is just naming clarity, no need to zero anything.

that just *renames*, not that just *ranges*
Comment 13 Ryosuke Niwa 2021-01-15 17:16:22 PST
Comment on attachment 417743 [details]
Patch

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

>>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:702
>>> +    CGRect ignoredRect = { };
>> 
>> No reason for the "= { }". The name emptyRect confused me into thinking it was needed. So the only thing wrong was the name, the code was totally fine. This patch is OK as is, but also one that just ranges would be fine. And the bug could be retitled since this is just naming clarity, no need to zero anything.
> 
> that just *renames*, not that just *ranges*

It's probably still a good practice to zero it out so that there is no ambiguity we won't have indeterministic behavior here.
Comment 14 Julian Gonzalez 2021-01-21 12:56:29 PST
OK - I'll rename the variable (keeping the zeroing as requested by Ryosuke) and retitle the bug here slightly to reflect the new plan. Thanks folks.
Comment 15 Julian Gonzalez 2021-01-21 13:42:17 PST
Created attachment 418080 [details]
Patch
Comment 16 EWS 2021-01-21 14:56:48 PST
Committed r271715: <https://trac.webkit.org/changeset/271715>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418080 [details].