fix know issues in RenderText

ui/gfx/render_text_unittest.cc

Index: ui/gfx/render_text_unittest.cc
===================================================================
--- ui/gfx/render_text_unittest.cc	(revision 100008)
+++ ui/gfx/render_text_unittest.cc	(working copy)
@@ -15,14 +15,14 @@
 
 TEST_F(RenderTextTest, DefaultStyle) {
   // Defaults to empty text with no styles.
-  scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateRenderText());
+  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
   EXPECT_TRUE(render_text->text().empty());
   EXPECT_TRUE(render_text->style_ranges().empty());
 
   // Test that the built-in default style is applied for new text.
   render_text->SetText(ASCIIToUTF16("abc"));
   EXPECT_EQ(1U, render_text->style_ranges().size());
-  gfx::StyleRange style;
+  StyleRange style;
   EXPECT_EQ(style.font.GetFontName(),
             render_text->style_ranges()[0].font.GetFontName());
   EXPECT_EQ(style.foreground, render_text->style_ranges()[0].foreground);
@@ -38,8 +38,8 @@
 
 TEST_F(RenderTextTest, CustomDefaultStyle) {
   // Test a custom default style.
-  scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateRenderText());
-  gfx::StyleRange color;
+  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
+  StyleRange color;
   color.foreground = SK_ColorRED;
   render_text->set_default_style(color);
   render_text->SetText(ASCIIToUTF16("abc"));
@@ -54,7 +54,7 @@
   EXPECT_EQ(color.foreground, render_text->style_ranges()[0].foreground);
 
   // Test ApplyDefaultStyle after setting a new default.
-  gfx::StyleRange strike;
+  StyleRange strike;
   strike.strike = true;
   render_text->set_default_style(strike);
   render_text->ApplyDefaultStyle();
@@ -64,24 +64,24 @@
 }
 
 TEST_F(RenderTextTest, ApplyStyleRange) {
-  scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateRenderText());
+  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
   render_text->SetText(ASCIIToUTF16("01234"));
   EXPECT_EQ(1U, render_text->style_ranges().size());
 
   // Test ApplyStyleRange (no-op on empty range).
-  gfx::StyleRange empty;
+  StyleRange empty;
   empty.range = ui::Range(1, 1);
   render_text->ApplyStyleRange(empty);
   EXPECT_EQ(1U, render_text->style_ranges().size());
 
   // Test ApplyStyleRange (no-op on invalid range).
-  gfx::StyleRange invalid;
+  StyleRange invalid;
   invalid.range = ui::Range::InvalidRange();
   render_text->ApplyStyleRange(invalid);
   EXPECT_EQ(1U, render_text->style_ranges().size());
 
   // Apply a style with a range contained by an existing range.
-  gfx::StyleRange underline;
+  StyleRange underline;
   underline.underline = true;
   underline.range = ui::Range(2, 3);
   render_text->ApplyStyleRange(underline);
@@ -94,7 +94,7 @@
   EXPECT_FALSE(render_text->style_ranges()[2].underline);
 
   // Apply a style with a range equal to another range.
-  gfx::StyleRange color;
+  StyleRange color;
   color.foreground = SK_ColorWHITE;
   color.range = ui::Range(2, 3);
   render_text->ApplyStyleRange(color);
@@ -111,7 +111,7 @@
 
   // Apply a style with a range containing an existing range.
   // This new style also overlaps portions of neighboring ranges.
-  gfx::StyleRange strike;
+  StyleRange strike;
   strike.strike = true;
   strike.range = ui::Range(1, 4);
   render_text->ApplyStyleRange(strike);
@@ -124,7 +124,7 @@
   EXPECT_FALSE(render_text->style_ranges()[2].strike);
 
   // Apply a style overlapping all ranges.
-  gfx::StyleRange strike_underline;
+  StyleRange strike_underline;
   strike_underline.strike = true;
   strike_underline.underline = true;
   strike_underline.range = ui::Range(0, render_text->text().length());
@@ -144,7 +144,7 @@
 
 TEST_F(RenderTextTest, StyleRangesAdjust) {
   // Test that style ranges adjust to the text size.
-  scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateRenderText());
+  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
   render_text->SetText(ASCIIToUTF16("abcdef"));
   EXPECT_EQ(1U, render_text->style_ranges().size());
   EXPECT_EQ(ui::Range(0, 6), render_text->style_ranges()[0].range);
@@ -155,7 +155,7 @@
   EXPECT_EQ(ui::Range(0, 3), render_text->style_ranges()[0].range);
 
   // Test that the last range extends to the length of longer text.
-  gfx::StyleRange strike;
+  StyleRange strike;
   strike.strike = true;
   strike.range = ui::Range(2, 3);
   render_text->ApplyStyleRange(strike);
@@ -178,4 +178,212 @@
   EXPECT_FALSE(render_text->style_ranges()[0].strike);
 }
 
+void RunMoveCursorLeftRightTest(RenderText* render_text,
+    const std::vector<SelectionModel>& expected,
+    int num_of_selection_entry,
+    bool move_right) {
+  for (int i = 0; i <= num_of_selection_entry; ++i) {

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

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.

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

Done.

+    int index = std::min(i, num_of_selection_entry - 1);
+    SelectionModel sel = expected[index];
+    EXPECT_TRUE(render_text->selection_model().Equals(sel));
+
+    if (move_right)
+      render_text->MoveCursorRight(CHARACTER_BREAK, false);
+    else
+      render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+  }
+
+  SelectionModel sel = expected[num_of_selection_entry - 1];

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

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.

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

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

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

Ah, I missed that, thanks.

+  if (move_right)
+    render_text->MoveCursorRight(LINE_BREAK, false);
+  else
+    render_text->MoveCursorLeft(LINE_BREAK, false);
+  EXPECT_TRUE(render_text->selection_model().Equals(sel));
+}
+
+TEST_F(RenderTextTest, MoveCursorLeftRight) {

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

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.

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

Done.

+  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
+
+  // Pure LTR.
+  render_text->SetText(ASCIIToUTF16("abc"));
+  // left_to_right saves the expected SelectionModel when moving cursor

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

|left_to_right| is now |ltr|.

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

Done.

+  // from left to right.
+  std::vector<SelectionModel> ltr;
+  ltr.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(1, 0, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(2, 1, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(3, 2, SelectionModel::TRAILING));

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

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.

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

Done.

+  RunMoveCursorLeftRightTest(render_text.get(), ltr, ltr.size(), true);
+
+  std::vector<SelectionModel> rtl;
+  rtl.push_back(SelectionModel(3, 2, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(2, 2, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(1, 1, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(0, 0, SelectionModel::LEADING));

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

similarly, add an extra (0,0,leading).

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

Done.

+  RunMoveCursorLeftRightTest(render_text.get(), rtl, rtl.size(), false);
+
+  // Pure RTL.
+  render_text->SetText(WideToUTF16(L"\x05d0\x05d1\x05d2"));
+  rtl.clear();
+  rtl.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(1, 0, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(2, 1, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(3, 2, SelectionModel::TRAILING));
+  RunMoveCursorLeftRightTest(render_text.get(), rtl, rtl.size(), false);
+
+  ltr.clear();
+  ltr.push_back(SelectionModel(3, 2, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(2, 2, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(1, 1, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  RunMoveCursorLeftRightTest(render_text.get(), ltr, ltr.size(), true);
+
+  // LTR-RTL
+  render_text->SetText(WideToUTF16(L"abc\x05d0\x05d1\x05d2"));
+  // The last one is the expected END position.
+  rtl.clear();

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

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

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

Done.

+  rtl.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(1, 0, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(2, 1, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(3, 2, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(5, 5, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(4, 4, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(3, 3, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(6, 3, SelectionModel::LEADING));
+  RunMoveCursorLeftRightTest(render_text.get(), rtl, rtl.size(), true);
+
+  ltr.clear();
+  ltr.push_back(SelectionModel(6, 3, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(4, 3, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(5, 4, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(6, 5, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(2, 2, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(1, 1, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  RunMoveCursorLeftRightTest(render_text.get(), ltr, ltr.size(), false);
+
+  // RTL-LTR
+  render_text->SetText(WideToUTF16(L"\x05d0\x05d1\x05d2"L"abc"));
+  rtl.clear();
+  rtl.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(1, 0, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(2, 1, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(3, 2, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(5, 5, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(4, 4, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(3, 3, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(6, 3, SelectionModel::LEADING));
+  RunMoveCursorLeftRightTest(render_text.get(), rtl, rtl.size(), false);
+
+  ltr.clear();
+  ltr.push_back(SelectionModel(6, 3, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(4, 3, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(5, 4, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(6, 5, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(2, 2, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(1, 1, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  RunMoveCursorLeftRightTest(render_text.get(), ltr, ltr.size(), true);
+
+  // LTR-RTL-LTR.
+  render_text->SetText(WideToUTF16(L"a"L"\x05d1"L"b"));
+  ltr.clear();
+  ltr.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(1, 0, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(1, 1, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(3, 2, SelectionModel::TRAILING));
+  RunMoveCursorLeftRightTest(render_text.get(), ltr, ltr.size(), true);
+
+  rtl.clear();
+  rtl.push_back(SelectionModel(3, 2, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(2, 2, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(2, 1, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  RunMoveCursorLeftRightTest(render_text.get(), rtl, rtl.size(), false);
+
+  // RTL-LTR-RTL.
+  render_text->SetText(WideToUTF16(L"\x05d0"L"a"L"\x05d1"));
+  rtl.clear();
+  rtl.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(1, 0, SelectionModel::TRAILING));
+  rtl.push_back(SelectionModel(1, 1, SelectionModel::LEADING));
+  rtl.push_back(SelectionModel(3, 2, SelectionModel::TRAILING));
+  RunMoveCursorLeftRightTest(render_text.get(), rtl, rtl.size(), false);
+
+  ltr.clear();
+  ltr.push_back(SelectionModel(3, 2, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(2, 2, SelectionModel::LEADING));
+  ltr.push_back(SelectionModel(2, 1, SelectionModel::TRAILING));
+  ltr.push_back(SelectionModel(0, 0, SelectionModel::LEADING));
+  RunMoveCursorLeftRightTest(render_text.get(), ltr, ltr.size(), true);
+}
+
+TEST_F(RenderTextTest, MoveCursorLeftRight_ComplexScript) {
+  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
+
+  render_text->SetText(WideToUTF16(L"\x0915\x093f\x0915\x094d\x0915"));
+  EXPECT_EQ(0U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, false);
+  EXPECT_EQ(2U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, false);
+  EXPECT_EQ(4U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, false);
+  EXPECT_EQ(5U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, false);
+  EXPECT_EQ(5U, render_text->GetCursorPosition());
+
+  render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+  EXPECT_EQ(4U, render_text->GetCursorPosition());
+  render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+  EXPECT_EQ(2U, render_text->GetCursorPosition());
+  render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+  EXPECT_EQ(0U, render_text->GetCursorPosition());
+  render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+  EXPECT_EQ(0U, render_text->GetCursorPosition());
+}
+
+TEST_F(RenderTextTest, MoveCursorLeftRightWithSelection) {
+  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
+  render_text->SetText(WideToUTF16(L"abc\x05d0\x05d1\x05d2"));
+  // Left arrow on select ranging (6, 4).
+  render_text->MoveCursorRight(LINE_BREAK, false);
+  EXPECT_EQ(6U, render_text->GetCursorPosition());
+  render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+  EXPECT_EQ(4U, render_text->GetCursorPosition());
+  render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+  EXPECT_EQ(5U, render_text->GetCursorPosition());
+  render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+  EXPECT_EQ(6U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, true);
+  EXPECT_EQ(6U, render_text->GetSelectionStart());
+  EXPECT_EQ(5U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, true);
+  EXPECT_EQ(6U, render_text->GetSelectionStart());
+  EXPECT_EQ(4U, render_text->GetCursorPosition());
+  render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+  EXPECT_EQ(6U, render_text->GetCursorPosition());
+
+  // Right arrow on select ranging (4, 6).
+  render_text->MoveCursorLeft(LINE_BREAK, false);
+  EXPECT_EQ(0U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, false);
+  EXPECT_EQ(1U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, false);
+  EXPECT_EQ(2U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, false);
+  EXPECT_EQ(3U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, false);
+  EXPECT_EQ(5U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, false);
+  EXPECT_EQ(4U, render_text->GetCursorPosition());
+  render_text->MoveCursorLeft(CHARACTER_BREAK, true);
+  EXPECT_EQ(4U, render_text->GetSelectionStart());
+  EXPECT_EQ(5U, render_text->GetCursorPosition());
+  render_text->MoveCursorLeft(CHARACTER_BREAK, true);
+  EXPECT_EQ(4U, render_text->GetSelectionStart());
+  EXPECT_EQ(6U, render_text->GetCursorPosition());
+  render_text->MoveCursorRight(CHARACTER_BREAK, false);
+  EXPECT_EQ(4U, render_text->GetCursorPosition());
+}
+
 }  // namespace gfx