[Webkit-unassigned] [Bug 72095] [chromium] Font::drawComplexText can not draw a segment of text run

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Kenichi Ishibashi <bashi at chromium.org>  2011-11-14 02:01:39 PST ---
(From update of attachment 114642)
View in context: https://bugs.webkit.org/attachment.cgi?id=114642&action=review

Thanks for the patch. It looks good to me. Some minor comments follows.

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:109
> +    const unsigned charStartPos() const { return m_item.item.pos; }

nit: WebKit doesn't prefer abbreviation. characterStartPosition() would be fine.

> Source/WebCore/platform/graphics/chromium/FontLinux.cpp:211
> +            // No chars in this item to display.

I think this comment is saying almost the same of above comment. Braces can be removed when this comment is no longer needed.

> Source/WebCore/platform/graphics/chromium/FontLinux.cpp:227
> +            continue;

How about having a function which takes |from| and |to| as arguments and handles these logic in ComplexTextController? The function would return false if there is no glyph to be displayed in the run. Otherwise, the function would return true and update values of length() and positions(). This way, we don't need to expose logclusters and beginning offset.

> Source/WebCore/platform/graphics/chromium/FontLinux.cpp:238
> +            canvas->drawPosText(controller.glyphs() + fromGlyph, glyphCount << 1, controller.positions() + fromGlyph, strokePaint);

If you agree with having a function that handle clipping in ComplexTextController, these changes might not be needed.

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