Fix multiline behaviors for RTL text.

ui/gfx/render_text_unittest.cc

Index: ui/gfx/render_text_unittest.cc
diff --git a/ui/gfx/render_text_unittest.cc b/ui/gfx/render_text_unittest.cc
index 44aaa1a43b5772eefff567e04d4a8f802b97a011..a5cb040de5d8f8cecb5258af5240e8e351608196 100644
--- a/ui/gfx/render_text_unittest.cc
+++ b/ui/gfx/render_text_unittest.cc
@@ -90,6 +90,67 @@ void VerifyLineSegments(const Range& expected_range,
     EXPECT_EQ(expected_range, segments[0].char_range);
 }
 
+class TestSkiaTextRenderer : public internal::SkiaTextRenderer {

[msw, 2015/02/14 03:03:09]

nit: add a comment.

[Jun Mukai, 2015/02/18 00:24:13]

Done.

+ public:
+  struct TextLog {
+    TextLog() : glyph_count(0u) {}
+    gfx::RectF bounds;
+    size_t glyph_count;
+  };
+
+  struct DecorationLog {
+    DecorationLog(int x, int y, int width, bool underline, bool strike,
+                  bool diagonal_strike)
+        : x(x), y(y), width(width), underline(underline), strike(strike),
+          diagonal_strike(diagonal_strike) {}
+    int x;
+    int y;
+    int width;
+    bool underline;
+    bool strike;
+    bool diagonal_strike;
+  };
+
+  explicit TestSkiaTextRenderer(Canvas* canvas)
+      : internal::SkiaTextRenderer(canvas) {}
+  ~TestSkiaTextRenderer() override {}
+
+  void GetDrawTextLogAndReset(std::vector<TextLog>* draw_text_log) {
+    draw_text_log_.swap(*draw_text_log);
+    draw_text_log_.clear();
+  }
+
+  void GetDecorationLogAndReset(std::vector<DecorationLog>* decoration_log) {
+    decoration_log_.swap(*decoration_log);
+    decoration_log_.clear();
+  }
+
+ private:
+  // internal::SkiaTextRenderer:
+  void DrawPosText(const SkPoint* pos,
+                   const uint16* glyphs,
+                   size_t glyph_count) override {
+    TextLog log_entry;
+    log_entry.glyph_count = glyph_count;
+    for (size_t i = 0u; i < glyph_count; ++i) {

[msw, 2015/02/14 03:03:10]

nit: remove curly braces.

[Jun Mukai, 2015/02/18 00:24:13]

Modified the code a bit and now it needs curly braces.

+      log_entry.bounds.Union(gfx::RectF(pos[i].x(), pos[i].y(), 1, 1));

[msw, 2015/02/14 03:03:10]

q: are the 1x1 bounds really faithful to what's happening here?

[Jun Mukai, 2015/02/18 00:24:13]

It is unknown the actual width and height for the glyphs in this method.
But actually the test cases are interested in where it will be drawn, which
means only the origin (positions) matter.  Modified |bounds| to |origin| so that
we don't have to care about the size (for now).

+    }
+    draw_text_log_.push_back(log_entry);
+    internal::SkiaTextRenderer::DrawPosText(pos, glyphs, glyph_count);
+  }
+
+  void DrawDecorations(int x, int y, int width, bool underline, bool strike,
+                       bool diagonal_strike) override {
+    decoration_log_.push_back(
+        DecorationLog(x, y, width, underline, strike, diagonal_strike));
+    internal::SkiaTextRenderer::DrawDecorations(
+        x, y, width, underline, strike, diagonal_strike);
+  }
+
+  std::vector<TextLog> draw_text_log_;

[msw, 2015/02/14 03:03:10]

nit: rename |text_log_| to parallel |decoration_log_|?
(update GetDrawTextLogAndReset and param too, if so)

[Jun Mukai, 2015/02/18 00:24:13]

Done.

+  std::vector<DecorationLog> decoration_log_;
+};
+
 }  // namespace
 
 class RenderTextTest : public testing::Test {
@@ -1919,11 +1980,15 @@ TEST_F(RenderTextTest, Multiline_NormalWidth) {
     const wchar_t* const text;
     const Range first_line_char_range;
     const Range second_line_char_range;
+    bool is_ltr;
   } kTestStrings[] = {
-    { L"abc defg hijkl", Range(0, 9), Range(9, 14) },
-    { L"qwertyzxcvbn", Range(0, 10), Range(10, 12) },
+    { L"abc defg hijkl", Range(0, 9), Range(9, 14), true },
+    { L"qwertyzxcvbn", Range(0, 10), Range(10, 12), true },
+    { L"\x0627\x0644\x0644\x063A\x0629 "
+      L"\x0627\x0644\x0639\x0631\x0628\x064A\x0629",
+      Range(0, 6), Range(6, 13), false },
     { L"\x062A\x0641\x0627\x062D\x05EA\x05E4\x05D5\x05D6\x05D9"
-      L"\x05DA\x05DB\x05DD", Range(4, 12), Range(0, 4) }
+      L"\x05DA\x05DB\x05DD", Range(0, 4), Range(4, 12), false }
   };
 
   RenderTextHarfBuzz render_text;
@@ -1932,12 +1997,17 @@ TEST_F(RenderTextTest, Multiline_NormalWidth) {
   render_text.set_glyph_width_for_test(5);
   render_text.SetDisplayRect(Rect(50, 1000));
   render_text.SetMultiline(true);
+  render_text.SetHorizontalAlignment(ALIGN_TO_HEAD);
+
   Canvas canvas;
+  TestSkiaTextRenderer renderer(&canvas);
 
   for (size_t i = 0; i < arraysize(kTestStrings); ++i) {
     SCOPED_TRACE(base::StringPrintf("kTestStrings[%" PRIuS "]", i));
     render_text.SetText(WideToUTF16(kTestStrings[i].text));
-    render_text.Draw(&canvas);
+    render_text.EnsureLayout();
+    render_text.DrawVisualTextInternal(&renderer);
+
     ASSERT_EQ(2U, render_text.lines_.size());
     ASSERT_EQ(1U, render_text.lines_[0].segments.size());
     EXPECT_EQ(kTestStrings[i].first_line_char_range,
@@ -1945,6 +2015,22 @@ TEST_F(RenderTextTest, Multiline_NormalWidth) {
     ASSERT_EQ(1U, render_text.lines_[1].segments.size());
     EXPECT_EQ(kTestStrings[i].second_line_char_range,
               render_text.lines_[1].segments[0].char_range);
+
+    std::vector<TestSkiaTextRenderer::TextLog> draw_text_log;
+    renderer.GetDrawTextLogAndReset(&draw_text_log);
+    ASSERT_EQ(2U, draw_text_log.size());
+    EXPECT_EQ(kTestStrings[i].first_line_char_range.length(),

[msw, 2015/02/14 03:03:10]

nit: comparing the char ranges to the glyph counts isn't always reliable, but
will work in simple cases... perhaps just add a NOTE: comment to that effect.

[Jun Mukai, 2015/02/18 00:24:13]

That's true, added.

+              draw_text_log[0].glyph_count);
+    EXPECT_EQ(kTestStrings[i].second_line_char_range.length(),
+              draw_text_log[1].glyph_count);
+    EXPECT_LT(draw_text_log[0].bounds.y(), draw_text_log[1].bounds.y());
+    if (kTestStrings[i].is_ltr) {
+      EXPECT_EQ(0, draw_text_log[0].bounds.x());
+      EXPECT_EQ(0, draw_text_log[1].bounds.x());
+    } else {
+      EXPECT_LT(0, draw_text_log[0].bounds.x());
+      EXPECT_LT(0, draw_text_log[1].bounds.x());
+    }
   }
 }
 
@@ -2021,6 +2107,28 @@ TEST_F(RenderTextTest, NewlineWithoutMultilineFlag) {
   }
 }
 
+TEST_F(RenderTextTest, HarfBuzz_RTLDrawingPositions) {

[msw, 2015/02/14 03:03:10]

What value does this add over your new test cases above?

[msw, 2015/02/18 18:58:37]

Ping! Why not just add this as a test case of Multiline_NormalWidth?

[Jun Mukai, 2015/02/18 19:55:56]

This verifies the visual position in a line, while Multiline_NormalWidth
verifies the visual position among lines.
In other words, Mulitline_NormalWidth verifies the logically first segment
appears above if they are split into two lines, this verifies the logically
first segment appears at right if they are in a single line.

Actually, at first I forgot the std::sort in LineBreaker, and my code still
passed the all tests, and then I realized the necessity to verify the horizontal
positions.

Revised the test cases and it now verifies ltr languages as well.

+  RenderTextHarfBuzz render_text;
+  render_text.SetText(WideToUTF16(
+      L"\x062A\x0641\x0627\x062D\x05EA\x05E4\x05D5\x05D6\x05D9"
+      L"\x05DA\x05DB\x05DD"));
+
+  Canvas canvas;
+  TestSkiaTextRenderer renderer(&canvas);
+
+  render_text.EnsureLayout();
+  render_text.DrawVisualTextInternal(&renderer);
+
+  std::vector<TestSkiaTextRenderer::TextLog> draw_text_log;
+  renderer.GetDrawTextLogAndReset(&draw_text_log);
+
+  EXPECT_EQ(2U, draw_text_log.size());
+
+  EXPECT_EQ(8U, draw_text_log[0].glyph_count);
+  EXPECT_EQ(4U, draw_text_log[1].glyph_count);
+  EXPECT_LT(draw_text_log[0].bounds.x(), draw_text_log[1].bounds.x());
+}
+
 // Test TextRunHarfBuzz's cluster finding logic.
 TEST_F(RenderTextTest, HarfBuzz_Clusters) {
   struct {