[webkit-reviews] review granted: [Bug 114949] Initial advance of text runs should be taken into account : [Attachment 199033] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 22 08:40:11 PDT 2013


Darin Adler <darin at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 114949: Initial advance of text runs should be taken into account
https://bugs.webkit.org/show_bug.cgi?id=114949

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

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


> Source/WebCore/ChangeLog:10
> +	   Keep track of complex text runs' initial advance by adding a new
> +	   member to GlyphBuffer and adding the initial advance of any
non-first
> +	   text run to the advance of the last glyph added for the previous
text run.

Why doesn’t this mention the newly added support for advances with Y-axis
changes? Isn’t that a separate thing we’re adding?

> Source/WebCore/platform/graphics/FontFastPath.cpp:482
> +    FloatPoint startPoint(point.x(), point.y() -
glyphBuffer.initialAdvance().height());
> +    float nextX = startPoint.x() + glyphBuffer.advanceAt(0).width();
> +    float nextY = startPoint.y() + glyphBuffer.advanceAt(0).height();

Now that the other advances are being done both X and Y, why is the initial
advance adjustment being done only for Y here?

> Source/WebCore/platform/graphics/GlyphBuffer.h:130
> -    float advanceAt(int index) const
> +    GlyphBufferAdvance advanceAt(int index) const

We might get slightly better performance if the return type here is const
GlyphBufferAdvance&.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:504
> +	   if (glyphBuffer && glyphBuffer->isEmpty())
> +	       glyphBuffer->setInitialAdvance(complexTextRun.initialAdvance());


The logic here seems a little oblique. If there are complex text runs that
don’t add a full glyph to the buffer, then the first run that does will set the
initial advance. That seems a little strange. Maybe never happens. Would be
nice to do this in a more direct way, but not sure that’s practical.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:576
> +	   if (r > 0 && m_adjustedAdvances.size() > 0) {
> +	       CGSize previousAdvance = m_adjustedAdvances.last();
> +	       previousAdvance.width += complexTextRun.initialAdvance().width;
> +	       previousAdvance.height -=
complexTextRun.initialAdvance().height;
> +	       m_adjustedAdvances[m_adjustedAdvances.size() - 1] =
previousAdvance;
> +	   }

This needs a comment. It’s not clear why this is the right thing to do. The
change log comment explains it, but people won’t necessarily be reading the
change log.

Also, normally we’d leave off those “> 0” in the conditional expression.

> Source/WebCore/platform/graphics/mac/ComplexTextController.h:119
> +	   CGSize m_initialAdvance;

The missing glyphs run constructor does not initialize this. I don’t think it’s
safe to leave it with an uninitialized value.

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:115
> +    m_initialAdvance = wkCTRunGetInitialAdvance(ctRun);

Would be better to use constructor syntax for this rather than assignment.

> LayoutTests/ChangeLog:9
> +	   * fast/text/complex-initial-advance-expected.html: Added.
> +	   * fast/text/complex-initial-advance.html: Added.

Do we need a fast path initial advance test case, too? If not, then why the
code changes to the fast path? I think they’re untested.


More information about the webkit-reviews mailing list