Fix multiline behaviors for RTL text.

ui/gfx/render_text_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/render_text.h"
 
 #include <algorithm>
 
 #include "base/format_macros.h"
 #include "base/i18n/break_iterator.h"
@@ skipping 72 matching lines @@
 // Test utility for Multiline_Newline test case. Empty |expected_range| means
 // the blank line which has no segments. Otherwise |segments| should contain
 // exactly one line segment whose range equals to |expected_range|.
 void VerifyLineSegments(const Range& expected_range,
                         const std::vector<internal::LineSegment>& segments) {
   EXPECT_EQ(expected_range.is_empty() ? 0ul : 1ul, segments.size());
   if (!expected_range.is_empty())
     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 {
 };
 
 TEST_F(RenderTextTest, DefaultStyle) {
   // Check the default styles applied to new instances and adjusted text.
   scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
   EXPECT_TRUE(render_text->text().empty());
   const wchar_t* const cases[] = { kWeak, kLtr, L"Hello", kRtl, L"", L"" };
@@ skipping 1809 matching lines @@
     EXPECT_GT(render_text.lines_.size(), 1U);
   }
 }
 
 // Ensure strings wrap onto multiple lines for a normal available width.
 TEST_F(RenderTextTest, Multiline_NormalWidth) {
   const struct {
     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;
   // Specify the fixed width for characters to suppress the possible variations
   // of linebreak results.
   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,
               render_text.lines_[0].segments[0].char_range);
     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());
+    }
   }
 }
 
 // Ensure strings don't wrap onto multiple lines for a sufficient available
 // width.
 TEST_F(RenderTextTest, Multiline_SufficientWidth) {
   const wchar_t* kTestStrings[] = { L"", L" ", L".", L" . ", L"abc", L"a b c",
                                     L"\x62E\x628\x632", L"\x62E \x628 \x632" };
 
   RenderTextHarfBuzz render_text;
@@ skipping 56 matching lines @@
 
   for (size_t i = 0; i < arraysize(kTestStrings); ++i) {
     SCOPED_TRACE(base::StringPrintf("kTestStrings[%" PRIuS "]", i));
     render_text.SetText(WideToUTF16(kTestStrings[i]));
     render_text.Draw(&canvas);
 
     EXPECT_EQ(1U, render_text.lines_.size());
   }
 }
 
+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 {
     uint32 glyph_to_char[4];
     Range chars[4];
     Range glyphs[4];
     bool is_rtl;
   } cases[] = {
     { // From string "A B C D" to glyphs "a b c d".
       { 0, 1, 2, 3 },
@@ skipping 349 matching lines @@
             base::StringToLowerASCII(fonts[0].GetActualFontNameForTesting()));
 
   scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
   render_text->SetDisplayRect(Rect(0, 0, 25, 25));
   render_text->SetFontList(font_list);
   EXPECT_GT(render_text->GetBaseline(), font_list.GetBaseline());
 }
 #endif
 
 }  // namespace gfx