[webkit-reviews] review granted: [Bug 128145] REGRESSION: missing underline under CJK text : [Attachment 232515] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 128145: REGRESSION: missing underline under CJK text
https://bugs.webkit.org/show_bug.cgi?id=128145

Attachment 232515: Patch
https://bugs.webkit.org/attachment.cgi?id=232515&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232515&action=review


> Source/WebCore/platform/graphics/Font.cpp:1057
> +bool advanceByConsumingBaseCharacterInSequence(const UChar*& iterator, const
UChar* end, UChar32& baseCharacter)

We already have U16_NEXT, so I don’t think we need this.

> Source/WebCore/platform/graphics/Font.cpp:1076
> +GlyphToPathTranslator::GlyphUnderlineType sharedUnderlineType(const TextRun&
textRun, const GlyphBuffer& glyphBuffer, int index)

Not sure I understand the meaning of the word “shared” here.

> Source/WebCore/platform/graphics/Font.cpp:1079
> +    // We want to SkipDescenders in general. However, skipping descenders on
CJK characters leads to undesirable renderings,
> +    // so we want to draw through CJK characters (on a
character-by-character basis).

Please say “skip descenders” rather than SkipDescenders.

> Source/WebCore/platform/graphics/Font.cpp:1086
> +	   if (!advanceByConsumingBaseCharacterInSequence(characterPointer,
textRun.characters16() + textRun.length(), baseCharacter))
> +	       return GlyphToPathTranslator::DrawOverGlyph;

Normally we just use U16_NEXT to implement something like this. We’d write:

    unsigned offset = glyphBuffer.offsetInString(index);
    U16_NEXT(textRun.characters16(), offset, textRun.length(), baseCharacter);

> Source/WebCore/platform/graphics/Font.cpp:1091
> +    // FIXME: we may need to cache the output of this ICU call.
https://bugs.webkit.org/show_bug.cgi?id=133529

This seems like a strange comment; makes me feel fear but doesn’t give me
information to know if I should be afraid or not. Did you do performance
testing yet?

> Source/WebCore/platform/graphics/Font.h:103
> +    enum GlyphUnderlineType {
> +	   SkipDescenders,
> +	   SkipGlyph,
> +	   DrawOverGlyph
> +    };

This would read nicely all on one line.

For most new enums we are using enum class.

> Source/WebCore/platform/graphics/Font.h:108
> +    virtual void increment() = 0;

I think advance would be a clearer name for this than increment.

> Source/WebCore/platform/graphics/Font.h:111
> +GlyphToPathTranslator::GlyphUnderlineType sharedUnderlineType(const
TextRun&, const GlyphBuffer&, int index);

Is int the right type for the index? Why not unsigned?

As I asked above, what does “shared” mean in this function name?

> Source/WebCore/platform/graphics/Font.h:359
> +bool advanceByConsumingBaseCharacterInSequence(const UChar*& iterator, const
UChar* end, UChar32& baseCharacter);

If you did want to put this in a header, it definitely wouldn’t be Font.h --
this is just a basic UTF-16 helper function. But luckily, we don’t need this
because ICU already has a function for this.

> Source/WebCore/platform/graphics/GlyphBuffer.h:127
> +    void add(Glyph glyph, const SimpleFontData* font, float width, int
offsetInString = -1, const FloatSize* offset = 0)

What does -1 mean here?

> Source/WebCore/platform/graphics/GlyphBuffer.h:198
> +	   ASSERT(m_offsetsInString.get());

I’m surprised we need the get() here.

> Source/WebCore/platform/graphics/WidthIterator.cpp:175
> +	   int currentCharacter = textIterator.currentCharacter();

Is int the right type for this?

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:264
> -
> +    

You shouldn’t land this whitespace change.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:272
> +    if (!advanceByConsumingBaseCharacterInSequence(iterator, end,
baseCharacter))
> +	   return false;

Here’s how to write this using U16_NEXT:

    unsigned i = 0;
    U16_NEXT(iterator, i, end - iterator, baseCharacter);
    if (U_IS_SURROGATE(baseCharacter))
	return false;
    iterator += i;

We can possibly even leave out the U_IS_SURROGATE check.

> Source/WebCore/platform/graphics/mac/FontMac.mm:521
> +    glyphBuffer.saveOffsetsInString();

What guarantees that everyone adding to this glyph buffer will add an offset,
rather than passing in -1?

> Source/WebCore/platform/graphics/win/UniscribeController.cpp:362
> +	       glyphBuffer->add(glyph, fontData, advance, -1, &size);

This magic -1 is unclear.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:118
> +    AffineTransform getCurrentTransform();

This should be named currentTransform() to follow WebKit naming conventions. Or
just transform(). After all, we call it path(), not currentPath().

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:120
> +    const TextRun* m_textRun;

This should be const TextRun* const, I think; m_stoppingPoint, m_scale, and
m_isVerticalText are all marked const, so why not this as well?

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:156
> +    

Should not add this whitespace here.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:278
> +    auto translator(createGlyphToPathTranslator(*fontData, nullptr,
glyphBuffer, from, numGlyphs, point));

I think this would read better with = than with () syntax:

    auto translator = createGlyphToPathTranslator(*fontData, nullptr,
glyphBuffer, from, numGlyphs, point);

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:279
>      while (translator->containsMorePaths()) {

This looks like it should be a for loop. Even if the initialization of the
translator is too long to put inside, I think this is better than the while:

    for (; translator->containsMorePaths(); translator->increment()) {