[webkit-reviews] review denied: [Bug 50621] Implement text-combine rendering code : [Attachment 80323] Fixed a patch conflict on LayoutTests/ChangeLog.

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 80323: Fixed a patch conflict on LayoutTests/ChangeLog.
https://bugs.webkit.org/attachment.cgi?id=80323&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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 (knoll at kde.org)
> + * (C) 2000 Dirk Mueller (mueller at kde.org)
> + * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
> + * Copyright (C) 2006 Andrew Wellington (proton at wiretapped.net)
> + * Copyright (C) 2006 Graham Dennis (graham.dennis at gmail.com)

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?