fix know issues in RenderText

This is a reapply of 
http://src.chromium.org/viewvc/chrome?view=rev&revision=102006

fix know issues in RenderText

1. add tests.
2. change SelectWord() to use BreakIterator, so it works for Chinese and Complex script.
3. DELETE/ReplaceChar delete/replace a whole grapheme. ReplaceTextInternal should only replace one grapheme when there is no selection.
4. pointing to position outside of text returns HOME/END position.
5. based on Chrome Linux omnibox and gedit, given 
"abc|   def", double click should select "   " instead of "abc". Change test expectation.

BUG=90426
TEST=compile with touchui=1 test omnibox. run views_unittests.NativeTextfieldViewsTest/TextfieldViewsModelTest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102160

[xji, 2011-09-09 01:08:27]

Note: some tests will fail in Win. I am thinking to disable them temporarily and
fix them after Linux version is completed.

[behdad_google, 2011-09-09 18:12:11]

LGTM

[msw, 2011-09-09 23:03:23]

Great tests and fixes, thanks! See comments.

Some of the tests might be better suited as RenderTextTests (it's good design to
limit the behavioral unit tests to the smallest component that they depend on),
but a second opinion from another reviewer is appreciated.

Please add BUG= and TEST= to the CL description.

http://codereview.chromium.org/7841056/diff/1/base/i18n/break_iterator.h
File base/i18n/break_iterator.h (right):

http://codereview.chromium.org/7841056/diff/1/base/i18n/break_iterator.h#newc...
base/i18n/break_iterator.h:93: // BREAK_NEWLINE modes,
use a period instead of a comma at the end of this comment.

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

http://codereview.chromium.org/7841056/diff/1/ui/gfx/render_text.h#newcode199
ui/gfx/render_text.h:199: bool MoveCursorTo(const SelectionModel&
selection_model);
Is it possible that the text could end with a non-cursorable position? For
example, if the logical text ends with the first half of a surrogate pair? What
happens then?

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

http://codereview.chromium.org/7841056/diff/1/ui/gfx/render_text_linux.cc#new...
ui/gfx/render_text_linux.cc:133: // When points outside of text, return HOME/END
position.
"When *the point is* outside" or "*For* points outside"

http://codereview.chromium.org/7841056/diff/1/ui/gfx/render_text_linux.cc#new...
ui/gfx/render_text_linux.cc:264: return (position >= 0 && position <
(size_t)num_log_attrs_ &&
Use c++ style static_cast.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:265: void
MouseClick(const gfx::Rect bound, int x_offset) {
Why not use a Point instead of a Rect? Use Rect::origin() as needed.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:270: MouseEvent
release(ui::ET_MOUSE_RELEASED, 0, 0, ui::EF_LEFT_BUTTON_DOWN);
The release event should probably be at the same x and y coordinates.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:274: void
NonClientMouseClick() {
Using this to avoid double/triple click is pretty ingenious. Can you add a
comment to clarify that purpose here?

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1199:
TEST_F(NativeTextfieldViewsTest, TextCursorDisplayTest) {
Some of these tests might be better suited as RenderTextTests.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1213:
SendKeyEvent(0x05E1);
Defining constants (if none exist) would be preferable to using un-explained
literals throughout this code.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1224:
SendKeyEvent(ui::VKEY_A, false, true);
You can just call SetText here and elsewhere that you clear the textfield.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1279:
SendKeyEvent(ui::VKEY_A, false, true);
SetText

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1354:
MouseClick(cursor_bounds[i + 1], -(half_width / 2));
space between '-' and '('

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1374:
MouseClick(bound, 10);
Is 10 pixels really longer than the text width?
Using the text width plus some buffer would be more robust.
Same for other 10 pixel literals below.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1445: void
OverflowCursorBoundTestVerifier(const gfx::Rect& display,
A comment would be appreciated:
This verifies that |bound| is contained by |display|.
|bound|'s right edge must be less than |diaplay|'s right edge.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1446: const
gfx::Rect& bound) {
Indent this line to match the line above.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1459:
SendKeyEvent(0x05E1);
Don't we have a bug where on overflow from LTR text, typing an RTL character
doesn't show the full character typed?

What's the intent of this aspect of these tests?

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1469:
SendKeyEvent(ui::VKEY_A, false, true);
SetText

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1501:
SendKeyEvent(ui::VKEY_A, false, true);
SetText

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:107:
model.Append(WideToUTF16(L"\x05e0\x05e1\x05e2"));
Perhaps make the second string different than the first so we can verify that it
was appended and not pre-pended?

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:161: // DELETE should
delte the whole grapheme.
"Delete" isn't all-caps and "del*e*te".

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:171: // Test cursor
position and deletion for Hindi VIRAMA.
Virama isn't all-caps.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:190: // Test
DELETE/BACKSPACE on Hebrew with non-spacing marks.
Delete and Backspace are not all-caps.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:200:
model.MoveCursorLeft(gfx::LINE_BREAK, false);
Won't this fail on windows if the UI mode isn't RTL?

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:210: for (int i = 0;
i <= num_of_selection_entry + 1; ++i) {
Why are you looping i from 0 to n+1 but then clamping index to n-1? Why not just
loop i to n-1?

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:212: struct Selection
sel = expected[index];
I don't believe 'struct' is needed here and elsewhere (I didn't mark each use).
But hopefully you'll use std::vector<gfx::SelectionModel> instead anyway.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:238: struct Selection
left_to_right[] = {
Use std::vector<gfx::SelectionModel> instead here and elsewhere. Example:
std::vector<gfx::SelectionModel> ltr;
ltr.append(gfx::SelectionModel(0, 0, gfx::SelectionModel::TRAILING));
ltr.append(gfx::SelectionModel(1, 0, gfx::SelectionModel::LEADING));
...

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:375:
model.Append(WideToUTF16(L"\x0915\x093f\x0915\x094d\x0915"));
Any reason you chose not to use RunMoveCursorLeftRightTest and also verify the
caret positions here?

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:469:
EXPECT_EQ(WideToUTF16(L"c"), model.GetSelectedText());
This is the spec'ed behavior, but it might confuse someone, can you add a
comment?

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:525:
model.SetText(WideToUTF16(L"abc\x05d0\x05d1\x05d2"));
Please add EXPECT_EQs for model.GetCursorPosition for each step of this test, or
add comments to explain what this is doing.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:541: 
nit: The very similar block above doesn't have a blank line like this. Can you
make both of them similar one way or the other?

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:745: // Test when
cursor is somewhere in a non-alph-numeric fragment.
nit: mind fixing "alph*a*"?

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:770: }
words_and_cursor[] = {
I suppose this works, but would you mind splitting the struct definition from
the words_and_cursor instance, just for readability? Or could you use a vector
of WordAndCursor?

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:785: // The text
consists of Ascii, Hebrew, Hindi with  Virama sign, and Chinese.
Extra space in "with * *Virama"

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:788: for (size_t i =
0; i < sizeof(words_and_cursor)/sizeof(WordAndCursor);
Wouldn't it be a bit clearer to remove the WordAndCursor struct and this loop;
and to just make individual calls to SelectWordTestVerifier for each index?

[xji, 2011-09-12 21:31:48]

http://codereview.chromium.org/7841056/diff/1/base/i18n/break_iterator.h
File base/i18n/break_iterator.h (right):

http://codereview.chromium.org/7841056/diff/1/base/i18n/break_iterator.h#newc...
base/i18n/break_iterator.h:93: // BREAK_NEWLINE modes,
On 2011/09/09 23:03:24, msw wrote:
> use a period instead of a comma at the end of this comment.

Done.

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

http://codereview.chromium.org/7841056/diff/1/ui/gfx/render_text.h#newcode199
ui/gfx/render_text.h:199: bool MoveCursorTo(const SelectionModel&
selection_model);
On 2011/09/09 23:03:24, msw wrote:
> Is it possible that the text could end with a non-cursorable position? For
> example, if the logical text ends with the first half of a surrogate pair?
What
> happens then?

We do not allow that.
When I choose whether to use u_strToUTF8 or u_strToUTF8WithSub for utf8 and
utf16 conversion, I talked with Junghsik. And we think since we are dealing with
text field, user either input surrogate pair directly, in which, the IME should
not allow half surrogate pair, or if user paste text, the pasted text would be
garbage character if the source contains half surrogate.
So, I used u_strToUTF8 and assert that there wont be INVALID  character in the
source string.

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

http://codereview.chromium.org/7841056/diff/1/ui/gfx/render_text_linux.cc#new...
ui/gfx/render_text_linux.cc:133: // When points outside of text, return HOME/END
position.
On 2011/09/09 23:03:24, msw wrote:
> "When *the point is* outside" or "*For* points outside"

Done.

http://codereview.chromium.org/7841056/diff/1/ui/gfx/render_text_linux.cc#new...
ui/gfx/render_text_linux.cc:264: return (position >= 0 && position <
(size_t)num_log_attrs_ &&
On 2011/09/09 23:03:24, msw wrote:
> Use c++ style static_cast.

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:265: void
MouseClick(const gfx::Rect bound, int x_offset) {
On 2011/09/09 23:03:24, msw wrote:
> Why not use a Point instead of a Rect? Use Rect::origin() as needed.

I need rect.height() as well.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:270: MouseEvent
release(ui::ET_MOUSE_RELEASED, 0, 0, ui::EF_LEFT_BUTTON_DOWN);
On 2011/09/09 23:03:24, msw wrote:
> The release event should probably be at the same x and y coordinates.

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:274: void
NonClientMouseClick() {
On 2011/09/09 23:03:24, msw wrote:
> Using this to avoid double/triple click is pretty ingenious. Can you add a
> comment to clarify that purpose here?

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1199:
TEST_F(NativeTextfieldViewsTest, TextCursorDisplayTest) {
On 2011/09/09 23:03:24, msw wrote:
> Some of these tests might be better suited as RenderTextTests.

Looking at those tests again, I think in NativeTextfieldViewsTest, other tests
have mouse clicking event and could not be moved to RenderTextTest except
TextCursorDisplay(_inRTL)Test. But if running in RenderTextTest, SetText() has
to be used along with SetCursorPosition() in order for the cursor bounds to be
displayed correctly. So, I feel all the tests here probably should stay.

In TextfieldViewsModelTest, those tests that explicitly reset cursor position
after set text, and they tests pure render text operations (such as
MoveCursorLeft/Right when there is no composition text involved) should move to
RenderTextTest. So, I am moving MoveCursorLeft/Right to RenderTextTest. 

Looks like I can also move Selection and SelectWord to RenderTextTest but then I
will need to test the selection range. I prefer to test the string so I leave
selection related tests untouched.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1213:
SendKeyEvent(0x05E1);
On 2011/09/09 23:03:24, msw wrote:
> Defining constants (if none exist) would be preferable to using un-explained
> literals throughout this code.

Hm... I tried that. But that makes the subsequent EXPECT_EQ not clear enough
unless change the subsequent EXPECT_EQ to use string append first.
So, I reverted it and only use the const when I do not care about the text
itself.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1224:
SendKeyEvent(ui::VKEY_A, false, true);
On 2011/09/09 23:03:24, msw wrote:
> You can just call SetText here and elsewhere that you clear the textfield.

I am trying to emulate the real UI operations. Maybe I should leave it as is.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1354:
MouseClick(cursor_bounds[i + 1], -(half_width / 2));
On 2011/09/09 23:03:24, msw wrote:
> space between '-' and '('

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1374:
MouseClick(bound, 10);
On 2011/09/09 23:03:24, msw wrote:
> Is 10 pixels really longer than the text width?
> Using the text width plus some buffer would be more robust.
> Same for other 10 pixel literals below.

10 pixel is the x_offset from END position's x-axis.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1445: void
OverflowCursorBoundTestVerifier(const gfx::Rect& display,
On 2011/09/09 23:03:24, msw wrote:
> A comment would be appreciated:
> This verifies that |bound| is contained by |display|.
> |bound|'s right edge must be less than |diaplay|'s right edge.

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1446: const
gfx::Rect& bound) {
On 2011/09/09 23:03:24, msw wrote:
> Indent this line to match the line above.

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views_unittest.cc:1459:
SendKeyEvent(0x05E1);
On 2011/09/09 23:03:24, msw wrote:
> Don't we have a bug where on overflow from LTR text, typing an RTL character
> doesn't show the full character typed?
> 
> What's the intent of this aspect of these tests?

Yes we do have that bug.
This test does not test whether the character displays inside display_rect. It
only test the cursor bound is inside the range.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:107:
model.Append(WideToUTF16(L"\x05e0\x05e1\x05e2"));
On 2011/09/09 23:03:24, msw wrote:
> Perhaps make the second string different than the first so we can verify that
it
> was appended and not pre-pended?

they are different.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:161: // DELETE should
delte the whole grapheme.
On 2011/09/09 23:03:24, msw wrote:
> "Delete" isn't all-caps and "del*e*te".

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:171: // Test cursor
position and deletion for Hindi VIRAMA.
On 2011/09/09 23:03:24, msw wrote:
> Virama isn't all-caps.

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:190: // Test
DELETE/BACKSPACE on Hebrew with non-spacing marks.
On 2011/09/09 23:03:24, msw wrote:
> Delete and Backspace are not all-caps.

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:200:
model.MoveCursorLeft(gfx::LINE_BREAK, false);
On 2011/09/09 23:03:24, msw wrote:
> Won't this fail on windows if the UI mode isn't RTL?

add #if defined and use move to right line end for Win.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:210: for (int i = 0;
i <= num_of_selection_entry + 1; ++i) {
On 2011/09/09 23:03:24, msw wrote:
> Why are you looping i from 0 to n+1 but then clamping index to n-1? Why not
just
> loop i to n-1?

It is to test when cursor is at the HOME/END, press right/left again, cursor
should stay at the same place.
Actually, the condition should be "i <= num_of_selection_entry", offset by 1
should be enough.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:212: struct Selection
sel = expected[index];
On 2011/09/09 23:03:24, msw wrote:
> I don't believe 'struct' is needed here and elsewhere (I didn't mark each
use).
> But hopefully you'll use std::vector<gfx::SelectionModel> instead anyway.

removed struct.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:238: struct Selection
left_to_right[] = {
On 2011/09/09 23:03:24, msw wrote:
> Use std::vector<gfx::SelectionModel> instead here and elsewhere. Example:
> std::vector<gfx::SelectionModel> ltr;
> ltr.append(gfx::SelectionModel(0, 0, gfx::SelectionModel::TRAILING));
> ltr.append(gfx::SelectionModel(1, 0, gfx::SelectionModel::LEADING));
> ...

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:375:
model.Append(WideToUTF16(L"\x0915\x093f\x0915\x094d\x0915"));
On 2011/09/09 23:03:24, msw wrote:
> Any reason you chose not to use RunMoveCursorLeftRightTest and also verify the
> caret positions here?

since this is uni-directional text, I am not testing caret_pos here, only test
cursor logical position, which is simpler.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:469:
EXPECT_EQ(WideToUTF16(L"c"), model.GetSelectedText());
On 2011/09/09 23:03:24, msw wrote:
> This is the spec'ed behavior, but it might confuse someone, can you add a
> comment?

added comments and selection_start/end expected results. hope it explains.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:525:
model.SetText(WideToUTF16(L"abc\x05d0\x05d1\x05d2"));
On 2011/09/09 23:03:24, msw wrote:
> Please add EXPECT_EQs for model.GetCursorPosition for each step of this test,
or
> add comments to explain what this is doing.

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:541: 
On 2011/09/09 23:03:24, msw wrote:
> nit: The very similar block above doesn't have a blank line like this. Can you
> make both of them similar one way or the other?
removed blank line.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:745: // Test when
cursor is somewhere in a non-alph-numeric fragment.
On 2011/09/09 23:03:24, msw wrote:
> nit: mind fixing "alph*a*"?

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:770: }
words_and_cursor[] = {
On 2011/09/09 23:03:24, msw wrote:
> I suppose this works, but would you mind splitting the struct definition from
> the words_and_cursor instance, just for readability? Or could you use a vector
> of WordAndCursor?

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:785: // The text
consists of Ascii, Hebrew, Hindi with  Virama sign, and Chinese.
On 2011/09/09 23:03:24, msw wrote:
> Extra space in "with * *Virama"

Done.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:788: for (size_t i =
0; i < sizeof(words_and_cursor)/sizeof(WordAndCursor);
On 2011/09/09 23:03:24, msw wrote:
> Wouldn't it be a bit clearer to remove the WordAndCursor struct and this loop;
> and to just make individual calls to SelectWordTestVerifier for each index?

that is true. But the current one also tests the selected word when cursor at
different visual position but the same logical position (index). For example,
"a\x05d0", when index is 1, visual position at "a|\x05d2\x05d1 \x05d0" and
"a|\x05d2\x05d1 \x05d0|"

[msw, 2011-09-14 02:43:48]

Thanks for moving some tests to RenderText!

If you can use more constants to define the uniscribe character hex codes that
you use, that'd be awesome, but don't worry if it's a lot of work.

I hope it's not a lot of work to fix the tests after you fix the Windows build.

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/7841056/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield_views_model_unittest.cc:107:
model.Append(WideToUTF16(L"\x05e0\x05e1\x05e2"));
On 2011/09/12 21:31:48, xji wrote:
> On 2011/09/09 23:03:24, msw wrote:
> > Perhaps make the second string different than the first so we can verify
that
> it
> > was appended and not pre-pended?
> 
> they are different.

doh! you are right.

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

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:460: return std::min(static_cast<long>(position + 1),
Both of these static casts have lint errors:
"Use int16/int64/etc, rather than the C type long"
If it's an easy fix, please also fix RenderText::GetIndexOfPreviousGrapheme().

Or, you could take this opportunity to merge the win/linux impls that just call
GetIndexOfAdjacentGrapheme (with true/false and left/right) into these base
impls. I think you can make RenderText::GetIndexOfAdjacentGrapheme pure virtual
and just leave the implementations to RenderText[Win/Linux].

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:578: return true;
I think this can potentially be pure virtual, and leave the impls to win/linux.

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

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:185: for (int i = 0; i <= num_of_selection_entry;
++i) {
num_of_selection_entry is no longer needed, you can use expected.size().

I think if you're trying to test the cursor position after moving left/right at
the ends, you should just add an additional selectionmodel to the vector that
you pass in, and add a comment where that's done to explain that the final
member(s) are meant to test the clamped line ends. You'll need to change '<=' to
'<' when you do that and you won't need |index|, the max() call, or the
redundant section outside this loop.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:196: SelectionModel sel =
expected[num_of_selection_entry - 1];
I'm confused why you have this section of code in addition to the unconventional
indicies/max operation above; as both apparently check the line ends. If you
implement my suggestion above, you shouldn't need this.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:204: TEST_F(RenderTextTest, MoveCursorLeftRight)
{
You should split this test up for each different test string, smaller tests is
better, especially if we need different behavior on Linux and Windows.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:209: // left_to_right saves the expected
SelectionModel when moving cursor
|left_to_right| is now |ltr|.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:215: ltr.push_back(SelectionModel(3, 2,
SelectionModel::TRAILING));
As an example of my above suggestions to check the line ends, here you could
just add another (3,2,trailing) to ensure that you can't move past the expected
line end. You can add a comment if you think readers would be confused by that
pattern.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:222: rtl.push_back(SelectionModel(0, 0,
SelectionModel::LEADING));
similarly, add an extra (0,0,leading).

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:244: rtl.clear();
This isn't necessarily important, but you flipped the meaning of |rtl| and |ltr|
here and in the block of ltr immediately below. (You're using rtl with |true|
for move right, etc.). Perhaps you could use a directionally ambiguous name for
the expected selectionmodel vector, like |expected|.

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

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:302: bool RenderTextLinux::IsCursorablePosition(size_t
position) {
RenderText*Win*

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:308: if (!run)
This could just be "if (run_index >= runs_.size()) /n return false;" followed up
by "internal::TextRun* run = runs_[run_index];"

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:312: return run->logical_clusters[position] !=
run->logical_clusters[position -1];
The logical clusters array is indexed by offsets from the run start.
This will crash unless you first subtract run->range.start() from position.
This needs a space at "[position -* *1]"

http://codereview.chromium.org/7841056/diff/17001/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/7841056/diff/17001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model_unittest.cc:25: const wchar_t*
word;
Data members belong below the ctor.

[xji, 2011-09-15 23:38:13]

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

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:460: return std::min(static_cast<long>(position + 1),
On 2011/09/14 02:43:48, msw wrote:
> Both of these static casts have lint errors:
> "Use int16/int64/etc, rather than the C type long"
> If it's an easy fix, please also fix RenderText::GetIndexOfPreviousGrapheme().
> 
> Or, you could take this opportunity to merge the win/linux impls that just
call
> GetIndexOfAdjacentGrapheme (with true/false and left/right) into these base
> impls. I think you can make RenderText::GetIndexOfAdjacentGrapheme pure
virtual
> and just leave the implementations to RenderText[Win/Linux].

Merged win/linux impls.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:578: return true;
On 2011/09/14 02:43:48, msw wrote:
> I think this can potentially be pure virtual, and leave the impls to
win/linux.

Done.

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

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:185: for (int i = 0; i <= num_of_selection_entry;
++i) {
On 2011/09/14 02:43:48, msw wrote:
> num_of_selection_entry is no longer needed, you can use expected.size().
> 
> I think if you're trying to test the cursor position after moving left/right
at
> the ends, you should just add an additional selectionmodel to the vector that
> you pass in, and add a comment where that's done to explain that the final
> member(s) are meant to test the clamped line ends. You'll need to change '<='
to
> '<' when you do that and you won't need |index|, the max() call, or the
> redundant section outside this loop.

Done.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:196: SelectionModel sel =
expected[num_of_selection_entry - 1];
On 2011/09/14 02:43:48, msw wrote:
> I'm confused why you have this section of code in addition to the
unconventional
> indicies/max operation above; as both apparently check the line ends. If you
> implement my suggestion above, you shouldn't need this.

The above loop checks move by character.
This one checks move by line boundary.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:204: TEST_F(RenderTextTest, MoveCursorLeftRight)
{
On 2011/09/14 02:43:48, msw wrote:
> You should split this test up for each different test string, smaller tests is
> better, especially if we need different behavior on Linux and Windows.

Done.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:209: // left_to_right saves the expected
SelectionModel when moving cursor
On 2011/09/14 02:43:48, msw wrote:
> |left_to_right| is now |ltr|.

Done.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:215: ltr.push_back(SelectionModel(3, 2,
SelectionModel::TRAILING));
On 2011/09/14 02:43:48, msw wrote:
> As an example of my above suggestions to check the line ends, here you could
> just add another (3,2,trailing) to ensure that you can't move past the
expected
> line end. You can add a comment if you think readers would be confused by that
> pattern.

Done.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:222: rtl.push_back(SelectionModel(0, 0,
SelectionModel::LEADING));
On 2011/09/14 02:43:48, msw wrote:
> similarly, add an extra (0,0,leading).

Done.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:244: rtl.clear();
On 2011/09/14 02:43:48, msw wrote:
> This isn't necessarily important, but you flipped the meaning of |rtl| and
|ltr|
> here and in the block of ltr immediately below. (You're using rtl with |true|
> for move right, etc.). Perhaps you could use a directionally ambiguous name
for
> the expected selectionmodel vector, like |expected|.

Done.

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

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:302: bool RenderTextLinux::IsCursorablePosition(size_t
position) {
On 2011/09/14 02:43:48, msw wrote:
> RenderText*Win*

Done.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:308: if (!run)
On 2011/09/14 02:43:48, msw wrote:
> This could just be "if (run_index >= runs_.size()) /n return false;" followed
up
> by "internal::TextRun* run = runs_[run_index];"

Done.

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:312: return run->logical_clusters[position] !=
run->logical_clusters[position -1];
On 2011/09/14 02:43:48, msw wrote:
> The logical clusters array is indexed by offsets from the run start.
> This will crash unless you first subtract run->range.start() from position.
> This needs a space at "[position -* *1]"

Done.

http://codereview.chromium.org/7841056/diff/17001/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/7841056/diff/17001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model_unittest.cc:25: const wchar_t*
word;
On 2011/09/14 02:43:48, msw wrote:
> Data members belong below the ctor.

Done.

[msw, 2011-09-16 17:39:08]

Mostly nits.

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

http://codereview.chromium.org/7841056/diff/17001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:196: SelectionModel sel =
expected[num_of_selection_entry - 1];
On 2011/09/15 23:38:13, xji wrote:
> On 2011/09/14 02:43:48, msw wrote:
> > I'm confused why you have this section of code in addition to the
> unconventional
> > indicies/max operation above; as both apparently check the line ends. If you
> > implement my suggestion above, you shouldn't need this.
> 
> The above loop checks move by character.
> This one checks move by line boundary.

Ah, I missed that, thanks.

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

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:276: size_t utf8_index_of_current_grapheme, bool
next) const {
|next| belongs on the following line, our style guide dictates that arguments
can only share a line if the entire function signature fits on one line.

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:301: size_t utf8_index_of_current_grapheme, bool
next) const {
|next| on next line.

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

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:206: // ltr saves the expected SelectionModel
when moving cursor from left to
|ltr| doesn't exist anymore. Perhaps just write "Save the expected ..."

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:276: // TODO(xji): temporarily disable in
platform Win.
Do you need to disable before landing? Is this TODO old?
Do we expect to unify the Linux and Win behavior, or are they expected to act
differently, that would be good to mention in a TODO here.

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:298: // expected.push_back(SelectionModel(0, 2,
SelectionModel::TRAILING));
Did you mean to leave this commented line of code here?

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:348: // expected.push_back(SelectionModel(4, 3,
SelectionModel::TRAILING));
4,3,T doesn't match your expected or actual, did you mean 0,2,T?

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:419: // characters turned into empty squre due to
font regression. So, not able
"squ*a*re", yeah, we need font fallback.

http://codereview.chromium.org/7841056/diff/25001/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/7841056/diff/25001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model_unittest.cc:28: }
Add a blank line between ctor and members.

http://codereview.chromium.org/7841056/diff/25001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model_unittest.cc:150: // characters
turned into empty squre due to font regression. So, not able
"squ*a*re" here and elsewhere.

[xji, 2011-09-16 20:29:24]

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

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:276: size_t utf8_index_of_current_grapheme, bool
next) const {
On 2011/09/16 17:39:08, msw wrote:
> |next| belongs on the following line, our style guide dictates that arguments
> can only share a line if the entire function signature fits on one line.

Done.

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:301: size_t utf8_index_of_current_grapheme, bool
next) const {
On 2011/09/16 17:39:08, msw wrote:
> |next| on next line.

Done.

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

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:206: // ltr saves the expected SelectionModel
when moving cursor from left to
On 2011/09/16 17:39:08, msw wrote:
> |ltr| doesn't exist anymore. Perhaps just write "Save the expected ..."

Done.

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:276: // TODO(xji): temporarily disable in
platform Win.
On 2011/09/16 17:39:08, msw wrote:
> Do you need to disable before landing? Is this TODO old?

The TODO is old.

> Do we expect to unify the Linux and Win behavior, or are they expected to act
> differently, that would be good to mention in a TODO here.

they are different since the text directionality is different. In Linux, text
directionality is based on 1st strong directionality character. In Windows, now
the text directionality is always LTR, which is a bug. We should change it to be
based on UI's directionality or 1st character's directionality.

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:298: // expected.push_back(SelectionModel(0, 2,
SelectionModel::TRAILING));
On 2011/09/16 17:39:08, msw wrote:
> Did you mean to leave this commented line of code here?

I'd prefer to leave it here.
It's the expected result, although the current Win behavior is wrong. We will
need to fix Win behavior.

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:348: // expected.push_back(SelectionModel(4, 3,
SelectionModel::TRAILING));
On 2011/09/16 17:39:08, msw wrote:
> 4,3,T doesn't match your expected or actual, did you mean 0,2,T?

You right, should be (0, 2, t)

http://codereview.chromium.org/7841056/diff/25001/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:419: // characters turned into empty squre due to
font regression. So, not able
On 2011/09/16 17:39:08, msw wrote:
> "squ*a*re", yeah, we need font fallback.

Done.

http://codereview.chromium.org/7841056/diff/25001/views/controls/textfield/te...
File views/controls/textfield/textfield_views_model_unittest.cc (right):

http://codereview.chromium.org/7841056/diff/25001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model_unittest.cc:28: }
On 2011/09/16 17:39:08, msw wrote:
> Add a blank line between ctor and members.

Done.

http://codereview.chromium.org/7841056/diff/25001/views/controls/textfield/te...
views/controls/textfield/textfield_views_model_unittest.cc:150: // characters
turned into empty squre due to font regression. So, not able
On 2011/09/16 17:39:08, msw wrote:
> "squ*a*re" here and elsewhere.

Done.

[msw, 2011-09-16 21:54:39]

LGTM; make sure trybots pass before landing!

[xji, 2011-09-16 23:02:24]

Hi Evan,

Could you please review the change in base/i18n?

Thanks,
xiaomei

[Evan Martin, 2011-09-16 23:09:20]

base LGTM

http://codereview.chromium.org/7841056/diff/25002/base/i18n/break_iterator.h
File base/i18n/break_iterator.h (right):

http://codereview.chromium.org/7841056/diff/25002/base/i18n/break_iterator.h#...
base/i18n/break_iterator.h:92: // at the start of word. It always retuns false
Under BREAK_LINE and
s/Under/under/

[xji, 2011-09-21 18:15:49]

patchset 8 fix clang compilation error in render_text_linux.cc:

ui/gfx/render_text_linux.cc:252:20: error: comparison of unsigned expression >=
0 is always true
 return (position >= 0 && position < static_cast<size_t>(num_log_attrs_) &&

changed to:
 return (position < static_cast<size_t>(num_log_attrs_) &&

[msw, 2011-09-21 18:16:55]

According to xji, the clang build error was:
ui/gfx/render_text_linux.cc:252:20: error: comparison of unsigned expression >=
0 is always true
The fix was:
return (position >= 0 && position < static_cast<size_t>(num_log_attrs_) &&
changed to:
return (position < static_cast<size_t>(num_log_attrs_) &&
fix LGTM