[webkit-reviews] review denied: [Bug 50621] Implement text-combine rendering code : [Attachment 78087] Fixed the build error on win.

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Takumi Takano <takano at apple.com>'s
request for review:
Bug 50621: Implement text-combine rendering code
https://bugs.webkit.org/show_bug.cgi?id=50621

Attachment 78087: Fixed the build error on win.
https://bugs.webkit.org/attachment.cgi?id=78087&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=78087&action=review

Overall this looks really good!  I would like you to refactor things a bit to
avoid the heavy virtualization of RenderText, since I don't think it's really
necessary.

> WebCore/css/CSSStyleSelector.cpp:5639
> +	   m_style->setUnique(); // The style could be modified in
RenderCombineText depending on text metrics.

You only have to setUnique if the value is not "none."

> WebCore/dom/Text.cpp:253
> +    if (style->textCombine() != TextCombineNone)

I would flip this condition, since the common case is for RenderTexts to be
made.

if (style->textCombine() == TextCombineNone)
...
else
...

This might read better with a style->hasTextCombine() helper.

> WebCore/platform/graphics/Font.h:127
> +    FontWidthVariant widthVariant() { return
m_fontDescription.widthVariant(); }

This should be const.

> WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:201
> +	   default:
> +	       featureSelectorValue = TextSpacingProportional;
> +	       break;

Isn't this never reached because you check for RegularWidth above?  It seems
better to ASSERT_NOT_REACHED() here and just return.

> WebCore/rendering/InlineTextBox.cpp:480
> +    textRenderer()->adjustTextOrigin(textOrigin, boxRect);

I'm concerned about the virtual function call overhead here.  Can't we avoid
that by checking if we're on a vertical line or not?

> WebCore/rendering/InlineTextBox.cpp:582
> +    const UChar* characters = textRenderer()->charactersToRender(m_start,
length);

I'm concerned about the virtual function call overhead here.  Can't we avoid
that by checking if we're on a vertical line or not?

> WebCore/rendering/RenderBlockLineLayout.cpp:-1386
> -static inline unsigned textWidth(RenderText* text, unsigned from, unsigned
len, const Font& font, int xPos, bool isFixedPitch, bool collapseWhiteSpace)
> -{
> -    if (isFixedPitch || (!from && len == text->textLength()))
> -	   return text->width(from, len, font, xPos);
> -    return font.width(TextRun(text->characters() + from, len,
!collapseWhiteSpace, xPos));
> -}

There is no need to virtualize width() and it will just slow things down.  Put
this inline helper textWidth back in and just cast and call a function on
RenderCombineText if style->hasTextCombine().  That way you avoid turning width
(a very very hot function) into a virtual function call.

> WebCore/rendering/RenderBlockLineLayout.cpp:1630
> +	       if (style->textCombine() != TextCombineNone) {

As mentioned above, I think a style->hasTextCombine() would read a bit better.

> WebCore/rendering/RenderBlockLineLayout.cpp:1703
> -		       charWidth = textWidth(t, pos, 1, f, w + wrapW,
isFixedPitch, collapseWhiteSpace);
> +		       charWidth = t->width(pos, 1, f, w + wrapW, isFixedPitch,
collapseWhiteSpace);

Let's not virtualize width().  See my comment above.

> WebCore/rendering/RenderBlockLineLayout.cpp:1730
> -			   additionalTmpW = textWidth(t, lastSpace, pos + 1 -
lastSpace, f, w + tmpW, isFixedPitch, collapseWhiteSpace) -
wordTrailingSpaceWidth + lastSpaceWordSpacing;
> +			   additionalTmpW = t->width(lastSpace, pos + 1 -
lastSpace, f, w + tmpW, isFixedPitch, collapseWhiteSpace) -
wordTrailingSpaceWidth + lastSpaceWordSpacing;

Let's not virtualize width().  See my comment above.

> WebCore/rendering/RenderBlockLineLayout.cpp:1732
> -			   additionalTmpW = textWidth(t, lastSpace, pos -
lastSpace, f, w + tmpW, isFixedPitch, collapseWhiteSpace) +
lastSpaceWordSpacing;
> +			   additionalTmpW = t->width(lastSpace, pos -
lastSpace, f, w + tmpW, isFixedPitch, collapseWhiteSpace) +
lastSpaceWordSpacing;

Let's not virtualize width().  See my comment above.

> WebCore/rendering/RenderBlockLineLayout.cpp:1749
> -			       int charWidth = textWidth(t, pos, 1, f, w +
tmpW, isFixedPitch, collapseWhiteSpace) + (applyWordSpacing ? wordSpacing : 0);

> +			       int charWidth = t->width(pos, 1, f, w + tmpW,
isFixedPitch, collapseWhiteSpace) + (applyWordSpacing ? wordSpacing : 0);

Let's not virtualize width().  See my comment above.

> WebCore/rendering/RenderBlockLineLayout.cpp:1878
> -	       int additionalTmpW = ignoringSpaces ? 0 : textWidth(t,
lastSpace, pos - lastSpace, f, w + tmpW, isFixedPitch, collapseWhiteSpace) +
lastSpaceWordSpacing;
> +	       int additionalTmpW = ignoringSpaces ? 0 : t->width(lastSpace,
pos - lastSpace, f, w + tmpW, isFixedPitch, collapseWhiteSpace) +
lastSpaceWordSpacing;

Let's not virtualize width().  See my comment above.

> WebCore/rendering/RenderCombineText.cpp:107
> +    if (!style()->isHorizontalWritingMode()) {

Why not invert this if?  If you're in horizontal writing modes just bail.  That
way the 30 lines of code that follow don't have to be indented.

> WebCore/rendering/RenderText.cpp:1247
>  
> +unsigned RenderText::width(unsigned from, unsigned len, const Font& f, int
xPos, bool isFixedPitch, bool collapseWhiteSpace) const
> +{
> +    if (isFixedPitch || (!from && len == textLength()))
> +	   return width(from, len, f, xPos);
> +    return f.width(TextRun(characters() + from, len, !collapseWhiteSpace,
xPos));
> +}

I don't think this virtualization was necessary.  See RenderBlockLineLayout
comments.

> WebCore/rendering/RenderText.cpp:1520
> +void RenderText::adjustTextOrigin(IntPoint&, IntRect) const
> +{
> +}
> +
> +const UChar* RenderText::charactersToRender(int start, int&) const
> +{
> +    return text()->characters() + start;
> +}

I don't think this virtualization was necessary.  See InlineTextBox comments.