Bug 50621 – Implement text-combine rendering code

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Takumi Takano]

Implement actual text-combine rendering code. A subtask of Bug 48538 : Support the text-combine CSS property.

[Takumi Takano]

Created attachment 75794 [details]
Proposed patch. Code review purpose only - no test, no non-Mac platform support yet.

[WebKit Review Bot]

Attachment 75794 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75794 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 75794 [details]
Proposed patch. Code review purpose only - no test, no non-Mac platform support yet.

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

Haven’t read through the entire patch yet, but here are some comments:

> WebCore/platform/graphics/Font.cpp:195
> +FontDescription Font::bestFontForTextCombine(const TextRun& run, bool* succeededToFit, float* combinedTextWidth) const

The second and third parameters should be references, not pointers.
There are multiple naming issues here. I think “best” is unnecessary. It is also strange that the method is called “bestFont” but it returns a FontDescription. “succeededToFit” does not fit with WebKit naming style. Perhaps “canCombine” is a better name.

> WebCore/platform/graphics/Font.cpp:197
> +    FontDescription bestOne = m_fontDescription;

“bestOne” is a weak name, especially since this method doesn’t involve explicitly comparing FontDescriptions and finding an optimum. I would move the definition of this variable further down (and use m_fontDescription in the first part of the method) and rename it to something like compressedFontDescription.

> WebCore/platform/graphics/Font.cpp:202
> +    if (!*succeededToFit) {

Please reverse the condition and return early if it’s true.

> WebCore/platform/graphics/Font.cpp:204
> +        static const CompressedWidthType widthTypes[] = {CompressedWidthHalf, CompressedWidthThird, CompressedWidthQuarter};

There should be spaces after the { and before the }.

> WebCore/platform/graphics/Font.cpp:205
> +        for (unsigned i = 0 ; i < sizeof(widthTypes) / sizeof(CompressedWidthType) ; ++i) {

Please use WTF_ARRAY_LENGTH. size_t is more appropriate than unsigned here.

> WebCore/platform/graphics/Font.cpp:207
> +            Font tempFont = Font(bestOne, m_letterSpacing, m_wordSpacing);

A better name would be “compressedFont”.

> WebCore/platform/graphics/Font.cpp:213
> +                break;

You can just return here.

> WebCore/platform/graphics/Font.h:58
> +const float textCombineMargin = 1.1; // Allow em + 10% margin

Any reason why this is in the header?

> WebCore/platform/graphics/FontDescription.h:51
> +enum CompressedWidthType { CompressedWidthNone, CompressedWidthHalf, CompressedWidthThird, CompressedWidthQuarter };

Can this be called “Compression” or “CompressedWidth”? Not sure what the significance of “type” is here.

> WebCore/platform/graphics/FontFastPath.cpp:89
> +                        // Shouldn't be possible to even reach this point.
> +                        ASSERT_NOT_REACHED();

The comment is redundant. It would be better to have two assertions instead of that one: ASSERT(compressedWidthPage) before the if (compressedWidthPage) and ASSERT(data.fontData) before the if (data.fontData).

> WebCore/platform/graphics/SimpleFontData.cpp:64
> +    , m_compressedHalfWidthFontData(0)
> +    , m_compressedThirdWidthFontData(0)
> +    , m_compressedQuarterWidthFontData(0)

I think we should consider rolling the small caps font, the broken ideographic font, the compressed fonts, and the (future) emphasis mark font into a lazily-allocated structure.

> WebCore/platform/graphics/SimpleFontData.cpp:86
>      , m_smallCapsFontData(0)
> +    , m_compressedHalfWidthFontData(0)
> +    , m_compressedThirdWidthFontData(0)
> +    , m_compressedQuarterWidthFontData(0)
>      , m_brokenIdeographFontData(0)

We should also make these OwnPtr<>s.

> WebCore/platform/graphics/cocoa/FontPlatformData.h:60
> +enum TextSpacingFeatureSelector { TextSpacingProportional, TextSpacingFullWidth, TextSpacingHalfWidth, TextSpacingThirdWidth, TextSpacingQuarterWidth };

I don’t understand the meaning of “Feature” and “Selector” in this name.

> WebCore/platform/graphics/cocoa/FontPlatformData.h:115
> +        uintptr_t hashCodes[3] = { (uintptr_t)m_font, (uintptr_t)m_textSpacingSelector, m_orientation << 2 | m_syntheticBold << 1 | m_syntheticOblique };

Did you really mean to cast to unitptr_t?

> WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:179
> +    } else {

WebKit style is to avoid else after return.

> WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:193
> +                m_CTFont.adoptCF(sourceFont.get());

I am not sure why you are adopting here. sourceFont is going to release the font as it goes out of scope, so who will keep it around? I think you meant to write m_CTFont = sourceFont.

> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:51
> +    bool isVertical = isCompressedWidthFont() ? false : orientation() == Vertical;

Another way to write this would be
bool isVertical = !isCompressedWidthFont() && orientation() == Vertical;

> WebCore/platform/graphics/mac/SimpleFontDataMac.mm:443
> +    default:

It’s best not to use default:, so that the compiler can warn us in the future if new enum values are added.

> WebCore/rendering/InlineTextBox.cpp:469
> +        textOrigin.move(boxRect.width() / 2 - ceil(textRenderer()->combinedTextWidth()) / 2, font.pixelSize());

I think you meant to use ceilf().

> WebCore/rendering/RenderText.cpp:102
> +     , m_combinedTextWidth(0.0)

Redundant “.0”

[Build Bot]

Attachment 75794 [details] did not build on win:
Build output: http://queues.webkit.org/results/6783075

[WebKit Review Bot]

Attachment 75794 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75794 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6877014

[WebKit Review Bot]

Attachment 75794 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75794 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Takumi Takano]

Created attachment 75894 [details]
Incorporated feedback from mitz. Still not for commit - no test added yet.

[WebKit Review Bot]

Attachment 75894 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6751127

[Build Bot]

Attachment 75894 [details] did not build on win:
Build output: http://queues.webkit.org/results/6727124

[Takumi Takano]

Created attachment 76045 [details]
Added a test. Added stubs for non-Mac platforms to avoid build errors.

[WebKit Review Bot]

Attachment 75894 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6953054

[WebKit Review Bot]

Attachment 76045 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6975044

[Takumi Takano]

Created attachment 76517 [details]
Fixed a build error on gtk and a conflict with r74005

Comment on attachment 76517 [details]
Fixed a build error on gtk and a conflict with r74005

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

There are a few unusual things about your approach. I am not sure doing all of this at style update time is the best way. It’s the sort of thing that fits more naturally into layout. I am also concerned about the creation of separate Font objects. Perhaps it would be better to give Font the ability to compress.

I would like to hear what Hyatt thinks about this, too.

> WebCore/ChangeLog:18
> +        * css/CSSParser.cpp:
> +        - text-combine syntax has been changed in the proposal.
> +        (WebCore::CSSParser::parseValue):
> +        * css/CSSPrimitiveValueMappings.h:
> +        - text-combine syntax has been changed in the proposal.
> +        (WebCore::CSSPrimitiveValue::CSSPrimitiveValue):
> +        (WebCore::CSSPrimitiveValue::operator TextCombine):
> +        * css/CSSValueKeywords.in:
> +        - text-combine syntax has been changed in the proposal.

Please split the CSS part (and related test change) into a separate patch.

> WebCore/platform/graphics/Font.cpp:212
> +        compressedFont.update(fontSelector());

It’s really scary that this function constructs and initializes Font objects all the time. We may need to consider a different design here.

> WebCore/platform/graphics/FontFastPath.cpp:82
> +                        ASSERT(compressedWidthPage);

I know I suggested this assertion, but asserting that a pointer is non-null just prior to dereferencing it is actually not very helpful.

> WebCore/platform/graphics/FontFastPath.cpp:84
> +                        ASSERT(data.fontData);

Why is this assertion correct?

> WebCore/platform/graphics/SimpleFontData.cpp:224
> +    m_smallCapsFontData = 0;
> +    m_compressedHalfWidthFontData = 0;
> +    m_compressedThirdWidthFontData = 0;
> +    m_compressedQuarterWidthFontData = 0;
> +    m_brokenIdeographFontData = 0;

Why is this necessary?

> WebCore/platform/graphics/SimpleFontData.h:80
> +    SimpleFontData* compressedWidthFontData(const FontDescription& fontDescription) const;

Please omit the parameter name here.

> WebCore/platform/graphics/SimpleFontData.h:233
> +        void unregisterFromGlyphPage();

This is not a very good name for this method. It doesn’t even say anything about custom fonts! Perhaps pruneGlyphPageTree(). You might want to add a boolean to DerivedFontData saying whether the base font is a custom font, then you could call pruneGlyphPageTree() unconditionally and it will test for the custom font boolean and only prune if needed.

> WebCore/platform/graphics/SimpleFontData.h:239
> +        OwnPtr <SimpleFontData> m_smallCapsFontData;
> +        OwnPtr <SimpleFontData> m_compressedHalfWidthFontData;
> +        OwnPtr <SimpleFontData> m_compressedThirdWidthFontData;
> +        OwnPtr <SimpleFontData> m_compressedQuarterWidthFontData;
> +        OwnPtr <SimpleFontData> m_brokenIdeographFontData;

I think WebKit style is to not use the m_ prefix for struct members.

> WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:175
> -    if (m_font)
> -        return toCTFontRef(m_font);
> -    if (!m_CTFont)
> -        m_CTFont.adoptCF(CTFontCreateWithGraphicsFont(m_cgFont.get(), m_size, 0, 0));
> +    if (m_textSpacingSelector == TextSpacingProportional) {
> +        if (m_font)
> +            return toCTFontRef(m_font);

Can we have the font cache return a compressed-width font so that we don’t have to derive it here?

> WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:187
> +        RetainPtr<CFNumberRef> featureSelector(AdoptCF, CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &featureSelectorValue));

Passing a WebCore enum value to Core Text is fragile. Why not pass Core Text constants?

> WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:190
> +        CGAffineTransform matrix = CTFontGetMatrix(sourceFont.get());
> +        CTFontRef newFont = CTFontCreateWithFontDescriptor(newDescriptor.get(), m_size, &matrix);

We create sourceFont with a null matrix. Can’t we just use a null matrix when creating newFont?

> WebCore/rendering/RenderText.cpp:180
> +    if (style()->textCombine() != TextCombineNone)
> +        prepareTextCombine();

Is this the only place this needs to be done? What if the text changes?

> WebCore/rendering/RenderText.cpp:1520
> +    if (!style()->isHorizontalWritingMode() && style()->textCombine() == TextCombineHorizontal) {

Please reverse this condition and return early.

> WebCore/rendering/RenderText.cpp:1528
> +            if (style()->setFontDescription(fontForTextCombine))
> +                style()->font().update(style()->font().fontSelector());

What guarantees that this style is not shared with another element?

> WebCore/rendering/RenderText.cpp:1533
> +            const UChar uc = objectReplacementCharacter;
> +            setTextInternal(String(&uc, 1).impl());

Instead of constructing a String every time, you can use a static String.

> WebCore/rendering/RenderText.h:128
> +    void prepareTextCombine();

Can this be private or at least protected?

> WebCore/rendering/RenderText.h:172
> +    float m_combinedTextWidth;

Seems like a waste to ass this member to every RenderText instance when it’s only going to be so rarely used. One way to avoid this would be to use an external global map. However, I think perhaps a better design for this feature would involve subclassing RenderText.

> WebCore/rendering/RenderText.h:187
> +    bool m_isCombinedText : 1;

Is this member necessary? Wouls it be too slow to check the font?

> LayoutTests/fast/text/international/text-combine-image-test.html:26
> +<span id="Hira24"><span id="combine">ON</span>=西æ¦<span id="combine">2010</span>å¹´<span id="combine">1</span>æ<span id="combine">20</span>æ¥<span id="combine">365</span>å</span>

Please test the case where the text can’t combine, too.

[Dave Hyatt]

Comment on attachment 76517 [details]
Fixed a build error on gtk and a conflict with r74005

It seems like a RenderText subclass, RenderCombinedText, would let you encapsulate some logic and possibly cache information without adding code to RenderText.

If you're going to mutate the RenderStyle of the RenderText, then you need to make sure the style is unique.  You can turn off style sharing when the text-combine property is specified.  (setIsUnique I think.)

I really dislike creating throwaway Fonts in fontDescriptionForTextCombine, but I'm not sure how to fix it without having to patch FontPlatformData.

[Dave Hyatt]

I think what you should be doing here is just pretending like compression could be specified in CSS and modeling it that way.   Think of it as being like style (italic) or weight (bold).  Imagine a hypothetical font-compression property where you could specify the values (normal, 1/2, 1/3, 1/4), and pretend like you're implementing it (minus the actual property definition itself).

Adding compression to the font description is good.  Keep that, and then make compression something you can then select fonts on, so that means making it part of the FontPlatformData, etc.

Don't make compression part of the derived data.  (I still think the derived data idea is a good one, even with just the small caps and broken ideographic font members, but it could be a separate patch.)  Derived data is about selectively falling back based off characters, and I don't think that's what you need here.  I think compression is simpler and can be more like style or weight (we might even be able to synthesize it like we can with bold/italic).

Once you do that, you have the ability to ask for any compressed font from the cache.  I think you could then make a RenderCombinedText subclass that does the font adjustment by asking for the right font and setting it on the RenderStyle.

[Takumi Takano]

Created attachment 76653 [details]
New patch. Code review purpose only - no non-Mac platform support yet.

Could you please check if I understand your comments correctly or not...

[WebKit Review Bot]

Attachment 76653 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7074035

[WebKit Review Bot]

Attachment 76653 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7077032

[Early Warning System Bot]

Attachment 76653 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7072033

[Build Bot]

Attachment 76653 [details] did not build on win:
Build output: http://queues.webkit.org/results/7096025

[Takumi Takano]

Created attachment 76738 [details]
New patch with updated test and stubs for non-Mac platforms

[Early Warning System Bot]

Attachment 76738 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7193061

[WebKit Review Bot]

Attachment 76738 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7194038

[Build Bot]

Attachment 76738 [details] did not build on win:
Build output: http://queues.webkit.org/results/7088058

[Takumi Takano]

Created attachment 76969 [details]
Resolved a conflict with TOT (r74328).

[Early Warning System Bot]

Attachment 76969 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7246055

[WebKit Review Bot]

Attachment 76969 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7226054

[Build Bot]

Attachment 76969 [details] did not build on win:
Build output: http://queues.webkit.org/results/7210053

[Darin Adler]

Comment on attachment 76969 [details]
Resolved a conflict with TOT (r74328).

The early warning system shows this patch breaking the build for every platform it’s able to build except for GTK. Setting review- so we can make a version that builds. Someone else will also need to review the substance of the patch.

[Takumi Takano]

Created attachment 77966 [details]
Attempt to fix build errors on non-Mac platforms

Tried to add new files to non-Mac platforms' build setting.

[Build Bot]

Attachment 77966 [details] did not build on win:
Build output: http://queues.webkit.org/results/7347037

[Takumi Takano]

Created attachment 78087 [details]
Fixed the build error on win.

[Dave Hyatt]

Comment on attachment 78087 [details]
Fixed the build error on win.

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.

[Takumi Takano]

Created attachment 79561 [details]
Incorporated Dave's feedback.

And also dealt with the recent repository layout change.

[Takumi Takano]

Created attachment 79565 [details]
Fixed a conflict in WebCore.xcodeproj.

[Dave Hyatt]

Comment on attachment 79565 [details]
Fixed a conflict in WebCore.xcodeproj.

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

widthFromCache can be de-virtualized, so do that also.  You can get rid of rotateWhenVertical completely with the changes I mention following this comment.

> Source/WebCore/rendering/InlineTextBox.cpp:492
> +    bool doRotation = !isHorizontal() && textRenderer()->rotateWhenVertical();
> +    if (doRotation) {

I'd suggest using styleToUse->hasTextCombine() to store RenderCombineText* combinedText in a local variable.  Then you can just make bool doRotation = !isHorizontal && combinedText;

> Source/WebCore/rendering/InlineTextBox.cpp:508
> +    if (styleToUse->hasTextCombine()) {

Can use the local I mentioned above:

if (combinedText)
    combinedText->adjustTextOrigin(textOrigin, boxRect();

> Source/WebCore/rendering/InlineTextBox.cpp:618
> +    if (!styleToUse->hasTextCombine())
> +        characters = textRenderer()->text()->characters() + m_start;
> +    else {
> +        RenderCombineText* t = toRenderCombineText(textRenderer());
> +        characters = t->charactersToRender(m_start, length);

Can use the local from above:

const UChar* characters = !combinedText ? textRenderer()->text()->characters() + m_start : combinedText->charactersToRender(m_start, length);

[Dave Hyatt]

(In reply to comment #40)

> I'd suggest using styleToUse->hasTextCombine() to store RenderCombineText* combinedText in a local variable.  Then you can just make bool doRotation = !isHorizontal && combinedText;
> 

Sorry, messed this up.  I meant:

bool doRotation = !isHorizontal && (!combinedText || !combinedText->isCombined());

[Takumi Takano]

Created attachment 79848 [details]
New patch with Dave's input.

[Dave Hyatt]

Comment on attachment 79848 [details]
New patch with Dave's input.

I don't get how your RenderCombinedText::widthFromCache method ever gets called.  RenderText isn't going to invoke the right one since it's not virtual.  You need to patch RenderText::widthFromCache to ask if you're combined and if so call RenderCombinedText's widthFromCache method after casting it.  (And then make sure RenderCombinedText doesn't turn around and call RenderText::widthFromCache any longer.)

[Dave Hyatt]

It seems like you need a test case with a float or positioned object that contains a run of text that has some text-combine involved in order to properly test widthFromCache.  I'd like to see that test case added.

Try making the test case before changing your code and you should be able to make a float in a vertical text environment that ends up being too tall (because it doesn't measure the text-combine portion of the text correctly).

[Dave Hyatt]

(In reply to comment #44)
> It seems like you need a test case with a float or positioned object that contains a run of text that has some text-combine involved in order to properly test widthFromCache.  I'd like to see that test case added.
> 
> Try making the test case before changing your code and you should be able to make a float in a vertical text environment that ends up being too tall (because it doesn't measure the text-combine portion of the text correctly).

Or is it too small rather than too tall?  I'm assuming the measurement may end up being slightly different than the square you're fitting to.

[Takumi Takano]

Created attachment 80294 [details]
Another new patch with Dave's input

Correct, RenderCombineText::widthFromCache was never invoked. My bad... When I made things correct, I noticed widthFromCache is called before RenderCombineText::prepareTextCombine is called. I added prepareTextCombine call in RenderBlock::computeInlinePreferredLogicalWidths() as well. Also I added a float:right block that contains combined text in my test html.

[Darin Adler]

Comment on attachment 80294 [details]
Another new patch with Dave's input

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

> LayoutTests/ChangeLog:-2858
> -2011-01-22  Nikolas Zimmermann  <[e-mail]>
> -
> -        Not reviewed.

This patch replaces someone else’s change log entry with a copy of yours. That needs to be fixed.

[Takumi Takano]

Created attachment 80323 [details]
Fixed a patch conflict on LayoutTests/ChangeLog.

[Darin Adler]

Comment on attachment 80323 [details]
Fixed a patch conflict on LayoutTests/ChangeLog.

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

I’m going to say review- because you’ll probably want to make at least one of the fixes I suggest. Generally looks good to me!

> Source/WebCore/css/CSSFontFaceSource.cpp:118
> -    unsigned hashKey = (fontDescription.computedPixelSize() + 1) << 3 | (fontDescription.orientation() == Vertical ? 4 : 0) | (syntheticBold ? 2 : 0) | (syntheticItalic ? 1 : 0);
> +    unsigned hashKey = (fontDescription.computedPixelSize() + 1) << 16 | fontDescription.widthVariant() << 8 | (fontDescription.orientation() == Vertical ? 4 : 0) | (syntheticBold ? 2 : 0) | (syntheticItalic ? 1 : 0);

Why shift the width variant by 8? I think it should be shifted by 3 as the size was before.

Shifting by 16 bits leaves 8 bits for the width variant, but that has only four values and so needs only 2 bits. I suggest shifting by 5 and by 3 rather than by 16 and by 8.

> Source/WebCore/dom/Text.cpp:256
> +    if (!style->hasTextCombine())
> +        return new (arena) RenderText(this, dataImpl());
> +
> +    return new (arena) RenderCombineText(this, dataImpl());

The logic should go the other way around. We put unusual cases inside the ifs and normal ones in the main flow.

> Source/WebCore/platform/graphics/FontWidthVariant.h:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

It’s 2011 now.

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:33
> +// These CoreText Text Spacing feature selectors are not defined in CoreText...

There’s no reason to use an ellipsis.

Did you request that these selectors be put in a CoreText header in the future?

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:202
> +        switch(m_widthVariant) {
> +        case HalfWidth:
> +            featureSelectorValue = TextSpacingHalfWidth;
> +            break;
> +        case ThirdWidth:
> +            featureSelectorValue = TextSpacingThirdWidth;
> +            break;
> +        case QuarterWidth:
> +            featureSelectorValue = TextSpacingQuarterWidth;
> +            break;
> +        default:
> +            ASSERT_NOT_REACHED();
> +            return 0;
> +        }

I suggest putting this conversion into a separate inline function. It breaks up the flow a bit too much to do it here inline.

Also, the separate function could use the style where we leave out the default. That makes the compiler warn if we forget to handle an enum value.

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:208
> +        CTFontRef newFont = CTFontCreateWithFontDescriptor(newDescriptor.get(), m_size, 0);
> +        if (newFont)
> +            m_CTFont.adoptCF(newFont);

If would be better to put the value directly into a RetainPtr<CTFontRef> since there’s no significant performance value to doing it that way.

> Source/WebCore/rendering/InlineTextBox.cpp:491
> +    bool doRotation = !isHorizontal() && (!combinedText || !combinedText->isCombined());

We normally try to avoid verb phrases in boolean variable names. For example, this should be shouldRotate rather than doRotation.

> Source/WebCore/rendering/RenderCombineText.cpp:6
> + * (C) 1999 Lars Knoll ([e-mail])
> + * (C) 2000 Dirk Mueller ([e-mail])
> + * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
> + * Copyright (C) 2006 Andrew Wellington ([e-mail])
> + * Copyright (C) 2006 Graham Dennis ([e-mail])

I’m not sure you need ever last one of these lines, but I suppose it’s hard to figure out what not to copy.

This also needs 2011 on the Apple line, since presumably you have new code here.

> Source/WebCore/rendering/RenderCombineText.cpp:34
> +RenderCombineText::RenderCombineText(Node* node, PassRefPtr<StringImpl> str)

Please use a full word, such as string rather than an abbreviation, str, for this variable name.

> Source/WebCore/rendering/RenderCombineText.cpp:38
> +     , m_fontNeedsUpdated(false)

This should be m_needsFontUpdate or m_fontNeedsUpdate or m_fontNeedsUpdating. The grammar is wrong here.

> Source/WebCore/rendering/RenderCombineText.cpp:56
> +unsigned RenderCombineText::width(unsigned from, unsigned len, const Font& f, int xPos, HashSet<const SimpleFontData*>* fallbackFonts, GlyphOverflow* glyphOverflow) const

Could you use better names for these than the other files did? At the very least you could use length and font instead of len and f.

> Source/WebCore/rendering/RenderCombineText.cpp:67
> +void RenderCombineText::adjustTextOrigin(IntPoint& textOrigin, IntRect boxRect) const

I think we may want to take a const IntRect& here instead of an IntRect.

> Source/WebCore/rendering/RenderCombineText.cpp:93
> +    // Spec says text-combine works only in vertical writing mode.
> +    if (style()->isHorizontalWritingMode())
> +        return;

This should be handled in Text::createRenderer instead. That code is using the style to decide what kind of renderer to create and can implement this rule. Why make a RenderCombineText and then decide all the way at this point not to do anything?!

> Source/WebCore/rendering/RenderCombineText.cpp:123
> +        static const UChar uc = objectReplacementCharacter;

Please use a short word or word phrase rather than the acronym uc here. It could just be character.

> Source/WebCore/rendering/RenderCombineText.h:34
> +    virtual unsigned width(unsigned from, unsigned len, const Font&, int xPos, HashSet<const SimpleFontData*>* fallbackFonts = 0, GlyphOverflow* = 0) const;

This override of a virtual function should be private instead of public. Nobody is calling it directly.

> Source/WebCore/rendering/RenderCombineText.h:36
> +    void prepareTextCombine();

This is an awkward function. It’s hard to know when to call it given its name.

> Source/WebCore/rendering/RenderCombineText.h:38
> +    const UChar* charactersToRender(int start, int& length) const;

This function should have get in its name since it has two return values, not just one. I also think it might be better if it had two out parameters instead of a return value plus an out parameter.

> Source/WebCore/rendering/RenderCombineText.h:40
> +    ALWAYS_INLINE int widthFromCache(const Font& f) const { return f.size(); }

This function seems pointless. Why is it a member function? Why not call size on the font directly at each call site?

[Dave Hyatt]

Darin Adler wrote:
> > Source/WebCore/rendering/RenderCombineText.cpp:93
> > +    // Spec says text-combine works only in vertical writing mode.
> > +    if (style()->isHorizontalWritingMode())
> > +        return;
> 
> This should be handled in Text::createRenderer instead. That code is using the style to decide what kind > of renderer to create and can implement this rule. Why make a RenderCombineText and then decide all > the way at this point not to do anything?!

I disagree with this point.  It's better to not create/destroy renderers just because the orientation changed. This is something people might do (dynamically switch back and forth), and I don't think we should be destroying the text-combine renderers when this happens.  It's easier to just have the renderer lie dormant until it detects it is in a vertical context.

[Takumi Takano]

Created attachment 80412 [details]
Incorporated Darin's feedback except the one that Dave objected.

[Early Warning System Bot]

Attachment 80412 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7513382

[Takumi Takano]

Created attachment 80431 [details]
Fixed a build error on qt.

[Takumi Takano]

Created attachment 80452 [details]
Found a bug on reloading a page.

When a page with combined text is reloaded, RenderCombineText objects previously prepared are reused. We need to restore the original text before we call combineText() on the reused object, otherwise the combineText() uses U+FFFC instead of the actual text for text width calculation. Added text restoration code in RenderCombineText::styleDidChange().

[Dave Hyatt]

Comment on attachment 80452 [details]
Found a bug on reloading a page.

r=me

[Dave Hyatt]

Landed in r77153.