[chromium] Font::drawComplexText can not draw a segment of text run

[Robin Cao]

The implementation of Font::drawComplexText() in Chromium Linux does not respect the |from| and |to| arguments. It just draws the whole text run. This will result in text overlap.

[Robin Cao]

Created attachment 114642 [details]
proposed patch with test case

[Robin Cao]

CC folks familiar with this part of the code

[Kenichi Ishibashi]

Comment on attachment 114642 [details]
proposed patch with test case

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.

[Robin Cao]

Thanks for your comments.

> 
> > 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.

Good point.

> 
> > 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.

I agree that we should add a function here, but personally, i don't like the idea of updating values of length() and positions() after calling nextScriptRun(). How about adding a function glyphsForRange() which will return the start index and length of glyphs that we need to draw?

[Robin Cao]

Created attachment 115186 [details]
patch-v2

[Robin Cao]

ping : i have updated the patch, could you take a look at it?

[Kenichi Ishibashi]

(In reply to comment #6)
> ping : i have updated the patch, could you take a look at it?

Sorry for late response. Looks good to me. Please wait for official reviewers review.

[Robin Cao]

(In reply to comment #7)
> 
> Sorry for late response. Looks good to me. Please wait for official reviewers review.

Thank you very much.

[Adam Barth]

Comment on attachment 115186 [details]
patch-v2

View in context: https://bugs.webkit.org/attachment.cgi?id=115186&action=review

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:548
> +void ComplexTextController::glyphsForRange(int from , int to, int& fromGlyph, int& glyphLength)

nit: "from ," has an extra space.

> LayoutTests/platform/chromium-linux/fast/text/international/draw-complex-text-from-to.html:11
> +<p>The text and ellipsis should not overlap.</p>

Should this test be run on all ports?

[Tony Chang]

Comment on attachment 115186 [details]
patch-v2

View in context: https://bugs.webkit.org/attachment.cgi?id=115186&action=review

>> LayoutTests/platform/chromium-linux/fast/text/international/draw-complex-text-from-to.html:11
>> +<p>The text and ellipsis should not overlap.</p>
> 
> Should this test be run on all ports?

Can you add a call to layoutTestController.dumpAsText(true) to not have the render tree in the text output?

[Robin Cao]

(In reply to comment #9)

> > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:548
> > +void ComplexTextController::glyphsForRange(int from , int to, int& fromGlyph, int& glyphLength)
> 
> nit: "from ," has an extra space.
> 

good catch! Will remove it.

[Robin Cao]

(In reply to comment #10)
> (From update of attachment 115186 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115186&action=review
> 
> >> LayoutTests/platform/chromium-linux/fast/text/international/draw-complex-text-from-to.html:11
> >> +<p>The text and ellipsis should not overlap.</p>
> > 
> > Should this test be run on all ports?
> 
> Can you add a call to layoutTestController.dumpAsText(true) to not have the render tree in the text output?

It makes sense to call layoutTestController.dumpAsText(true). If this test be run on all ports, should i generate all the expected PNGs on all platform in this patch?

[Robin Cao]

Created attachment 115720 [details]
patch-v3

[WebKit Review Bot]

Comment on attachment 115720 [details]
patch-v3

Clearing flags on attachment: 115720

Committed r100718: <http://trac.webkit.org/changeset/100718>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.