Bug 239484

Created attachment 457860 [details] Patch

<rdar://problem/91187123>

Created attachment 457936 [details] Patch

Removing r? to investigate test failures.

Created attachment 458034 [details] Patch

Comment on attachment 458034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458034&action=review > Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:218 > +static Vector<CGSize> computeHorizontalAdvancesFromPositions(const CGPoint positions[], size_t count, const CGAffineTransform& textMatrix) I think this function should return AdvancesAndInitialPosition also. The initialPenPosition is calculated in DrawGlyphsRecorder::recordDrawGlyphs() for the horizontal case like this: initialPenPosition = textMatrix.mapPoint(positions[0]); > Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:223 > Vector<CGSize> result; > + result.reserveInitialCapacity(count); Can't this written like the one below: Vector<CGSize, 256> result(count); I honestly do not understand the difference between setting the initialCapcity of the Vector, passing the size in the constructor and calling reserveInitialCapacity()? What is the most efficient way to create a fixed length Vector? > Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:230 > + result.uncheckedConstructAndAppend(CGSizeMake(0, 0)); Should this append CGSizeZero like what computeVerticalAdvancesFromPositions does? > Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:234 > +struct AdvancesAndInitialPosition { Should this struct have a constructor which takes the size of the advances and initializes its capacity? > Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:244 > + if (!count) > + return { { }, CGPointZero }; This special case is already handled by the caller DrawGlyphsRecorder::recordDrawGlyphs()? I do not think we should be drawing anything if count is zero. So this function should not be called in this case. I think we can add an ASSERT(count) here and in computeHorizontalAdvancesFromPositions() as well. > Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:251 > + auto translationInUserCoordinates = CGSizeApplyAffineTransform(translation, syntheticObliqueLessTextMatrix); > + return CGPointMake(positionInUserCoordinates.x - translationInUserCoordinates.width, positionInUserCoordinates.y - translationInUserCoordinates.height); I am not sure if this would be clear/clean or not, but are these statements equivalent to? return CGPointMake(positionInUserCoordinates.x + translation.height, positionInUserCoordinates.y + translation.width); > Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:262 > + result.advances.uncheckedConstructAndAppend(CGSizeMake(nextPosition.x - previousPosition.x, nextPosition.y - previousPosition.y)); I find It diffract to read this code which says "currentAdvance = nextPosition - previousPosition". The question is: what is the current position then? Since I starts with 1, I would suggest using "currentPosition" instead of nextPosition. > Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:319 > + FloatPoint initialPenPosition; > > + Vector<CGSize> advances; These can replaced by the following statement if computeHorizontalAdvancesFromPositions returns AdvancesAndInitialPosition: AdvancesAndInitialPosition result; > Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:321 > + Vector<CGSize, 256> translations(count); Is there a reason for setting the initialCapacity and the size of a fixed-size vector? > Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:266 > + auto syntheticObliqueLessTextMatrix = computeBaseVerticalTextMatrix(computeBaseOverallTextMatrix(std::nullopt)); I am confused by the name and the calculation of this matrix. computeBaseOverallTextMatrix(std::nullopt) returns AffineTransform().flipyY() = AffineTransform(1, 0, 0, -1, 0, 0) computeBaseVerticalTextMatrix(computeBaseOverallTextMatrix(std::nullopt)) returns rotateLeftTransform() * computeBaseOverallTextMatrix(std::nullopt) = AffineTransform(0, -1, 1, 0, 0, 0) * AffineTransform(1, 0, 0, -1, 0, 0) So everything here is constant. Can't this be a "static constexpr"? Can't we choose a better name which reflects it is constant matrix?

Comment on attachment 458034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458034&action=review >> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:218 >> +static Vector<CGSize> computeHorizontalAdvancesFromPositions(const CGPoint positions[], size_t count, const CGAffineTransform& textMatrix) > > I think this function should return AdvancesAndInitialPosition also. The initialPenPosition is calculated in DrawGlyphsRecorder::recordDrawGlyphs() for the horizontal case like this: > > initialPenPosition = textMatrix.mapPoint(positions[0]); Good idea. >> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:223 >> + result.reserveInitialCapacity(count); > > Can't this written like the one below: > > Vector<CGSize, 256> result(count); > > I honestly do not understand the difference between setting the initialCapcity of the Vector, passing the size in the constructor and calling reserveInitialCapacity()? What is the most efficient way to create a fixed length Vector? Passing the size in the constructor causes the constructor to actually create a bunch of default items inside the vector. The Vector constructor is smart enough to either call T() or use memset() if possible, when initializing. In the case of CGSize, it will call memset(). If you then overwrite every item in the vector, then that initialization was wasted. It matters more for complex types which do nontrivial work in their constructors. On the other hand, reserveInitialCapacity() doesn't actually construct any items - it just allocates the storage for where the items will be constructed later when you call uncheckedAppend(). If you're going to scatter a bunch of writes throughout the vector, rather than filling it in beginning-to-end, then that's a situation where you would pass the size to the constructor. >> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:230 >> + result.uncheckedConstructAndAppend(CGSizeMake(0, 0)); > > Should this append CGSizeZero like what computeVerticalAdvancesFromPositions does? Yes! Good catch. >> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:244 >> + return { { }, CGPointZero }; > > This special case is already handled by the caller DrawGlyphsRecorder::recordDrawGlyphs()? I do not think we should be drawing anything if count is zero. So this function should not be called in this case. I think we can add an ASSERT(count) here and in computeHorizontalAdvancesFromPositions() as well. Right. Good catch. >> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:251 >> + return CGPointMake(positionInUserCoordinates.x - translationInUserCoordinates.width, positionInUserCoordinates.y - translationInUserCoordinates.height); > > I am not sure if this would be clear/clean or not, but are these statements equivalent to? > > return CGPointMake(positionInUserCoordinates.x + translation.height, positionInUserCoordinates.y + translation.width); I think it's important for clarity to use the same functions that fillVectorWithVerticalGlyphPositions() uses. However, CGSizeApplyAffineTransform() is slower than the math that you're proposing. I guess I should run an A/B test to make sure this isn't causing a performance regression. If it is, then this patch should use the simpler math as you suggest. >> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:262 >> + result.advances.uncheckedConstructAndAppend(CGSizeMake(nextPosition.x - previousPosition.x, nextPosition.y - previousPosition.y)); > > I find It diffract to read this code which says "currentAdvance = nextPosition - previousPosition". The question is: what is the current position then? > > Since I starts with 1, I would suggest using "currentPosition" instead of nextPosition. Yes, that's a good improvement. >> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:319 >> + Vector<CGSize> advances; > > These can replaced by the following statement if computeHorizontalAdvancesFromPositions returns AdvancesAndInitialPosition: > > AdvancesAndInitialPosition result; Right, yes! >> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:321 >> + Vector<CGSize, 256> translations(count); > > Is there a reason for setting the initialCapacity and the size of a fixed-size vector? We're passing in the size to the Vector constructor here because the data in the vector is populated by CTFontGetVerticalTranslationsForGlyphs(), which accepts a CGSize* rather than a Vector<CGSize>&. If we used reserveInitialCapacity() instead, then as soon as we use operator[] to access any of the values, we'd get an ASSERT inside Vector. The "256" is inline capacity, rather than initial capacity. If count <= 256, then the translations will lie on the stack, inside the vector object itself, rather than in heap malloc()'ed storage. sizeof(Vector<CGSize, 256>) is (close to) sizeof(CGSize) * 256. >> Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:266 >> + auto syntheticObliqueLessTextMatrix = computeBaseVerticalTextMatrix(computeBaseOverallTextMatrix(std::nullopt)); > > I am confused by the name and the calculation of this matrix. > > computeBaseOverallTextMatrix(std::nullopt) returns AffineTransform().flipyY() = AffineTransform(1, 0, 0, -1, 0, 0) > computeBaseVerticalTextMatrix(computeBaseOverallTextMatrix(std::nullopt)) returns rotateLeftTransform() * computeBaseOverallTextMatrix(std::nullopt) = AffineTransform(0, -1, 1, 0, 0, 0) * AffineTransform(1, 0, 0, -1, 0, 0) > > So everything here is constant. Can't this be a "static constexpr"? Can't we choose a better name which reflects it is constant matrix? It can be static, but I can't make it constexpr because: error: constexpr variable cannot have non-literal type 'const WebCore::AffineTransform' Maybe I should try to refactor this to use CGAffineTransform, so that it *can* be constexpr...

Pull request: https://github.com/WebKit/WebKit/pull/1885

Committed 251981@main (d6a262142655): <https://commits.webkit.org/251981@main> Reviewed commits have been landed. Closing PR #1885 and removing active labels.