Bug 219177

Created attachment 414618 [details] Patch

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.

(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.

Created attachment 417721 [details] Patch

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.

(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 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.

(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 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".

Created attachment 417743 [details] Patch

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 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 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.

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.

Created attachment 418080 [details] Patch

Committed r271715: <https://trac.webkit.org/changeset/271715> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418080 [details].