[Webkit-unassigned] [Bug 50621] Implement text-combine rendering code

Tue Dec 14 10:39:56 PST 2010

https://bugs.webkit.org/show_bug.cgi?id=50621

mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #76517|review?                     |review-
               Flag|                            |

--- Comment #17 from mitz at webkit.org  2010-12-14 10:39:56 PST ---
(From update of attachment 76517)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.