RenderTextHarfBuzz no longer ignores its specified text direction.

The method RenderText::SetDirectionalityMode is supposed to allow the
client to override the default directionality mode (getting the
directionality from the text). However, a bug in BiDiLineIterator means
that it is largely ignored (in the majority of cases, it will use the
directionality of the text regardless).

Fixed BiDiLineIterator to honour the supplied direction.

This should have almost no noticeable effect, as almost all text fields
use the default directionality mode anyway.

BUG=496023,495933

Review URL: https://codereview.chromium.org/1153603004

Cr-Commit-Position: refs/heads/master@{#334106}

4 files changed, 55 insertions, 9 deletions

diff --git a/base/i18n/bidi_line_iterator.cc b/base/i18n/bidi_line_iterator.cc
index 8c81d85..216129e 100644
--- a/base/i18n/bidi_line_iterator.cc
+++ b/base/i18n/bidi_line_iterator.cc
@@ -9,6 +9,25 @@
 namespace base {
 namespace i18n {
 
+namespace {
+  UBiDiLevel GetParagraphLevelForDirection(TextDirection direction) {
+    switch (direction) {
+      case UNKNOWN_DIRECTION:
+        return UBIDI_DEFAULT_LTR;
+        break;
+      case RIGHT_TO_LEFT:
+        return 1;  // Highest RTL level.
+        break;
+      case LEFT_TO_RIGHT:
+        return 0;  // Highest LTR level.
+        break;
+      default:
+        NOTREACHED();
+        return 0;
+    }
+  }
+}  // namespace
+
 BiDiLineIterator::BiDiLineIterator() : bidi_(NULL) {
 }
 
@@ -19,15 +38,14 @@ BiDiLineIterator::~BiDiLineIterator() {
   }
 }
 
-bool BiDiLineIterator::Open(const string16& text, bool right_to_left) {
+bool BiDiLineIterator::Open(const string16& text, TextDirection direction) {
   DCHECK(!bidi_);
   UErrorCode error = U_ZERO_ERROR;
   bidi_ = ubidi_openSized(static_cast<int>(text.length()), 0, &error);
   if (U_FAILURE(error))
     return false;
   ubidi_setPara(bidi_, text.data(), static_cast<int>(text.length()),
-                right_to_left ? UBIDI_DEFAULT_RTL : UBIDI_DEFAULT_LTR,
-                NULL, &error);
+                GetParagraphLevelForDirection(direction), NULL, &error);
   return (U_SUCCESS(error) == TRUE);
 }
 
diff --git a/base/i18n/bidi_line_iterator.h b/base/i18n/bidi_line_iterator.h
index 07d9aa2..0bf1ec6 100644
--- a/base/i18n/bidi_line_iterator.h
+++ b/base/i18n/bidi_line_iterator.h
@@ -7,6 +7,7 @@
 
 #include "base/basictypes.h"
 #include "base/i18n/base_i18n_export.h"
+#include "base/i18n/rtl.h"
 #include "base/strings/string16.h"
 #include "third_party/icu/source/common/unicode/ubidi.h"
 
@@ -23,7 +24,7 @@ class BASE_I18N_EXPORT BiDiLineIterator {
 
   // Initializes the bidirectional iterator with the specified text.  Returns
   // whether initialization succeeded.
-  bool Open(const string16& text, bool right_to_left);
+  bool Open(const string16& text, TextDirection direction);
 
   // Returns the number of visual runs in the text, or zero on error.
   int CountRuns();
diff --git a/ui/gfx/render_text_harfbuzz.cc b/ui/gfx/render_text_harfbuzz.cc
index 8ab298f..2274c7c 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -1209,14 +1209,13 @@ SelectionModel RenderTextHarfBuzz::LastSelectionModelInsideRun(
 void RenderTextHarfBuzz::ItemizeTextToRuns(
     const base::string16& text,
     internal::TextRunList* run_list_out) {
-  const bool is_text_rtl = GetTextDirection(text) == base::i18n::RIGHT_TO_LEFT;
   DCHECK_NE(0U, text.length());
 
   // If ICU fails to itemize the text, we create a run that spans the entire
   // text. This is needed because leaving the runs set empty causes some clients
   // to misbehave since they expect non-zero text metrics from a non-empty text.
   base::i18n::BiDiLineIterator bidi_iterator;
-  if (!bidi_iterator.Open(text, is_text_rtl)) {
+  if (!bidi_iterator.Open(text, GetTextDirection(text))) {
     internal::TextRunHarfBuzz* run = new internal::TextRunHarfBuzz;
     run->range = Range(0, text.length());
     run_list_out->add(run);
diff --git a/ui/gfx/render_text_unittest.cc b/ui/gfx/render_text_unittest.cc
index 70fd790..2448620 100644
--- a/ui/gfx/render_text_unittest.cc
+++ b/ui/gfx/render_text_unittest.cc
@@ -2730,15 +2730,43 @@ TEST_F(RenderTextTest, HarfBuzz_SubglyphGraphemePartition) {
 
 TEST_F(RenderTextTest, HarfBuzz_RunDirection) {
   RenderTextHarfBuzz render_text;
-  const base::string16 mixed =
-      WideToUTF16(L"\x05D0\x05D1" L"1234" L"\x05D2\x05D3");
+  const base::string16 mixed = WideToUTF16(
+      L"\x05D0\x05D1"
+      L"1234"
+      L"\x05D2\x05D3"
+      L"abc");
   render_text.SetText(mixed);
+
+  // Get the run list for both display directions.
+  render_text.SetDirectionalityMode(DIRECTIONALITY_FORCE_LTR);
   render_text.EnsureLayout();
   internal::TextRunList* run_list = render_text.GetRunList();
-  ASSERT_EQ(3U, run_list->size());
+  ASSERT_EQ(4U, run_list->size());
+  EXPECT_TRUE(run_list->runs()[0]->is_rtl);
+  EXPECT_FALSE(run_list->runs()[1]->is_rtl);
+  EXPECT_TRUE(run_list->runs()[2]->is_rtl);
+  EXPECT_FALSE(run_list->runs()[3]->is_rtl);
+
+  // The Latin letters should appear to the right of the other runs.
+  EXPECT_EQ(2U, run_list->logical_to_visual(0));
+  EXPECT_EQ(1U, run_list->logical_to_visual(1));
+  EXPECT_EQ(0U, run_list->logical_to_visual(2));
+  EXPECT_EQ(3U, run_list->logical_to_visual(3));
+
+  render_text.SetDirectionalityMode(DIRECTIONALITY_FORCE_RTL);
+  render_text.EnsureLayout();
+  run_list = render_text.GetRunList();
+  ASSERT_EQ(4U, run_list->size());
   EXPECT_TRUE(run_list->runs()[0]->is_rtl);
   EXPECT_FALSE(run_list->runs()[1]->is_rtl);
   EXPECT_TRUE(run_list->runs()[2]->is_rtl);
+  EXPECT_FALSE(run_list->runs()[3]->is_rtl);
+
+  // The Latin letters should appear to the left of the other runs.
+  EXPECT_EQ(3U, run_list->logical_to_visual(0));
+  EXPECT_EQ(2U, run_list->logical_to_visual(1));
+  EXPECT_EQ(1U, run_list->logical_to_visual(2));
+  EXPECT_EQ(0U, run_list->logical_to_visual(3));
 }
 
 TEST_F(RenderTextTest, HarfBuzz_BreakRunsByUnicodeBlocks) {