[webkit-reviews] review granted: [Bug 214769] Spacing of Chinese characters is inconsistent in macOS 11/Safari 14 beta : [Attachment 406266] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 214769: Spacing of Chinese characters is inconsistent in macOS 11/Safari 14
beta
https://bugs.webkit.org/show_bug.cgi?id=214769

Attachment 406266: Patch

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




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

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

The win EWS failure does seem to be related to these changes. The mac-debug-wk1
one does not, so I am pushing the retry button.

Looks good, but need to figure out why the Windows failure is happening.

> Source/WebCore/platform/graphics/FontCascade.cpp:1395
> +    glyphBuffer.setInitialAdvance(FloatSize(initialAdvance, 0) +
static_cast<FloatSize>(glyphBuffer.initialAdvance()));

Could we use a local variable to avoid the static_cast here?

> Source/WebCore/platform/graphics/GlyphBuffer.h:262
> +	   GlyphBufferAdvance& lastAdvance = m_advances[index];

auto&

> Source/WebCore/platform/graphics/WidthIterator.cpp:197
> +	   if (character == tabCharacter)
> +	       m_containsTabs = true;

Consider |= instead?

> Source/WebCore/platform/graphics/WidthIterator.cpp:201
> +	   bool currentIsLastCharacter = currentCharacterIndex + advanceLength
== static_cast<size_t>(m_run.length());
> +	   if (currentIsLastCharacter)

I think this would read better without the local variable.

Is the static_cast<size_t> really needed?

> Source/WebCore/platform/graphics/WidthIterator.cpp:303
> +    if (hasExtraSpacing()) { // Tab stops apparently don't count as "extra
spacing".

The "apparently" here spreads a little fear, uncertainty, and doubt. Could you
write this in a more affirmative way? Or if you think this is wrong, say why?

> Source/WebCore/platform/graphics/WidthIterator.cpp:306
> +	   { // Letter-spacing

Could we comment in a more traditional way? I don’t think adding a comment on
the brace that starts a block is helpful enough to be worth the unconventional
formatting. I also also don’t think we need to scope "baseWidth", and so
traditional blocks of code would be OK without adding braces. If it’s critical
to break these sections out a traditional way would be to use named functions,
and I think that *would* be practical here.

> Source/WebCore/platform/graphics/WidthIterator.cpp:307
> +	       // This is an approximation to determine if the character is
non-visible. Non-visible characters don't get letter-spacing.

Not totally sure about the code below.

I’m not sure what the word "approximation" means in this context. Checking a
floating point sum for exactly zero is quite precise, so maybe we mean
something more like "heuristic". What would the "real" way to find out if
something is non-visible be? Is it OK to work around it by checking for the sum
of advances to be zero? Since are are summing the widths it seems that we think
it’s likely that there will be a combination of positive and negative widths
that exactly cancel each other out, and it’s important to treat that case as
non-visible? Is that right? Do we have tests of cases like that?

> Source/WebCore/platform/graphics/WidthIterator.cpp:321
> +	       bool currentIsLastCharacter = m_lastCharacterIndex &&
currentCharacterIndex ==
static_cast<GlyphBufferStringOffset>(*m_lastCharacterIndex);

Is there a way to avoid the static_cast<GlyphBufferStringOffset> that makes
this line so long?

> Source/WebCore/platform/graphics/WidthIterator.cpp:339
> +	       bool runForcesLeftExpansion = (m_run.expansionBehavior() &
LeftExpansionMask) == ForceLeftExpansion;
> +	       bool runForcesRightExpansion = (m_run.expansionBehavior() &
RightExpansionMask) == ForceRightExpansion;
> +	       bool runForbidsLeftExpansion = (m_run.expansionBehavior() &
LeftExpansionMask) == ForbidLeftExpansion;
> +	       bool runForbidsRightExpansion = (m_run.expansionBehavior() &
RightExpansionMask) == ForbidRightExpansion;
> +
> +	       bool forceLeftExpansion = false;
> +	       bool forceRightExpansion = false;
> +	       bool forbidLeftExpansion = false;
> +	       bool forbidRightExpansion = false;
> +	       if (runForcesLeftExpansion)
> +		   forceLeftExpansion = m_run.ltr() ? !currentCharacterIndex :
currentIsLastCharacter;
> +	       if (runForcesRightExpansion)
> +		   forceRightExpansion = m_run.ltr() ? currentIsLastCharacter :
!currentCharacterIndex;
> +	       if (runForbidsLeftExpansion)
> +		   forbidLeftExpansion = m_run.ltr() ? !currentCharacterIndex :
currentIsLastCharacter;
> +	       if (runForbidsRightExpansion)
> +		   forbidRightExpansion = m_run.ltr() ? currentIsLastCharacter
: !currentCharacterIndex;

I think these local variables makes the code harder to read. I suggest writing
it more like this:

    bool isLeft = !currentCharacterIndex;
    bool isRight = m_lastCharacterIndex && currentCharacterIndex ==
static_cast<GlyphBufferStringOffset>(*m_lastCharacterIndex);
    if (m_run.ltr())
	std::swap(isLeft, isRight);

    bool forceLeftExpansion = isLeft && ((m_run.expansionBehavior() &
LeftExpansionMask) == ForceLeftExpansion);
    bool forceRightExpansion = isRight && ((m_run.expansionBehavior() &
RightExpansionMask) == ForceRightExpansion);
    bool forbidLeftExpansion = isLeft && ((m_run.expansionBehavior() &
LeftExpansionMask) == ForbidLeftExpansion);
    bool forbidRightExpansion = isRight && ((m_run.expansionBehavior() &
RightExpansionMask) == ForbidRightExpansion);

Easier to see what’s going on, I think. Maybe we can go even further in
improving this. Might be helpful to put m_run.expansionBehavior() into a local?
Maybe use a structure to pass these four booleans rather than as four separate
function arguments?

> Source/WebCore/platform/graphics/WidthIterator.cpp:341
> +	       static bool expandAroundIdeographs =
FontCascade::canExpandAroundIdeographsInComplexText();

I think here we want to write static const bool to make it clear that we can’t
accidentally change during on one call and have it affect a subsequent call.

> Source/WebCore/platform/graphics/WidthIterator.cpp:342
> +	       bool ideograph = (expandAroundIdeographs &&
FontCascade::isCJKIdeographOrSymbol(character));

We’d want to call this "isIdeograph" because the boolean doesn’t contain an
ideograph. Also I would omit the parentheses here.

> Source/WebCore/platform/graphics/WidthIterator.cpp:405
> +    Vector<Optional<GlyphIndexRange>>
characterIndexToGlyphIndexRange(m_run.length());
> +    Vector<float> advanceWidths(m_run.length(), 0);

Not sure why in the case of Optional we don’t explicitly pass WTF::nullopt but
in the case of float we explicitly pass 0. I don’t think either is required,
but perhaps I am mistaken.

> Source/WebCore/platform/graphics/WidthIterator.cpp:433
> +    for (GlyphBufferStringOffset currentCharacterIndex =
characterStartIndex; currentCharacterIndex <
static_cast<GlyphBufferStringOffset>(characterDestinationIndex);
++currentCharacterIndex) {

auto?

That static_cast here and these long variable names make this super-simple loop
hard to recognize as such. Compare with the loop above using "I".

> Source/WebCore/platform/graphics/WidthIterator.cpp:438
> +	   auto additionalWidth = calculateAdditionalWidth(glyphBuffer,
currentCharacterIndex, glyphIndexRange->leadingGlyphIndex,
glyphIndexRange->trailingGlyphIndex, position);

In this very narrow and clear context I think we can name this local just
"width", making the code easier to read. After all, each structure member
*also* has the word additional. Additional additional.

> Source/WebCore/platform/graphics/WidthIterator.cpp:463
> +void WidthIterator::finalize(GlyphBuffer& glyphBuffer)

I recommend naming the argument just "buffer" given the context is narrow and
clear. Single words for the win.

> Source/WebCore/platform/graphics/WidthIterator.cpp:495
> +    // In general, we have to apply spacing after shaping, because shaping
requires its input to be unperterbed (see
https://bugs.webkit.org/show_bug.cgi?id=215052).

Spelling error here: "unperturbed".

> Source/WebCore/platform/graphics/WidthIterator.cpp:497
> +    {

Why the braces here?

> Source/WebCore/platform/graphics/WidthIterator.h:68
> +    inline bool hasExtraSpacing() const;

Not sure this inline is needed or valuable.

> Source/WebCore/platform/graphics/WidthIterator.h:74
> +	   float leftAdditionalWidth;
> +	   float rightAdditionalWidth;
> +	   float leftExpansionAdditionalWidth;
> +	   float rightExpansionAdditionalWidth;

Could consider leaving the words "additional width" off the names of these data
members since the structure itself is named additional width.