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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from mitz at webkit.org  2010-12-07 09:48:39 PST ---
(From update of attachment 75794)
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”

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