[webkit-reviews] review denied: [Bug 27889] [Chromium] RTL autocomplete popup is not layout correctly. : [Attachment 34536] patch w/ manual test

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Xiaomei Ji <xji at chromium.org>'s request for review:
Bug 27889: [Chromium] RTL autocomplete popup is not layout correctly.
https://bugs.webkit.org/show_bug.cgi?id=27889

Attachment 34536: patch w/ manual test
https://bugs.webkit.org/attachment.cgi?id=34536&action=review

------- Additional Comments from mitz at webkit.org
Switching to StringTruncator is a move in the right direction, but I don’t
think the GraphicsContext changes are good or necessary. GraphicsContext is not
the place to create Strings and truncate them. The logical structure where
drawBidiText calls drawBidiTextWithinWidth, which calls back into drawBidiText
does not make much sense either. Also, if width != -1 (that is, the caller
wants truncation), calling Font::floatWidth() once seems wasteful, as the
StringTruncator should have the smarts to detect the case where no truncation
is necessary.

I think your PopupListBox should handle the truncation, create a TextRun from
the potentially-truncated String, and call drawBidiText with that. I also don’t
understand why you need the ellipsesNeeded member. It seems like you could just
always use the StringTruncator. It won’t truncate when unnecessary. If you find
out that you need to optimize, then you can cache the actual truncated strings
(rather than just a bit telling you whether to truncate).