Revert 113635 (speculative revert for bug 107104)

- 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.
Review URL: http://codereview.chromium.org/8575020

TBR=asvitkine@chromium.org

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@113956 0039d316-1c4b-4281-b951-d872f2087c98

ui/gfx/render_text.cc

@@ -68,10 +68,9 @@ void ApplyStyleRangeImpl(gfx::StyleRanges* style_ranges,
     } else if (i->range.end() > new_range.end()) {
       i->range.set_start(new_range.end());
       break;
-    } else {
+    } else
       NOTREACHED();
   }
-  }
   // Add the new range in its sorted location.
   style_ranges->insert(i, style_range);
 }
@@ -263,8 +262,8 @@ void RenderText::MoveCursorRight(BreakType break_type, bool select) {
   MoveCursorTo(position);
 }
 
-bool RenderText::MoveCursorTo(const SelectionModel& model) {
-  SelectionModel sel(model);
+bool RenderText::MoveCursorTo(const SelectionModel& selection_model) {
+  SelectionModel sel(selection_model);
   size_t text_length = text().length();
   // Enforce valid selection model components.
   if (sel.selection_start() > text_length)
@@ -443,7 +442,7 @@ SelectionModel RenderText::FindCursorPosition(const Point& point) {
   // binary searching the cursor position.
   // TODO(oshima): use the center of character instead of edge.
   // Binary search may not work for language like Arabic.
-  while (std::abs(right_pos - left_pos) > 1) {
+  while (std::abs(static_cast<long>(right_pos - left_pos)) > 1) {
     int pivot_pos = left_pos + (right_pos - left_pos) / 2;
     int pivot = font.GetStringWidth(text().substr(0, pivot_pos));
     if (pivot < x) {
@@ -505,7 +504,8 @@ SelectionModel RenderText::GetLeftSelectionModel(const SelectionModel& current,
                                                  BreakType break_type) {
   if (break_type == LINE_BREAK)
     return LeftEndSelectionModel();
-  size_t pos = std::max<int>(current.selection_end() - 1, 0);
+  size_t pos = std::max(static_cast<long>(current.selection_end() - 1),
+                        static_cast<long>(0));
   if (break_type == CHARACTER_BREAK)
     return SelectionModel(pos, pos, SelectionModel::LEADING);
 
@@ -579,7 +579,8 @@ void RenderText::SetSelectionModel(const SelectionModel& model) {
   selection_model_.set_selection_start(model.selection_start());
   DCHECK_LE(model.selection_end(), text().length());
   selection_model_.set_selection_end(model.selection_end());
-  DCHECK_LT(model.caret_pos(), std::max<size_t>(text().length(), 1));
+  DCHECK_LT(model.caret_pos(),
+            std::max(text().length(), static_cast<size_t>(1)));
   selection_model_.set_caret_pos(model.caret_pos());
   selection_model_.set_caret_placement(model.caret_placement());
 

ui/gfx/render_text.h

@@ -264,9 +264,6 @@ class UI_EXPORT RenderText {
   virtual void DrawVisualText(Canvas* canvas) = 0;
 
   // Get the logical index of the grapheme preceding the argument |position|.
-  // If |IsCursorablePosition(position)| is true, the result will be the start
-  // of the previous grapheme, if any. Otherwise, the result will be the start
-  // of the grapheme containing |position|.
   size_t GetIndexOfPreviousGrapheme(size_t position);
 
   // Apply composition style (underline) to composition range and selection
@@ -288,13 +285,8 @@ class UI_EXPORT RenderText {
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, CustomDefaultStyle);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, ApplyStyleRange);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, StyleRangesAdjust);
-  FRIEND_TEST_ALL_PREFIXES(RenderTextTest, GraphemePositions);
-  FRIEND_TEST_ALL_PREFIXES(RenderTextTest, SelectionModels);
 
   // Return an index belonging to the |next| or previous logical grapheme.
-  // If |next| is false and |IsCursorablePosition(index)| is true, the result
-  // will be the start of the previous grapheme, if any. Otherwise, the result
-  // will be the start of the grapheme containing |index|.
   // The return value is bounded by 0 and the text length, inclusive.
   virtual size_t IndexOfAdjacentGrapheme(size_t index, bool next) = 0;
 

ui/gfx/render_text_linux.cc

@@ -380,8 +380,6 @@ void RenderTextLinux::DrawVisualText(Canvas* canvas) {
 }
 
 size_t RenderTextLinux::IndexOfAdjacentGrapheme(size_t index, bool next) {
-  if (index > text().length())
-    return text().length();
   EnsureLayout();
   return Utf16IndexOfAdjacentGrapheme(Utf16IndexToUtf8Index(index), next);
 }

ui/gfx/render_text_unittest.cc

@@ -8,10 +8,6 @@
 #include "base/utf_string_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-#if defined(OS_WIN)
-#include "base/win/windows_version.h"
-#endif
-
 namespace gfx {
 
 class RenderTextTest : public testing::Test {
@@ -547,119 +543,6 @@ TEST_F(RenderTextTest, MoveCursorLeftRight_ComplexScript) {
 }
 #endif
 
-TEST_F(RenderTextTest, GraphemePositions) {
-  // LTR 2-character grapheme, LTR abc, LTR 2-character grapheme.
-  const string16 kText1 = WideToUTF16(L"\x0915\x093f"L"abc"L"\x0915\x093f");
-
-  // LTR ab, LTR 2-character grapheme, LTR cd.
-  const string16 kText2 = WideToUTF16(L"ab"L"\x0915\x093f"L"cd");
-
-  struct {
-    string16 text;
-    size_t index;
-    size_t expected_previous;
-    size_t expected_next;
-  } cases[] = {
-    { string16(), 0, 0, 0 },
-    { string16(), 1, 0, 0 },
-    { string16(), 50, 0, 0 },
-    { kText1, 0, 0, 2 },
-    { kText1, 1, 0, 2 },
-    { kText1, 2, 0, 3 },
-    { kText1, 3, 2, 4 },
-    { kText1, 4, 3, 5 },
-    { kText1, 5, 4, 7 },
-    { kText1, 6, 5, 7 },
-    { kText1, 7, 5, 7 },
-    { kText1, 8, 7, 7 },
-    { kText1, 50, 7, 7 },
-    { kText2, 0, 0, 1 },
-    { kText2, 1, 0, 2 },
-    { kText2, 2, 1, 4 },
-    { kText2, 3, 2, 4 },
-    { kText2, 4, 2, 5 },
-    { kText2, 5, 4, 6 },
-    { kText2, 6, 5, 6 },
-    { kText2, 7, 6, 6 },
-    { kText2, 50, 6, 6 },
-  };
-
-  // TODO(asvitkine): Disable tests that fail on XP bots due to lack of complete
-  //                  font support for some scripts - http://crbug.com/106450
-#if defined(OS_WIN)
-  if (base::win::GetVersion() < base::win::VERSION_VISTA)
-    return;
-#endif
-
-  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
-  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
-    render_text->SetText(cases[i].text);
-
-    size_t next = render_text->GetIndexOfNextGrapheme(cases[i].index);
-    EXPECT_EQ(cases[i].expected_next, next);
-    EXPECT_TRUE(render_text->IsCursorablePosition(next));
-
-    size_t previous = render_text->GetIndexOfPreviousGrapheme(cases[i].index);
-    EXPECT_EQ(cases[i].expected_previous, previous);
-    EXPECT_TRUE(render_text->IsCursorablePosition(previous));
-  }
-}
-
-TEST_F(RenderTextTest, SelectionModels) {
-  // Simple Latin text.
-  const string16 kLatin = WideToUTF16(L"abc");
-  // LTR 2-character grapheme.
-  const string16 kLTRGrapheme = WideToUTF16(L"\x0915\x093f");
-  // LTR 2-character grapheme, LTR a, LTR 2-character grapheme.
-  const string16 kHindiLatin = WideToUTF16(L"\x0915\x093f"L"a"L"\x0915\x093f");
-  // RTL 2-character grapheme.
-  const string16 kRTLGrapheme = WideToUTF16(L"\x05e0\x05b8");
-  // RTL 2-character grapheme, LTR a, RTL 2-character grapheme.
-  const string16 kHebrewLatin = WideToUTF16(L"\x05e0\x05b8"L"a"L"\x05e0\x05b8");
-
-  struct {
-    string16 text;
-    size_t expected_left_end_caret;
-    SelectionModel::CaretPlacement expected_left_end_placement;
-    size_t expected_right_end_caret;
-    SelectionModel::CaretPlacement expected_right_end_placement;
-  } cases[] = {
-    { string16(), 0, SelectionModel::LEADING, 0, SelectionModel::LEADING },
-    { kLatin, 0, SelectionModel::LEADING, 2, SelectionModel::TRAILING },
-    { kLTRGrapheme, 0, SelectionModel::LEADING, 0, SelectionModel::TRAILING },
-    { kHindiLatin, 0, SelectionModel::LEADING, 3, SelectionModel::TRAILING },
-    { kRTLGrapheme, 0, SelectionModel::TRAILING, 0, SelectionModel::LEADING },
-#if defined(OS_LINUX)
-    // On Linux, the whole string is displayed RTL, rather than individual runs.
-    { kHebrewLatin, 3, SelectionModel::TRAILING, 0, SelectionModel::LEADING },
-#else
-    { kHebrewLatin, 0, SelectionModel::TRAILING, 3, SelectionModel::LEADING },
-#endif
-  };
-
-  // TODO(asvitkine): Disable tests that fail on XP bots due to lack of complete
-  //                  font support for some scripts - http://crbug.com/106450
-#if defined(OS_WIN)
-  if (base::win::GetVersion() < base::win::VERSION_VISTA)
-    return;
-#endif
-
-  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
-  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {
-    render_text->SetText(cases[i].text);
-
-    SelectionModel model = render_text->LeftEndSelectionModel();
-    EXPECT_EQ(cases[i].expected_left_end_caret, model.caret_pos());
-    EXPECT_TRUE(render_text->IsCursorablePosition(model.caret_pos()));
-    EXPECT_EQ(cases[i].expected_left_end_placement, model.caret_placement());
-
-    model = render_text->RightEndSelectionModel();
-    EXPECT_EQ(cases[i].expected_right_end_caret, model.caret_pos());
-    EXPECT_TRUE(render_text->IsCursorablePosition(model.caret_pos()));
-    EXPECT_EQ(cases[i].expected_right_end_placement, model.caret_placement());
-  }
-}
-
 TEST_F(RenderTextTest, MoveCursorLeftRightWithSelection) {
   scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
   render_text->SetText(WideToUTF16(L"abc\x05d0\x05d1\x05d2"));

ui/gfx/render_text_win.cc

@@ -254,15 +254,10 @@ SelectionModel RenderTextWin::LeftEndSelectionModel() {
   EnsureLayout();
   size_t cursor = base::i18n::IsRTL() ? text().length() : 0;
   internal::TextRun* run = runs_[visual_to_logical_[0]];
-  size_t caret;
-  SelectionModel::CaretPlacement placement;
-  if (run->script_analysis.fRTL) {
-    caret = IndexOfAdjacentGrapheme(run->range.end(), false);
-    placement = SelectionModel::TRAILING;
-  } else {
-    caret = run->range.start();
-    placement = SelectionModel::LEADING;
-  }
+  bool rtl = run->script_analysis.fRTL;
+  size_t caret = rtl ? run->range.end() - 1 : run->range.start();
+  SelectionModel::CaretPlacement placement =
+      rtl ? SelectionModel::TRAILING : SelectionModel::LEADING;
   return SelectionModel(cursor, caret, placement);
 }
 
@@ -273,15 +268,10 @@ SelectionModel RenderTextWin::RightEndSelectionModel() {
   EnsureLayout();
   size_t cursor = base::i18n::IsRTL() ? 0 : text().length();
   internal::TextRun* run = runs_[visual_to_logical_[runs_.size() - 1]];
-  size_t caret;
-  SelectionModel::CaretPlacement placement;
-  if (run->script_analysis.fRTL) {
-    caret = run->range.start();
-    placement = SelectionModel::LEADING;
-  } else {
-    caret = IndexOfAdjacentGrapheme(run->range.end(), false);
-    placement = SelectionModel::TRAILING;
-  }
+  bool rtl = run->script_analysis.fRTL;
+  size_t caret = rtl ? run->range.start() : run->range.end() - 1;
+  SelectionModel::CaretPlacement placement =
+      rtl ? SelectionModel::LEADING : SelectionModel::TRAILING;
   return SelectionModel(cursor, caret, placement);
 }
 
@@ -412,50 +402,22 @@ void RenderTextWin::DrawVisualText(Canvas* canvas) {
 
 size_t RenderTextWin::IndexOfAdjacentGrapheme(size_t index, bool next) {
   EnsureLayout();
-
-  if (text().empty())
-    return 0;
-
-  if (index >= text().length()) {
-    if (next || index > text().length()) {
-      return text().length();
-    } else {
-      // The requested |index| is at the end of the text. Use the index of the
-      // last character to find the grapheme.
-      index = text().length() - 1;
-      if (IsCursorablePosition(index))
-        return index;
-    }
-  }
-
   size_t run_index = GetRunContainingPosition(index);
-  DCHECK(run_index < runs_.size());
-  internal::TextRun* run = runs_[run_index];
-  size_t start = run->range.start();
-  size_t ch = index - start;
+  internal::TextRun* run = run_index < runs_.size() ? runs_[run_index] : NULL;
+  int start = run ? run->range.start() : 0;
+  int length = run ? run->range.length() : text().length();
+  int ch = index - start;
+  WORD cluster = run ? run->logical_clusters[ch] : 0;
 
   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];
-      start = run->range.start();
-      ch = run->range.length();
-    }
-
-    // Loop to find the start of the grapheme.
-    WORD cluster = run->logical_clusters[ch - 1];
     do {
       ch--;
-    } while (ch > 0 && run->logical_clusters[ch - 1] == cluster);
+    } while (ch >= 0 && run && run->logical_clusters[ch] == cluster);
   } else {
-    WORD cluster = run->logical_clusters[ch];
-    while (ch < run->range.length() && run->logical_clusters[ch] == cluster)
+    while (ch < length && run && run->logical_clusters[ch] == cluster)
       ch++;
   }
-
-  return start + ch;
+  return std::max(std::min(ch, length) + start, 0);
 }
 
 void RenderTextWin::ItemizeLogicalText() {
@@ -525,7 +487,6 @@ void RenderTextWin::LayoutVisualText() {
     internal::TextRun* run = *run_iter;
     size_t run_length = run->range.length();
     const wchar_t* run_text = &(text()[run->range.start()]);
-    bool tried_fallback = false;
 
     // Select the font desired for glyph generation.
     SelectObject(hdc, run->font.GetNativeFont());
@@ -550,21 +511,12 @@ void RenderTextWin::LayoutVisualText() {
       if (hr == E_OUTOFMEMORY) {
         max_glyphs *= 2;
       } else if (hr == USP_E_SCRIPT_NOT_IN_FONT) {
-         // Only try font fallback if it hasn't yet been attempted for this run.
-         if (tried_fallback) {
-           // TODO(msw): Don't use SCRIPT_UNDEFINED. Apparently Uniscribe can
-           //            crash on certain surrogate pairs with SCRIPT_UNDEFINED.
+        // TODO(msw): Don't use SCRIPT_UNDEFINED. Apparently Uniscribe can crash
+        //            on certain surrogate pairs with SCRIPT_UNDEFINED.
         //            See https://bugzilla.mozilla.org/show_bug.cgi?id=341500
         //            And http://maxradi.us/documents/uniscribe/
-           run->script_analysis.eScript = SCRIPT_UNDEFINED;
-           // Reset |hr| to 0 to not trigger the DCHECK() below when a font is
-           // not found that can display the text. This is expected behavior
-           // under Windows XP without additional language packs installed and
-           // may also happen on newer versions when trying to display text in
-           // an obscure script that the system doesn't have the right font for.
-           hr = 0;
+        if (run->script_analysis.eScript == SCRIPT_UNDEFINED)
           break;
-         }
 
         // The run's font doesn't contain the required glyphs, use an alternate.
         if (ChooseFallbackFont(hdc, run->font, run_text, run_length,
@@ -573,7 +525,7 @@ void RenderTextWin::LayoutVisualText() {
           SelectObject(hdc, run->font.GetNativeFont());
         }
 
-        tried_fallback = true;
+        run->script_analysis.eScript = SCRIPT_UNDEFINED;
       } else {
         break;
       }

ui/views/controls/textfield/textfield_views_model_unittest.cc

@@ -20,10 +20,6 @@
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/views_delegate.h"
 
-#if defined(OS_WIN)
-#include "base/win/windows_version.h"
-#endif
-
 namespace {
 
 struct WordAndCursor {
@@ -141,15 +137,7 @@ TEST_F(TextfieldViewsModelTest, EditString_SimpleRTL) {
 }
 
 TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
-  // TODO(asvitkine): Disable tests that fail on XP bots due to lack of complete
-  //                  font support for some scripts - http://crbug.com/106450
-  bool on_windows_xp = false;
-#if defined(OS_WIN)
-  on_windows_xp = base::win::GetVersion() < base::win::VERSION_VISTA;
-#endif
-
   TextfieldViewsModel model(NULL);
-
   // Append two Hindi strings.
   model.Append(WideToUTF16(L"\x0915\x093f\x0915\x094d\x0915"));
   EXPECT_EQ(WideToUTF16(L"\x0915\x093f\x0915\x094d\x0915"),
@@ -159,12 +147,14 @@ TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
       L"\x0915\x093f\x0915\x094d\x0915\x0915\x094d\x092e\x094d"),
       model.GetText());
 
-  // TODO(asvitkine): Disable tests that fail on XP bots due to lack of complete
-  //                  font support for some scripts - http://crbug.com/106450
-  if (!on_windows_xp) {
   // Check it is not able to place cursor in middle of a grapheme.
+  // TODO(xji): temporarily disable in platform Win since the complex script
+  // characters turned into empty square due to font regression. So, not able
+  // to test 2 characters belong to the same grapheme.
+#if defined(OS_LINUX)
   model.MoveCursorTo(gfx::SelectionModel(1U));
   EXPECT_EQ(0U, model.GetCursorPosition());
+#endif
 
   model.MoveCursorTo(gfx::SelectionModel(2U));
   EXPECT_EQ(2U, model.GetCursorPosition());
@@ -184,7 +174,6 @@ TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
       model.GetText());
 #endif
   EXPECT_EQ(4U, model.GetCursorPosition());
-  }
 
   // Delete should delete the whole grapheme.
   model.MoveCursorTo(gfx::SelectionModel(0U));
@@ -196,7 +185,6 @@ TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
   EXPECT_EQ(WideToUTF16(L"\x0061\x0062\x0915\x0915\x094d\x092e\x094d"),
             model.GetText());
   model.MoveCursorTo(gfx::SelectionModel(model.GetText().length()));
-  EXPECT_EQ(model.GetText().length(), model.GetCursorPosition());
   EXPECT_TRUE(model.Backspace());
   EXPECT_EQ(WideToUTF16(L"\x0061\x0062\x0915\x0915\x094d\x092e"),
             model.GetText());
@@ -207,24 +195,30 @@ TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
   model.MoveCursorTo(gfx::SelectionModel(0));
   EXPECT_EQ(0U, model.GetCursorPosition());
 
-  // TODO(asvitkine): Disable tests that fail on XP bots due to lack of complete
-  //                  font support for some scripts - http://crbug.com/106450
-  if (!on_windows_xp) {
+  // TODO(xji): temporarily disable in platform Win since the complex script
+  // characters turned into empty square due to font regression. So, not able
+  // to test 2 characters belong to the same grapheme.
+#if defined(OS_LINUX)
   model.MoveCursorTo(gfx::SelectionModel(1));
   EXPECT_EQ(0U, model.GetCursorPosition());
+#endif
+
+  model.MoveCursorTo(gfx::SelectionModel(2));
+  EXPECT_EQ(2U, model.GetCursorPosition());
+
   model.MoveCursorTo(gfx::SelectionModel(3));
   EXPECT_EQ(3U, model.GetCursorPosition());
-  }
 
-  // TODO(asvitkine): Temporarily disable the following check on Windows. It
-  // seems Windows treats "\x0D38\x0D4D\x0D15" as a single grapheme.
-#if !defined(OS_WIN)
   model.MoveCursorTo(gfx::SelectionModel(2));
-  EXPECT_EQ(2U, model.GetCursorPosition());
+
   EXPECT_TRUE(model.Backspace());
   EXPECT_EQ(WideToUTF16(L"\x0D38\x0D15\x0D16\x0D2E"), model.GetText());
-#endif
 
+  // Test Delete/Backspace on Hebrew with non-spacing marks.
+  // TODO(xji): temporarily disable in platform Win since the complex script
+  // characters turned into empty square due to font regression. So, not able
+  // to test 2 characters belong to the same grapheme.
+#if defined(OS_LINUX)
   model.SetText(WideToUTF16(L"\x05d5\x05b7\x05D9\x05B0\x05D4\x05B4\x05D9"));
   model.MoveCursorTo(gfx::SelectionModel(0));
   EXPECT_TRUE(model.Delete());
@@ -232,6 +226,7 @@ TEST_F(TextfieldViewsModelTest, EditString_ComplexScript) {
   EXPECT_TRUE(model.Delete());
   EXPECT_TRUE(model.Delete());
   EXPECT_EQ(WideToUTF16(L""), model.GetText());
+#endif
 
   // The first 2 characters are not strong directionality characters.
   model.SetText(WideToUTF16(L"\x002C\x0020\x05D1\x05BC\x05B7\x05E9\x05BC"));
@@ -566,7 +561,7 @@ TEST_F(TextfieldViewsModelTest, MAYBE_Clipboard) {
   EXPECT_EQ(29U, model.GetCursorPosition());
 }
 
-static void SelectWordTestVerifier(const TextfieldViewsModel& model,
+void SelectWordTestVerifier(TextfieldViewsModel &model,
     const string16 &expected_selected_string, size_t expected_cursor_pos) {
   EXPECT_EQ(expected_selected_string, model.GetSelectedText());
   EXPECT_EQ(expected_cursor_pos, model.GetCursorPosition());