Fix Views inline autocomplete with multi-char graphemes.

Fix Views inline autocomplete with multi-char graphemes.

Allow RenderText selection bounds amid multi-char graphemes.
Add a consolidated IsValidLogicalIndex implementation.
(this is less strict than the renamed IsValidCursorIndex)

Use adjacent cursor positions in MoveCursor as needed.
(prevents invalid cursors when collapsing selections)

Skip painting empty glyph ranges on Windows.
Add a unit test; minor cleanup and comment changes.

BUG=327903, 366786
TEST=Search for "จำลอง" in the omnibox; enter "จ" and get a valid selection of the inline autocomplete text.
R=asvitkine@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267752

[msw, 2014-04-29 06:26:25]

Hey Alexei, please take a look; thanks!

[msw, 2014-04-30 19:39:06]

Friendly ping, will you have a chance to review soon?
(others CC'ed are welcome to jump in with review)

[Alexei Svitkine (slow), 2014-04-30 19:40:43]

Sorry, will take a look now. Have been swarmed with reviews in the last
couple of days.


On Wed, Apr 30, 2014 at 3:39 PM, <msw@chromium.org> wrote:

> Friendly ping, will you have a chance to review soon?
> (others CC'ed are welcome to jump in with review)
>
> https://codereview.chromium.org/252563003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2014-04-30 19:53:24]

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:777: index < GetLayoutText().length()));
Hmm, I don't understand this logic anymore.

I know that these checks were done at the other functions already.... but...

It seems incorrect to me to check against both text().length() and
GetLayotText().length().

It should be clear whether the caller is expected to pass in a text() index or a
GetLayoutText() index. And depending on that, we should only use one of text()
or GetLayoutText() to validate, not both.

But perhaps I'm missing something. It's quite a while that I've worked on this
and don't remember all the subtleties. Either way, we should make code comments
clarify what type of indexes are being passed in and then once we have this
clarification, we can reason from there.

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:386: // multi-character graphemes may be allowed, unlike in
IsValidCursorIndex.
Can these comments clarify whether the indexes they're expecting are layout text
indexes or text() indexes?

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:857: if (colored_glyphs.is_empty())
Add a comment explaining in what situation this will be the case.

Also, this should probably be above the DCHECK() above.

[msw, 2014-05-01 20:40:04]

Please take another look; thanks!

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:777: index < GetLayoutText().length()));
On 2014/04/30 19:53:24, Alexei Svitkine wrote:
> Hmm, I don't understand this logic anymore.
> 
> I know that these checks were done at the other functions already.... but...
> 
> It seems incorrect to me to check against both text().length() and
> GetLayotText().length().
> 
> It should be clear whether the caller is expected to pass in a text() index or
a
> GetLayoutText() index. And depending on that, we should only use one of text()
> or GetLayoutText() to validate, not both.
> 
> But perhaps I'm missing something. It's quite a while that I've worked on this
> and don't remember all the subtleties. Either way, we should make code
comments
> clarify what type of indexes are being passed in and then once we have this
> clarification, we can reason from there.

I've modified the check to be simpler, clearer, and more correct. Sorry for the
confusion; |index| is a logical index into text(), but we also verify here that
it does not exceed the truncation length of the layout text. The
truncate_length_ is only used on Windows, where it's set to
kMaxUniscribeTextLength (10000). This hard limit only comes into play in
extraordinary circumstances and may not be needed once we switch to HarfBuzz.

I also added a text() length check to avoid IsValidCodePointIndex crashes (this
callpath accounts for ~24 of ~220 lifetime crash reports). LMK if you think that
new check is too aggressive.

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:386: // multi-character graphemes may be allowed, unlike in
IsValidCursorIndex.
On 2014/04/30 19:53:24, Alexei Svitkine wrote:
> Can these comments clarify whether the indexes they're expecting are layout
text
> indexes or text() indexes?

Done.

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/252563003/diff/140001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:857: if (colored_glyphs.is_empty())
On 2014/04/30 19:53:24, Alexei Svitkine wrote:
> Add a comment explaining in what situation this will be the case.
> 
> Also, this should probably be above the DCHECK() above.

Done.

[Alexei Svitkine (slow), 2014-05-01 20:48:08]

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:776: (index <= text().length() &&
Nit: Since you check for == above, change <= to < to make it clearer.

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:777: (truncate_length_ == 0 || index < truncate_length_)
&&
It seems wrong to me to accept text().length() but reject e.g. text().length() -
1 if text().length() is > truncate_length_. Should the truncate check be done
before the index == text().length() check?

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:922: base::string16 cases[] = { kHindi, kThai };
Nit: Can this be a "const base::string16& cases[] ="?

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:925: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(cases); i++) {
Nit: I think you can just use arraysize(cases) here since |cases| is not an anon
struct.

[msw, 2014-05-01 22:43:22]

Thanks for the comments, please take another look.

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:776: (index <= text().length() &&
On 2014/05/01 20:48:09, Alexei Svitkine wrote:
> Nit: Since you check for == above, change <= to < to make it clearer.

Done.

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:777: (truncate_length_ == 0 || index < truncate_length_)
&&
On 2014/05/01 20:48:09, Alexei Svitkine wrote:
> It seems wrong to me to accept text().length() but reject e.g. text().length()
-
> 1 if text().length() is > truncate_length_. Should the truncate check be done
> before the index == text().length() check?

That case isn't actually wrong. We disallow indices between the truncation
length and the underlying string length so that users can easily interact with
the underlying truncated text using the ellipsis as a proxy. So, users can
select all text, select the truncated text, and cursor from the last rendered
cursor position before truncation to the end of the string, all without setting
cursor positions to invisible indices or requiring unbounded arrow key presses
to traverse the ellipsis. (i.e. cursoring over the ellipsis that indicates
truncation transitions from the last visible position to the last logical
position). I tested this manually a good deal when I landed r209286.

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:922: base::string16 cases[] = { kHindi, kThai };
On 2014/05/01 20:48:09, Alexei Svitkine wrote:
> Nit: Can this be a "const base::string16& cases[] ="?

This yields "error: 'cases' declared as array of references of type 'const
base::string16 &'". An array of references is not a legal type, just as pointers
to references and references to references are prohibited. I did make the array
const, however.

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:925: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(cases); i++) {
On 2014/05/01 20:48:09, Alexei Svitkine wrote:
> Nit: I think you can just use arraysize(cases) here since |cases| is not an
anon
> struct.

Done.

[Alexei Svitkine (slow), 2014-05-01 22:53:38]

LGTM, thanks for the detailed explanation!

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:777: (truncate_length_ == 0 || index < truncate_length_)
&&
On 2014/05/01 22:43:22, msw wrote:
> On 2014/05/01 20:48:09, Alexei Svitkine wrote:
> > It seems wrong to me to accept text().length() but reject e.g.
text().length()
> -
> > 1 if text().length() is > truncate_length_. Should the truncate check be
done
> > before the index == text().length() check?
> 
> That case isn't actually wrong. We disallow indices between the truncation
> length and the underlying string length so that users can easily interact with
> the underlying truncated text using the ellipsis as a proxy. So, users can
> select all text, select the truncated text, and cursor from the last rendered
> cursor position before truncation to the end of the string, all without
setting
> cursor positions to invisible indices or requiring unbounded arrow key presses
> to traverse the ellipsis. (i.e. cursoring over the ellipsis that indicates
> truncation transitions from the last visible position to the last logical
> position). I tested this manually a good deal when I landed r209286.

That makes a lot of sense. Can you incorporate this explanation into the
comment?

[msw, 2014-05-01 23:04:34]

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/252563003/diff/160001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:777: (truncate_length_ == 0 || index < truncate_length_)
&&
On 2014/05/01 22:53:39, Alexei Svitkine wrote:
> On 2014/05/01 22:43:22, msw wrote:
> > On 2014/05/01 20:48:09, Alexei Svitkine wrote:
> > > It seems wrong to me to accept text().length() but reject e.g.
> text().length()
> > -
> > > 1 if text().length() is > truncate_length_. Should the truncate check be
> done
> > > before the index == text().length() check?
> > 
> > That case isn't actually wrong. We disallow indices between the truncation
> > length and the underlying string length so that users can easily interact
with
> > the underlying truncated text using the ellipsis as a proxy. So, users can
> > select all text, select the truncated text, and cursor from the last
rendered
> > cursor position before truncation to the end of the string, all without
> setting
> > cursor positions to invisible indices or requiring unbounded arrow key
presses
> > to traverse the ellipsis. (i.e. cursoring over the ellipsis that indicates
> > truncation transitions from the last visible position to the last logical
> > position). I tested this manually a good deal when I landed r209286.
> 
> That makes a lot of sense. Can you incorporate this explanation into the
> comment?

Done.

[msw, 2014-05-01 23:04:42]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-05-01 23:05:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/252563003/200001

[commit-bot: I haz the power, 2014-05-02 00:14:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 00:14:12]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[msw, 2014-05-02 00:38:19]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-05-02 00:38:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/252563003/200001

[commit-bot: I haz the power, 2014-05-02 05:15:15]

Change committed as 267752

[falken, 2014-05-02 09:13:24]

A revert of this CL has been created in
https://codereview.chromium.org/263833010/ by falken@chromium.org.

The reason for reverting is: Sorry to revert. After this patch the following
test started failing on XP Tests (3):
RenderTextTest.MidGraphemeSelectionBounds

Log:
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%283%29/builds...

Snippet from log:
c:\b\build\slave\cr-win-rel\build\src\ui\gfx\render_text_unittest.cc(930):
error: Value of: render_text->IsValidCursorIndex(1)
  Actual: true
Expected: false
.