Improve RenderTextWin font fallback.

Improve RenderTextWin font fallback.

Don't use SCRIPT_UNDEFINED in the case of font fallback,
unless font fallback actually fails to return a script
that can display the characters. This fixes the problem
of some scripts not being properly displayed.

This actually makes RenderTextWin properly validate whether
a text position accepts a cursor, which caused several tests
to fail and revealed some additional issues in RenderTextWin.

The CL includes some modifications to address this.

Some tests are disabled under XP, see:
http://crbug.com/106450

Also, fixes some lint warnings.

BUG=90426
TEST=Run chrome.exe --use-pure-views and paste some Bengali
text into the omnibox. It should show up properly.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113635

[Alexei Svitkine (slow), 2011-11-21 20:08:02]

msw: review
sky: OWNERS review

[tfarina, 2011-11-22 00:33:18]

On 2011/11/21 20:08:02, Alexei Svitkine wrote:
> msw: review
> sky: OWNERS review

Looks like you forgot to add them to the reviewers list. Added them for you :)

[Alexei Svitkine (slow), 2011-11-22 02:04:14]

On 2011/11/22 00:33:18, tfarina wrote:
> On 2011/11/21 20:08:02, Alexei Svitkine wrote:
> > msw: review
> > sky: OWNERS review
> 
> Looks like you forgot to add them to the reviewers list. Added them for you :)

Wow, heh - thanks Thiago!

[msw, 2011-11-22 06:07:27]

It would be best if xji could review the IsCursorablePosition change, etc.
I'll take a look soon, but heavily buried under bubble work before i leave for
vacation tomorrow.

[Alexei Svitkine (slow), 2011-11-29 16:00:53]

Ping.

[msw, 2011-11-29 20:41:59]

Sorry for my questions; BiDi specifics can confuse even those that wrote the
code :)

http://codereview.chromium.org/8575020/diff/16001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/8575020/diff/16001/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:228: virtual bool IsCursorablePosition(size_t position,
bool is_trailing) = 0;
Can this take a SelectionModel::CaretPlacement instead of a bool?

http://codereview.chromium.org/8575020/diff/16001/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/8575020/diff/16001/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:223: bool
RenderTextLinux::IsCursorablePosition(size_t position, bool is_trailing) {
|is_trailing| is unused here... does it really have a purpose?

http://codereview.chromium.org/8575020/diff/16001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8575020/diff/16001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:371: if (is_trailing && !run->script_analysis.fRTL)
Shouldn't this be done before |GetRunContainingPosition(position);|?
What if the caret's |position| is trailing the last character of an RTL
run/string? Seems like potential out of bounds indexing...
I'm generally confused by this part of the CL; perhaps my check for cursorable
positions via comparing logical clusters is incorrect/insufficient?
At the same time, it seems odd that we were(Win)/are(Linux) checking the caret
position as cursorable without considering the LEADING/TRAILING placement.

http://codereview.chromium.org/8575020/diff/16001/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/8575020/diff/16001/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_views_model_unittest.cc:220: // TODO(xji):
temporarily disable in platform Win since the complex script
Can we enable these codepaths on Windows now that font fallback should eliminate
the empty/placeholder/square grapheme? There are 6 or so such "font regression"
blocks disabled for Windows in this file, maybe more in other tests.

[xji, 2011-11-29 21:15:48]

I did not look at RenderTextWin part.

http://codereview.chromium.org/8575020/diff/16001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/8575020/diff/16001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:212: (sel.caret_placement() == SelectionModel::TRAILING)))
{
I did not quite get the reason of introducing "bool is_trailing" to
IsCursorablePosition.

As to this call site, it originally means caret_pos() itself should also be a
cursorable position regardless whether it is the leading edge or trailing edge.

For example:
ABC,  "BC" belongs to one grapheme, 
cursorable positions are |ABC, A|BC, and ABC|.
If selection model is (2, trailing, 3), which means caret_pos is 2, it is the
trailing edge of 2nd character, and the logical index is 3, it is not a correct
selection_model because 2nd character is not the start of a grapheme.

After this change, if caret_placement() is trailing, does 
IsCursorPosition(sel.caret_pos(), true)
check the same position as
IsCursorPosition(sel.selection_end(), false)?

will it be treated as a valid selection model in Win?

http://codereview.chromium.org/8575020/diff/16001/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/8575020/diff/16001/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_views_model_unittest.cc:220: // TODO(xji):
temporarily disable in platform Win since the complex script
On 2011/11/29 20:42:00, msw wrote:
> Can we enable these codepaths on Windows now that font fallback should
eliminate
> the empty/placeholder/square grapheme? There are 6 or so such "font
regression"
> blocks disabled for Windows in this file, maybe more in other tests.

Agree.

[Alexei Svitkine (slow), 2011-11-29 22:09:21]

> For example:
> ABC,  "BC" belongs to one grapheme,
> cursorable positions are |ABC, A|BC, and ABC|.
> If selection model is (2, trailing, 3), which means caret_pos is 2, it
> is the trailing edge of 2nd character, and the logical index is 3, it is
> not a correct selection_model because 2nd character is not the start of
> a grapheme.

Maybe in this case, it is the test that is wrong? Let me describe
what fails that prompted this change and maybe we can agree on a
different solution from there.

The test that fails is at the start of TextfieldViewsModelTest,
EditString_ComplexScript.

It initially sets the text to the Hindi:

\x0915 \x093f \x0915 \x094d \x0915

After this text is set, the failure ends up being caused by
IsCursorablePosition() validating (caret_pos = 4,
SelectionModel::TRAILING) as invalid under Windows with my font
fallback change. Then when the next text is appended, it gets
appended to the start rather than to the end of the existing text, and
causes the test to fail when it checks the result string.

I just verified that Windows treats the following positions - that I've
indicated via |'s - as cursorable in that string:

| \x0915 \x093f | \x0915 \x094d \x0915 |

So essentially Windows treats this string as having two graphemes -
the first pair of characters and the last three characters. (I
verified this by making an HTML page with content
"किक्क", opening in a browser
and copying and pasting into a native Windows text field.)

So the question is, given the above, is (caret_pos = 4,
SelectionModel::TRAILING) a cursorable position?

If that position refers to the end of the string - i.e. _after_
character at index 4 - then it should clearly be cursorable. However,
the current code treats that position as being between characters at
index 3 and 4 - i.e. at this location:

\x0915 \x093f \x0915 \x094d | \x0915

According to behaviour in other Windows apps and the Windows API we're
using, that position is NOT cursorable.

So the question is whether the code is validating the wrong position
or whether the text is wrong.

Xiaomei, what do you think?

>
http://codereview.chromium.org/8575020/diff/16001/ui/views/controls/textfield...
> File ui/views/controls/textfield/textfield_views_model_unittest.cc
> (right):
>
>
http://codereview.chromium.org/8575020/diff/16001/ui/views/controls/textfield...
> ui/views/controls/textfield/textfield_views_model_unittest.cc:220: //
> TODO(xji): temporarily disable in platform Win since the complex script
> On 2011/11/29 20:42:00, msw wrote:
>>
>> Can we enable these codepaths on Windows [...] ?

I'd like to reach a consensus on how to fix the new failures first,
but I agree we should try to enable this now that we'll have Windows
font fallback.

[xji, 2011-11-29 23:51:56]

On 2011/11/29 22:09:21, Alexei Svitkine wrote:
> > For example:
> > ABC,  "BC" belongs to one grapheme,
> > cursorable positions are |ABC, A|BC, and ABC|.
> > If selection model is (2, trailing, 3), which means caret_pos is 2, it
> > is the trailing edge of 2nd character, and the logical index is 3, it is
> > not a correct selection_model because 2nd character is not the start of
> > a grapheme.
> 
> Maybe in this case, it is the test that is wrong? Let me describe
> what fails that prompted this change and maybe we can agree on a
> different solution from there.
> 
> The test that fails is at the start of TextfieldViewsModelTest,
> EditString_ComplexScript.
> 
> It initially sets the text to the Hindi:
> 
> \x0915 \x093f \x0915 \x094d \x0915
> 
> After this text is set, the failure ends up being caused by
> IsCursorablePosition() validating (caret_pos = 4,
> SelectionModel::TRAILING) as invalid under Windows with my font
> fallback change. Then when the next text is appended, it gets
> appended to the start rather than to the end of the existing text, and
> causes the test to fail when it checks the result string.
> 
> I just verified that Windows treats the following positions - that I've
> indicated via |'s - as cursorable in that string:
> 
> | \x0915 \x093f | \x0915 \x094d \x0915 |
> 
> So essentially Windows treats this string as having two graphemes -
> the first pair of characters and the last three characters. (I
> verified this by making an HTML page with content
> "किक्क", opening in a browser
> and copying and pasting into a native Windows text field.)
> 
> So the question is, given the above, is (caret_pos = 4,
> SelectionModel::TRAILING) a cursorable position?
> 
> If that position refers to the end of the string - i.e. _after_
> character at index 4 - then it should clearly be cursorable. However,
> the current code treats that position as being between characters at
> index 3 and 4 - i.e. at this location:
> 
> \x0915 \x093f \x0915 \x094d | \x0915
> 
> According to behaviour in other Windows apps and the Windows API we're
> using, that position is NOT cursorable.
> 
> So the question is whether the code is validating the wrong position
> or whether the text is wrong.
> 
> Xiaomei, what do you think?
> 

Got it. Jungshik and I actually discussed this with a Hindi speaking co-worker
before, and I just double checked with him.

As to whether treat "\x0915 \x094d \x0915" as one grapheme or 2 graphemes, there
is no hard-line. Both are acceptable.
Windows treats it as 1 grapheme, Pango treats it as 2.

So, in windows, (caret_pos = 4, SelectionModel::TRAILING) is not a cursorable
position, while in Linux, it is.

IsCursorablePosition() is doing the right thing, instead of changing its
behavior, you need to change the test expectation for Win.

I tested in Win (IE and Firefox) and Linux (Chrome and Firefox), neither will
introduce crbug.com/24840, and we are fine here.

[Alexei Svitkine (slow), 2011-11-30 15:47:41]

> IsCursorablePosition() is doing the right thing, instead of changing its
> behavior, you need to change the test expectation for Win.

Unfortunately, I believe the problem is deeper than this - I don't
think disabling this test is the right solution.

The underlying issue happens in TextfieldViewsModel::Append(), when it
calls MoveCursorRight(), in this code:

void TextfieldViewsModel::Append(const string16& text) {
  if (HasCompositionText())
    ConfirmCompositionText();
  size_t save = GetCursorPosition();
  if (render_text_->GetTextDirection() == base::i18n::LEFT_TO_RIGHT)
    MoveCursorRight(gfx::LINE_BREAK, false);
  else
    MoveCursorLeft(gfx::LINE_BREAK, false);
  InsertText(text);
  render_text_->SetCursorPosition(save);
  ClearSelection();
}


This failing Append() gets called when the existing text is:

\x0915 \x093f \x0915 \x094d \x0915

The call MoveCursorRight(LINE_BREAK, false) fails to move the cursor
to the end of the string, due to failing to validate caret_pos (4,
TRAILING) as valid. This results the call to
MoveCursorRight(LINE_BREAK, false) being a nop in this case, and the
string getting appended to the start rather than the end.

So I believe the correct fix will have to be to MoveCursorRight() - it
shouldn't be trying to set caret_pos to (4, TRAILING), since it is not
cursorable? However, I am still a bit unclear on this...

Given that the current text is of length 5, so character index 4 is
the last character. A caret position at the end of the text should be
trailing after the last character (since this is LTR) - then isn't (4,
TRAILING) the correct caret pos? But this position gets rejected by
IsCursorablePosition()!

If that shouldn't be the caret_pos in this case, then what should
MoveCursorRight(LINE_BREAK, false) be setting caret_pos to?

-Alexei

[Alexei Svitkine (slow), 2011-11-30 15:52:33]

I guess my question just boils down to, given that Hindi string, what
is the correct caret_pos when at the very end of the input?

I had understood it as (4, TRAILING) - i.e. trailing after character
index 4, the last character in this string. This also corresponds to
what MoveCursorRight() is setting it to.

But you say that (4, TRAILING) is not a cursorable position in this
case (on Windows). So what should it be instead?

On Wed, Nov 30, 2011 at 10:46 AM, Alexei Svitkine
<asvitkine@chromium.org> wrote:
>> IsCursorablePosition() is doing the right thing, instead of changing its
>> behavior, you need to change the test expectation for Win.
>
> Unfortunately, I believe the problem is deeper than this - I don't
> think disabling this test is the right solution.
>
> The underlying issue happens in TextfieldViewsModel::Append(), when it
> calls MoveCursorRight(), in this code:
>
> void TextfieldViewsModel::Append(const string16& text) {
>  if (HasCompositionText())
>    ConfirmCompositionText();
>  size_t save = GetCursorPosition();
>  if (render_text_->GetTextDirection() == base::i18n::LEFT_TO_RIGHT)
>    MoveCursorRight(gfx::LINE_BREAK, false);
>  else
>    MoveCursorLeft(gfx::LINE_BREAK, false);
>  InsertText(text);
>  render_text_->SetCursorPosition(save);
>  ClearSelection();
> }
>
>
> This failing Append() gets called when the existing text is:
>
> \x0915 \x093f \x0915 \x094d \x0915
>
> The call MoveCursorRight(LINE_BREAK, false) fails to move the cursor
> to the end of the string, due to failing to validate caret_pos (4,
> TRAILING) as valid. This results the call to
> MoveCursorRight(LINE_BREAK, false) being a nop in this case, and the
> string getting appended to the start rather than the end.
>
> So I believe the correct fix will have to be to MoveCursorRight() - it
> shouldn't be trying to set caret_pos to (4, TRAILING), since it is not
> cursorable? However, I am still a bit unclear on this...
>
> Given that the current text is of length 5, so character index 4 is
> the last character. A caret position at the end of the text should be
> trailing after the last character (since this is LTR) - then isn't (4,
> TRAILING) the correct caret pos? But this position gets rejected by
> IsCursorablePosition()!
>
> If that shouldn't be the caret_pos in this case, then what should
> MoveCursorRight(LINE_BREAK, false) be setting caret_pos to?
>
> -Alexei
>

[xji, 2011-11-30 18:22:49]

On 2011/11/30 15:52:33, Alexei Svitkine wrote:
> I guess my question just boils down to, given that Hindi string, what
> is the correct caret_pos when at the very end of the input?

caret_pos should be at the boundary of grapheme, not boundary of character.
(carat_pos, caret_placement) means visually the cursor is placed at the trailing
or leading edge of grapheme caret_pos-th.

> 
> I had understood it as (4, TRAILING) - i.e. trailing after character
> index 4, the last character in this string. This also corresponds to
> what MoveCursorRight() is setting it to.
> 
> But you say that (4, TRAILING) is not a cursorable position in this
> case (on Windows). So what should it be instead?

Given your example:
\x0915 \x093f \x0915 \x094d \x0915
in windows, the valid caret_pos are: 0, 2, 5 (but I think we disabled (5,
leading) as a valid one, and we will always use (2, trailing) as replacement).

The end of string is (2, trailing).

In MoveLeft/Right, it should always traverse the string looking for left/right
grapheme (not character), and it should handle it correctly. If not, it is a
bug.

BTW, I did not mean to disable the test, I mean to provide a different
expectation for windows.


> 
> On Wed, Nov 30, 2011 at 10:46 AM, Alexei Svitkine
> <mailto:asvitkine@chromium.org> wrote:
> >> IsCursorablePosition() is doing the right thing, instead of changing its
> >> behavior, you need to change the test expectation for Win.
> >
> > Unfortunately, I believe the problem is deeper than this - I don't
> > think disabling this test is the right solution.
> >
> > The underlying issue happens in TextfieldViewsModel::Append(), when it
> > calls MoveCursorRight(), in this code:
> >
> > void TextfieldViewsModel::Append(const string16& text) {
> > &nbsp;if (HasCompositionText())
> > &nbsp; &nbsp;ConfirmCompositionText();
> > &nbsp;size_t save = GetCursorPosition();
> > &nbsp;if (render_text_->GetTextDirection() == base::i18n::LEFT_TO_RIGHT)
> > &nbsp; &nbsp;MoveCursorRight(gfx::LINE_BREAK, false);
> > &nbsp;else
> > &nbsp; &nbsp;MoveCursorLeft(gfx::LINE_BREAK, false);
> > &nbsp;InsertText(text);
> > &nbsp;render_text_->SetCursorPosition(save);
> > &nbsp;ClearSelection();
> > }
> >
> >
> > This failing Append() gets called when the existing text is:
> >
> > \x0915 \x093f \x0915 \x094d \x0915
> >
> > The call MoveCursorRight(LINE_BREAK, false) fails to move the cursor
> > to the end of the string, due to failing to validate caret_pos (4,
> > TRAILING) as valid. This results the call to
> > MoveCursorRight(LINE_BREAK, false) being a nop in this case, and the
> > string getting appended to the start rather than the end.
> >
> > So I believe the correct fix will have to be to MoveCursorRight() - it
> > shouldn't be trying to set caret_pos to (4, TRAILING), since it is not
> > cursorable? However, I am still a bit unclear on this...
> >
> > Given that the current text is of length 5, so character index 4 is
> > the last character. A caret position at the end of the text should be
> > trailing after the last character (since this is LTR) - then isn't (4,
> > TRAILING) the correct caret pos? But this position gets rejected by
> > IsCursorablePosition()!
> >
> > If that shouldn't be the caret_pos in this case, then what should
> > MoveCursorRight(LINE_BREAK, false) be setting caret_pos to?
> >
> > -Alexei
> >

[xji, 2011-11-30 18:25:46]

On 2011/11/30 15:47:41, Alexei Svitkine wrote:
> > IsCursorablePosition() is doing the right thing, instead of changing its
> > behavior, you need to change the test expectation for Win.
> 
> Unfortunately, I believe the problem is deeper than this - I don't
> think disabling this test is the right solution.
> 
> The underlying issue happens in TextfieldViewsModel::Append(), when it
> calls MoveCursorRight(), in this code:
> 
> void TextfieldViewsModel::Append(const string16& text) {
>   if (HasCompositionText())
>     ConfirmCompositionText();
>   size_t save = GetCursorPosition();
>   if (render_text_->GetTextDirection() == base::i18n::LEFT_TO_RIGHT)
>     MoveCursorRight(gfx::LINE_BREAK, false);
>   else
>     MoveCursorLeft(gfx::LINE_BREAK, false);
>   InsertText(text);
>   render_text_->SetCursorPosition(save);
>   ClearSelection();
> }
> 
> 
> This failing Append() gets called when the existing text is:
> 
> \x0915 \x093f \x0915 \x094d \x0915
> 
> The call MoveCursorRight(LINE_BREAK, false) fails to move the cursor
> to the end of the string, due to failing to validate caret_pos (4,
> TRAILING) as valid. This results the call to
> MoveCursorRight(LINE_BREAK, false) being a nop in this case, and the
> string getting appended to the start rather than the end.

why selection model is passed as (4, trailing) instead of (2, trailing) from
Append() to MoveCursorRight/Left()?


> 
> So I believe the correct fix will have to be to MoveCursorRight() - it
> shouldn't be trying to set caret_pos to (4, TRAILING), since it is not
> cursorable? However, I am still a bit unclear on this...
> 
> Given that the current text is of length 5, so character index 4 is
> the last character. A caret position at the end of the text should be
> trailing after the last character (since this is LTR) - then isn't (4,
> TRAILING) the correct caret pos? But this position gets rejected by
> IsCursorablePosition()!
> 
> If that shouldn't be the caret_pos in this case, then what should
> MoveCursorRight(LINE_BREAK, false) be setting caret_pos to?
> 
> -Alexei

[Alexei Svitkine (slow), 2011-11-30 18:40:03]

On Wed, Nov 30, 2011 at 1:25 PM,  <xji@chromium.org> wrote:
> On 2011/11/30 15:47:41, Alexei Svitkine wrote:
>>
>> > IsCursorablePosition() is doing the right thing, instead of changing its
>> > behavior, you need to change the test expectation for Win.
>
>> Unfortunately, I believe the problem is deeper than this - I don't
>> think disabling this test is the right solution.
>
>> The underlying issue happens in TextfieldViewsModel::Append(), when it
>> calls MoveCursorRight(), in this code:
>
>> void TextfieldViewsModel::Append(const string16& text) {
>>   if (HasCompositionText())
>>     ConfirmCompositionText();
>>   size_t save = GetCursorPosition();
>>   if (render_text_->GetTextDirection() == base::i18n::LEFT_TO_RIGHT)
>>     MoveCursorRight(gfx::LINE_BREAK, false);
>>   else
>>     MoveCursorLeft(gfx::LINE_BREAK, false);
>>   InsertText(text);
>>   render_text_->SetCursorPosition(save);
>>   ClearSelection();
>> }
>
>
>> This failing Append() gets called when the existing text is:
>
>> \x0915 \x093f \x0915 \x094d \x0915
>
>> The call MoveCursorRight(LINE_BREAK, false) fails to move the cursor
>> to the end of the string, due to failing to validate caret_pos (4,
>> TRAILING) as valid. This results the call to
>> MoveCursorRight(LINE_BREAK, false) being a nop in this case, and the
>> string getting appended to the start rather than the end.
>
> why selection model is passed as (4, trailing) instead of (2, trailing) from
> Append() to MoveCursorRight/Left()?

Aha, I understand it now - my confusion was understanding as "trailing
character" rather than "trailing grapheme". The bug is in
RightEndSelectionModel() in RenderTextWin which just subtracts 1,
rather than getting the index of the previous grapheme.

There appears to be a similar bug in LeftEndSelectionModel() in
RenderTextWin. I'll fix this and add some unit tests specifically for
this behaviour. Thanks for helping me understand this.

[Alexei Svitkine (slow), 2011-11-30 21:12:42]

PTAL.

There were problems in RenderTextWin's RightEndSelectionModel() and
IndexOfAdjacentGrapheme() functions that were responsible for the non-grapheme
boundary caret_pos, which I've fixed.

RightEndSelectionModel() was simply returning a caret of run->range.end() - 1,
rather than the index of the previous grapheme.

IndexOfAdjacentGrapheme() was not returning grapheme-boundary results when
called with index == text().length() and also wasn't returning grapheme-boundary
results when !next.

Note: This CL is still missing some unit tests that I want to write for these
fixes. Also, when I write the unit tests, I plan to check
LeftEndSelectionModel(), which I believe has the same issue as
RightEndSelectionModel() did.

[msw, 2011-11-30 21:37:33]

LGTM with some additional comments and one nit.
You are awesome for resolving this complicated issue!

http://codereview.chromium.org/8575020/diff/26002/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8575020/diff/26002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:481: next = true;
This is a bit of trickery right here.
It looks okay, but please add explanatory comments.

http://codereview.chromium.org/8575020/diff/26002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:600: // Only try font fallback if it hasn't yet been
attempted for this run.
nit: disambiguate this comment and the |tried_fallback| flag for
SCRIPT_UNDEFINED from the term "font fallback".

[msw, 2011-11-30 21:38:26]

Oh, also, please update the CL description.

[msw, 2011-11-30 21:38:49]

Doh, nvm, it contains the right info.

[sky, 2011-11-30 21:54:59]

Rubber stamp LGTM

[xji, 2011-12-01 08:31:57]

http://codereview.chromium.org/8575020/diff/31001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8575020/diff/31001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:478: index = IndexOfAdjacentGrapheme(last, false);
is the reason of this special handling that:
run_index returned from GetRunContainingPosition(text.length()) >= runs_.size()
,so that the 'run' will be assigned to NULL and following ops are wrong?

http://codereview.chromium.org/8575020/diff/31001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:500: ch--;
this does not seem right.
for example: ABCDE, assume CDE are in same grapheme,
if index is 3, IndexOfAdjacentGrapheme(3, false) should be 2, not 1.

It is the same for Linux.
The name is confusing, I think the purpose of this function is looking for the
previous or next grapheme boundary.

I think we should have test to cover this, or maybe the test is disabled in Win?
If there is not test coverage, we should definitely add one. 

And could you please also clarify this in the comments in render_text.h?

http://codereview.chromium.org/8575020/diff/31001/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield_views_model_unittest.cc (left):

http://codereview.chromium.org/8575020/diff/31001/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_views_model_unittest.cc:154: #if
defined(OS_LINUX)
just wondering why those test are enabled but not others?

[Alexei Svitkine (slow), 2011-12-01 18:17:23]

Please take another look.

I've added unit tests for both IndexOfAdjacentGrapheme() and
LeftEndSelectionModel()/RightEndSelectionModel() and fixed
IndexOfAdjacentGrapheme() to behave per xji@'s comment.

However, there is one case in my unit test where I'm seeing
RightEndSelectionModel() inexplicably giving a different result on Linux, which
I've ifdef'd out with a TODO.

Thanks.

http://codereview.chromium.org/8575020/diff/26002/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8575020/diff/26002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:481: next = true;
On 2011/11/30 21:37:33, msw wrote:
> This is a bit of trickery right here.
> It looks okay, but please add explanatory comments.

Done.

http://codereview.chromium.org/8575020/diff/26002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:600: // Only try font fallback if it hasn't yet been
attempted for this run.
On 2011/11/30 21:37:33, msw wrote:
> nit: disambiguate this comment and the |tried_fallback| flag for
> SCRIPT_UNDEFINED from the term "font fallback".

Sorry, I don't understand your suggestion here. I named it 'tried_fallback' to
indicate whether we've already called |ChooseFallbackFont()| once for this text
run.

The idea being to set SCRIPT_UNDEFINED only if we did tried to find a fallback
font but still got USP_E_SCRIPT_NOT_IN_FONT. (As opposed to the previous code
which would set SCRIPT_UNDEFINED even if a suitable font was found.)

Can you elaborate on what you'd rather see for the name of this variable?

http://codereview.chromium.org/8575020/diff/31001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8575020/diff/31001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:478: index = IndexOfAdjacentGrapheme(last, false);
On 2011/12/01 08:31:58, xji wrote:
> is the reason of this special handling that:
> run_index returned from GetRunContainingPosition(text.length()) >=
runs_.size()
> ,so that the 'run' will be assigned to NULL and following ops are wrong?

Correct. The previous logic was incorrect in this case. By special-casing it, it
also makes the run != NULL code cleaner.

http://codereview.chromium.org/8575020/diff/31001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:500: ch--;
On 2011/12/01 08:31:58, xji wrote:
> this does not seem right.
> for example: ABCDE, assume CDE are in same grapheme,
> if index is 3, IndexOfAdjacentGrapheme(3, false) should be 2, not 1.
> 
> It is the same for Linux.
> The name is confusing, I think the purpose of this function is looking for the
> previous or next grapheme boundary.

I see - this wasn't clear to me before. I've adjusted the code to fit this
behaviour and fixed the header comments to be clear about this.

> I think we should have test to cover this, or maybe the test is disabled in
Win?
> If there is not test coverage, we should definitely add one. 

I've now added unit tests for this, as promised. I also had to fix the case of
param |index| >= text().length() on Linux to return text().length().

> 
> And could you please also clarify this in the comments in render_text.h?

Done.

http://codereview.chromium.org/8575020/diff/31001/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield_views_model_unittest.cc (left):

http://codereview.chromium.org/8575020/diff/31001/ui/views/controls/textfield...
ui/views/controls/textfield/textfield_views_model_unittest.cc:154: #if
defined(OS_LINUX)
On 2011/12/01 08:31:58, xji wrote:
> just wondering why those test are enabled but not others?

I only enabled the ones that now pass on Windows. I did not investigate why the
others still do not pass, so I've left them disabled with the existing TODO's.

[xji, 2011-12-01 20:19:14]

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:288: return text().length();
Thanks for fixing this.
Without the fix, does Linux treats "index > text().length()" as "index ==
text().length()"?

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:623: // TODO(asvitkine): This behaves differently
on Linux. Why?
Thanks for adding those tests!

In Windows, text is always displayed as left-to-right, (I wont use your case,
since for your case, the text is displayed exactly the same whether the
directionality is LTR or RTL, even though the rightmost character's logical
index is different for LTR and RTL).
Let's say the text is ABaCD where capital letter represents Hebrew,
it is displayed as  BAaDC

In Linux, pango use the directionality of first strong character's to determine
the direction of the text, so for above case, it is right-to-left, and it is
displayed as:  DCaBA
 
So for pango, the left-end is: (3, trailing), the right-end is (0, leading).
The expectation should be:
{ kHebrewLatin, 3, SelectionModel::TRAILING, 0, SelectionModel::LEADING },

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:482: // If the requested |index| is past the end, use
the index of the last
is the comment apply? till here,  "index <= text().length()".

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:504: start = run->range.start();
you did not seem to use the 'start'.

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:507: } else if (run->logical_clusters[ch - 1] !=
run->logical_clusters[ch]) {
So, the old code does not handle 'ch==0' correctly, right?

It took me quite long to figure out why you need this "else if" instead of
"else".

I can not think of anything better except duplicate loop as following (hopefully
the same functionality). Not sure whether it is more readable.

if (!next) {
  // If |ch| is the start of the run, use the preceding run, if any.
  if (ch == 0) {
    if (run_index == 0)
      return 0;
    run = runs_[run_index - 1];
    length = run->range.length();
    ch = length - 1;
    cluster = run->logical_clusters[ch]; 
    while (ch > 0 && run->logical_clusters[ch - 1] == cluster);
      ch--;
  } else {
   do {
      ch--;
    } while (ch >= 0 && run->logical_clusters[ch] == cluster);
  }
}

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:513: while (ch > 0 && run->logical_clusters[ch - 1] ==
run->logical_clusters[ch])
Is the logic correct here?

for example: ABCDE, where "ABCD" is one grapheme, when index is at ABCDE|,
'ch' is set to "ABCD|E" at line 509. then, will 'ch' be continually decrease to
'|ABCDE' in this loop?

should it check against cluster? (and add 'cluster = run->logical_clusters[ch]'
in 'ch==0' branch?

And actually, in the above "else if", "ch--" should be the returned value (no
need to continue in loop).

[Alexei Svitkine (slow), 2011-12-01 20:58:58]

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:288: return text().length();
On 2011/12/01 20:19:15, xji wrote:
> Thanks for fixing this.
> Without the fix, does Linux treats "index > text().length()" as "index ==
> text().length()"?

Yes and also fails intermittently in Utf16IndexToUtf8Index() with some gtk error
codes due to out of bounds...

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:623: // TODO(asvitkine): This behaves differently
on Linux. Why?
On 2011/12/01 20:19:15, xji wrote:
> Thanks for adding those tests!
> 
> In Windows, text is always displayed as left-to-right, (I wont use your case,
> since for your case, the text is displayed exactly the same whether the
> directionality is LTR or RTL, even though the rightmost character's logical
> index is different for LTR and RTL).
> Let's say the text is ABaCD where capital letter represents Hebrew,
> it is displayed as  BAaDC
> 
> In Linux, pango use the directionality of first strong character's to
determine
> the direction of the text, so for above case, it is right-to-left, and it is
> displayed as:  DCaBA
>  
> So for pango, the left-end is: (3, trailing), the right-end is (0, leading).
> The expectation should be:
> { kHebrewLatin, 3, SelectionModel::TRAILING, 0, SelectionModel::LEADING },

I see. I've added the Linux expectation.

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:482: // If the requested |index| is past the end, use
the index of the last
On 2011/12/01 20:19:15, xji wrote:
> is the comment apply? till here,  "index <= text().length()".

Good catch, the comment meant to say is "at the end". Fixed.

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:504: start = run->range.start();
On 2011/12/01 20:19:15, xji wrote:
> you did not seem to use the 'start'.

It's used at the return point. Also, just keeping (run, start, length)
consistent with each other to avoid confusion.

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:507: } else if (run->logical_clusters[ch - 1] !=
run->logical_clusters[ch]) {
On 2011/12/01 20:19:15, xji wrote:
> So, the old code does not handle 'ch==0' correctly, right?

Correct. This is covered by the unit test.

> It took me quite long to figure out why you need this "else if" instead of
> "else".


> 
> I can not think of anything better except duplicate loop as following
(hopefully
> the same functionality). Not sure whether it is more readable. [snip]

I feel that is less readable, but maybe I'm biased because I wrote the other
version of the code. ;) Let's see what Mike thinks.

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:513: while (ch > 0 && run->logical_clusters[ch - 1] ==
run->logical_clusters[ch])
On 2011/12/01 20:19:15, xji wrote:
> Is the logic correct here?
> 
> for example: ABCDE, where "ABCD" is one grapheme, when index is at ABCDE|,
> 'ch' is set to "ABCD|E" at line 509. then, will 'ch' be continually decrease
to
> '|ABCDE' in this loop?

Let me change your example to ABCD|e|f, since ABCD|e will be handled by a
special case above and won't reach this code. We also assume everything is
within a single run, else line 509 won't be executed because ch == 0 if there
was a run boundary.

The index passed in would be: ABCDe|f. Line 509 will execute because
cluster('e') != cluster('f'). So now, it will point to: ABCD|ef.

Then ch > 0 is true, so we check "run->logical_clusters[ch - 1] ==
run->logical_clusters[ch]", which is equivalent to "cluster('D') ==
cluster('e')". This is FALSE, so the loop will not execute, and the correct
value will be returned.

> And actually, in the above "else if", "ch--" should be the returned value (no
> need to continue in loop).

No, that is absolutely needed. Just "ch--" will not produce a grapheme-boundary
result. That "else if" is checking essentially IsCursorablePosition(index). If
it is cursorable, then |ch| is at a grapheme boundary and thus the return value
should be the start of the previous grapheme. So we decrement 1 to get to the
last character of the previous grapheme, and then loop to get to the start of
that grapheme.

[xji, 2011-12-01 22:56:06]

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:513: while (ch > 0 && run->logical_clusters[ch - 1] ==
run->logical_clusters[ch])
On 2011/12/01 20:58:58, Alexei Svitkine wrote:
> On 2011/12/01 20:19:15, xji wrote:
> > Is the logic correct here?
> > 
> > for example: ABCDE, where "ABCD" is one grapheme, when index is at ABCDE|,
> > 'ch' is set to "ABCD|E" at line 509. then, will 'ch' be continually decrease
> to
> > '|ABCDE' in this loop?
> 
> Let me change your example to ABCD|e|f, since ABCD|e will be handled by a
> special case above and won't reach this code. We also assume everything is
> within a single run, else line 509 won't be executed because ch == 0 if there
> was a run boundary.
> 
> The index passed in would be: ABCDe|f. Line 509 will execute because
> cluster('e') != cluster('f'). So now, it will point to: ABCD|ef.
> 
> Then ch > 0 is true, so we check "run->logical_clusters[ch - 1] ==
> run->logical_clusters[ch]", which is equivalent to "cluster('D') ==
> cluster('e')". This is FALSE, so the loop will not execute, and the correct
> value will be returned.
> 
> > And actually, in the above "else if", "ch--" should be the returned value
(no
> > need to continue in loop).
> 
> No, that is absolutely needed. Just "ch--" will not produce a
grapheme-boundary
> result. That "else if" is checking essentially IsCursorablePosition(index). If
> it is cursorable, then |ch| is at a grapheme boundary and thus the return
value
> should be the start of the previous grapheme. So we decrement 1 to get to the
> last character of the previous grapheme, and then loop to get to the start of
> that grapheme.
> 
> 

Ah, my bad, mis-matched the index and its character.
thanks for the explanation. 
Then, why line 507 is "else if" instead of "else"?

[Alexei Svitkine (slow), 2011-12-01 22:59:06]

On 2011/12/01 22:56:06, xji wrote:
> http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc
> File ui/gfx/render_text_win.cc (right):
> 
>
http://codereview.chromium.org/8575020/diff/37001/ui/gfx/render_text_win.cc#n...
> ui/gfx/render_text_win.cc:513: while (ch > 0 && run->logical_clusters[ch - 1]
==
> run->logical_clusters[ch])
> On 2011/12/01 20:58:58, Alexei Svitkine wrote:
> > On 2011/12/01 20:19:15, xji wrote:
> > > Is the logic correct here?
> > > 
> > > for example: ABCDE, where "ABCD" is one grapheme, when index is at ABCDE|,
> > > 'ch' is set to "ABCD|E" at line 509. then, will 'ch' be continually
decrease
> > to
> > > '|ABCDE' in this loop?
> > 
> > Let me change your example to ABCD|e|f, since ABCD|e will be handled by a
> > special case above and won't reach this code. We also assume everything is
> > within a single run, else line 509 won't be executed because ch == 0 if
there
> > was a run boundary.
> > 
> > The index passed in would be: ABCDe|f. Line 509 will execute because
> > cluster('e') != cluster('f'). So now, it will point to: ABCD|ef.
> > 
> > Then ch > 0 is true, so we check "run->logical_clusters[ch - 1] ==
> > run->logical_clusters[ch]", which is equivalent to "cluster('D') ==
> > cluster('e')". This is FALSE, so the loop will not execute, and the correct
> > value will be returned.
> > 
> > > And actually, in the above "else if", "ch--" should be the returned value
> (no
> > > need to continue in loop).
> > 
> > No, that is absolutely needed. Just "ch--" will not produce a
> grapheme-boundary
> > result. That "else if" is checking essentially IsCursorablePosition(index).
If
> > it is cursorable, then |ch| is at a grapheme boundary and thus the return
> value
> > should be the start of the previous grapheme. So we decrement 1 to get to
the
> > last character of the previous grapheme, and then loop to get to the start
of
> > that grapheme.
> > 
> > 
> 
> Ah, my bad, mis-matched the index and its character.
> thanks for the explanation. 
> Then, why line 507 is "else if" instead of "else"?

Because it only needs to do "ch--" if the index is at a grapheme boundary. If
it's not at the grapheme boundary (i.e. in the middle of a grapheme), then the
loop will do the right thing already.

[Alexei Svitkine (slow), 2011-12-01 23:01:40]

> > Then, why line 507 is "else if" instead of "else"?
> 
> Because it only needs to do "ch--" if the index is at a grapheme boundary. If
> it's not at the grapheme boundary (i.e. in the middle of a grapheme), then the
> loop will do the right thing already.

Ah, I see what you're saying. I think you're right, it can be just an "else",
since the "ch--" won't cause any harm in the other case (it will be essentially
equivalent to the first iteration of the loop).

[Alexei Svitkine (slow), 2011-12-01 23:09:32]

On Thu, Dec 1, 2011 at 6:01 PM,  <asvitkine@chromium.org> wrote:
>> > Then, why line 507 is "else if" instead of "else"?
>
>> Because it only needs to do "ch--" if the index is at a grapheme boundary.
>> If
>> it's not at the grapheme boundary (i.e. in the middle of a grapheme), then
>> the
>> loop will do the right thing already.
>
> Ah, I see what you're saying. I think you're right, it can be just an
> "else",
> since the "ch--" won't cause any harm in the other case (it will be
> essentially
> equivalent to the first iteration of the loop).
>
> http://codereview.chromium.org/8575020/
>

So in fact, I think I can then change the code to:

    if (ch == 0) {
       if (run_index == 0)
         return 0;
       run = runs_[run_index - 1];
       start = run->range.start();
       length = run->range.length();
       ch = length;
     }

     // Loop to find the start of the grapheme.
     do {
       ch--;
     } while (ch > 0 && run->logical_clusters[ch - 1] ==
run->logical_clusters[ch]);

I'll give this a try tomorrow.

[Alexei Svitkine (slow), 2011-12-02 14:40:28]

I've simplified the code per our discussion; I was also able to get rid of
std::max(std::min()) on the return value, since the loops now guarantee that
|ch| won't go out of bounds.

PTAL.

[xji, 2011-12-02 18:09:19]

On 2011/12/02 14:40:28, Alexei Svitkine wrote:
> I've simplified the code per our discussion; I was also able to get rid of
> std::max(std::min()) on the return value, since the loops now guarantee that
> |ch| won't go out of bounds.
> 
> PTAL.

Great. LGTM.

[msw, 2011-12-03 00:03:58]

LGTM with nits. Thanks!

http://codereview.chromium.org/8575020/diff/36007/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

http://codereview.chromium.org/8575020/diff/36007/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:547: scoped_ptr<RenderText>
render_text(RenderText::CreateRenderText());
Move this down to just above the loop where it's used.

http://codereview.chromium.org/8575020/diff/36007/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:616: size_t expected_next;
expected_next isn't used.

http://codereview.chromium.org/8575020/diff/36007/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/8575020/diff/36007/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:472: size_t
RenderTextWin::IndexOfAdjacentGrapheme(size_t index, bool next) {
Eh, this isn't very clean, but it seems to get the job done.

http://codereview.chromium.org/8575020/diff/36007/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:514: while (ch < length && run->logical_clusters[ch]
== cluster)
nit: Inline |run->range.length()| here and remove the |length| local.

[commit-bot: I haz the power, 2011-12-05 14:43:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/8575020/45001

[commit-bot: I haz the power, 2011-12-05 16:11:27]

Change committed as 112983

[Alexei Svitkine (slow), 2011-12-05 18:21:53]

On 2011/12/05 16:11:27, I haz the power (commit-bot) wrote:
> Change committed as 112983

Sigh. GraphemePositions test and EditString_ComplexScript fail on XP - so this
was reverted. :(

I'm not sure what the solution is here, since XP won't have the right fonts etc.
for some of these scripts. I guess we can runtime-exclude the tests under XP....
What do you think?

[xji, 2011-12-05 18:35:22]

On 2011/12/05 18:21:53, Alexei Svitkine wrote:
> On 2011/12/05 16:11:27, I haz the power (commit-bot) wrote:
> > Change committed as 112983
> 
> Sigh. GraphemePositions test and EditString_ComplexScript fail on XP - so this
> was reverted. :(
> 
> I'm not sure what the solution is here, since XP won't have the right fonts
etc.
> for some of these scripts. 

So, those characters are displayed as empty square in XP?
(because those language packages need to be manually installed in XP, but they
are not in buildbot?)
If that is the case, I am fine with exclude the test under XP with comments.

Those characters might appear in other (not-yet-enabled-for-windows) tests as
well.

BTW, seems that only \x915\x93f are used in GraphemePositions, and they are used
in SelctionModels as well, why SelectionModels does not fail?


>I guess we can runtime-exclude the tests under XP....
> What do you think?

[msw, 2011-12-05 18:44:29]

Please file a bug and leave a comment if you disable the test on XP.
Let me know if you need help remote debugging this code on a Hive XP VM.

[Alexei Svitkine (slow), 2011-12-05 20:03:38]

So, some more info:

SelectionModels also fails under XP in the same way as GraphemeBoundaries.

In the case of these two test failures, they are actually caused by
this DCHECK() in RenderTextWin::LayoutVisualText(), after the
ScriptShape() loop:

    DCHECK(SUCCEEDED(hr));

Debugging this further shows that this hits the case of (hr ==
USP_E_SCRIPT_NOT_IN_FONT), even after choosing a fallback font - as I
suspected.

Interestingly, if I just comment out that DCHECK(), the RenderTextTest
tests run successfully and pass on XP. So I am thinking, since that is
indeed an expected case (e.g. on XP or in other cases where some fonts
may not be installed), that the correct fix is to just reset hr to 0
in that case, to not trigger the DCHECK(), i.e.:

if (tried_fallback) {
  run->script_analysis.eScript = SCRIPT_UNDEFINED;
  hr = 0;
  break;
}

Unfortunately, the same is not true for views test
EditString_ComplexScript, where cursor positions are actually
different due to different grapheme boundaries being reported on XP
for those strings and as a result some MoveCursorTo() calls failing.
So for these, I think we can disable the tests with a corresponding
crbug.

Mike/Xiaomei, how does that sound to you?

[msw, 2011-12-05 20:18:53]

On 2011/12/05 20:03:38, Alexei Svitkine wrote:
> So, some more info:
> 
> SelectionModels also fails under XP in the same way as GraphemeBoundaries.
> 
> In the case of these two test failures, they are actually caused by
> this DCHECK() in RenderTextWin::LayoutVisualText(), after the
> ScriptShape() loop:
> 
>     DCHECK(SUCCEEDED(hr));
> 
> Debugging this further shows that this hits the case of (hr ==
> USP_E_SCRIPT_NOT_IN_FONT), even after choosing a fallback font - as I
> suspected.
> 
> Interestingly, if I just comment out that DCHECK(), the RenderTextTest
> tests run successfully and pass on XP. So I am thinking, since that is
> indeed an expected case (e.g. on XP or in other cases where some fonts
> may not be installed), that the correct fix is to just reset hr to 0
> in that case, to not trigger the DCHECK(), i.e.:
> 
> if (tried_fallback) {
>   run->script_analysis.eScript = SCRIPT_UNDEFINED;
>   hr = 0;
>   break;
> }
> 
> Unfortunately, the same is not true for views test
> EditString_ComplexScript, where cursor positions are actually
> different due to different grapheme boundaries being reported on XP
> for those strings and as a result some MoveCursorTo() calls failing.
> So for these, I think we can disable the tests with a corresponding
> crbug.
> 
> Mike/Xiaomei, how does that sound to you?

Please only reset hr when it equals USP_E_SCRIPT_NOT_IN_FONT, and add a comment
there as to why; that is better than disabling more tests, thanks!

Do the cursor positions for EditString_ComplexScript seem wrong or just
different, via some open-ended rules? Would it be better to skip certain checks
or hold different expectations on XP? Either way, please do file a bug and add a
comment. Thanks!

[Alexei Svitkine (slow), 2011-12-05 20:54:20]

> Please only reset hr when it equals USP_E_SCRIPT_NOT_IN_FONT, and add a
comment
> there as to why; that is better than disabling more tests, thanks!

Done. PTAL.

> Do the cursor positions for EditString_ComplexScript seem wrong or just
> different, via some open-ended rules? Would it be better to skip certain
checks
> or hold different expectations on XP? Either way, please do file a bug and add
a
> comment. Thanks!

The failure happens somewhere in model.MoveCursorTo(), resulting in the cursor
not moving as expected, per the test. I've filed a bug and added the runtime
check + comments. Are you okay with deferring further investigation of this to
that bug?

[msw, 2011-12-05 21:02:37]

LGTM, thanks for being thorough!

[commit-bot: I haz the power, 2011-12-05 21:11:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/8575020/53002

[xji, 2011-12-05 21:17:43]

On 2011/12/05 21:02:37, msw wrote:
> LGTM, thanks for being thorough!

LGTM. Thanks for carrying this through!
BTW, I just talked to Jungshik, we should have those language packages installed
in buildbot, maybe it was missed in some. Will talk to manuel about it.

[commit-bot: I haz the power, 2011-12-06 00:32:34]

Change committed as 113075

[Alexei Svitkine (slow), 2011-12-06 02:00:55]

On 2011/12/06 00:32:34, I haz the power (commit-bot) wrote:
> Change committed as 113075

And this failed on XP again... Apparently my local XP VM is not representative
of the XP on our build bots.

Failure links:

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

[msw, 2011-12-06 02:12:17]

Bummer, sorry you've had a rough time with that.
Perhaps delay landing until after the M17 branch day madness.

[msw, 2011-12-06 02:12:47]

On 2011/12/06 02:12:17, msw wrote:
> Bummer, sorry you've had a rough time with that.
> Perhaps delay landing until after the M17 branch day madness.

PS, those links are broken for me.

[Alexei Svitkine (slow), 2011-12-06 16:04:42]

> PS, those links are broken for me.

Looks like a Rietveld bug, I've filed a bug:

http://code.google.com/p/rietveld/issues/detail?id=349

Here are shortened links:

http://goo.gl/mxdjg
http://goo.gl/fsKQo
http://goo.gl/lojPv
http://goo.gl/PCgMB

Unfortunately, I can't reproduce these failures on either the "XP
Mode" Win7 VM (which is essentially a VirtualPC VM), or a Hive XP VM.
I asked maruel@, and he says our XP bots only have en-US with no
language packs installed.

However, from what I can tell, my Hive instances is the same, but the
tests pass there. :(

[Alexei Svitkine (slow), 2011-12-06 16:19:26]

Updated CL disabled both RenderTextTest.GraphemePositions and
RenderTextTest.SelectionModels on XP and disabled another check in
TextfieldViewsModelTest.EditString_ComplexScript under XP.

From the build failure output, this should fix the failing on XP. Unfortunately,
I have not found a way to test that this is indeed the case, since I am unable
to reproduce these failures on either of the two XP VMs I've tried.

I agree that at this point, its better to wait until things settle down after
the M17 branch before trying to re-land anything. I also welcome any ideas on
how I can reproduce what happens on the XP test bot.

[msw, 2011-12-06 18:07:59]

That's a shame, but I think we're out of options besides disabling for now.
We should try some manual testing once the change hits canary. LGTM.

[xji, 2011-12-06 19:05:43]

LGTM. Sorry to hear the trouble...