Windows implementation of multiline RenderText

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 263ef92d8159c7088650ddd538457e49061d7ea0..acbbde74bb2725cd3dec5cca7145e5b043a0d1b5 100644
--- a/ui/gfx/render_text_unittest.cc
+++ b/ui/gfx/render_text_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -78,6 +79,50 @@ void RunMoveCursorLeftRightTest(RenderText* render_text,
   render_text->MoveCursor(LINE_BREAK, direction, false);
   EXPECT_EQ(expected.back(), render_text->selection_model());
 }
+
+// For segments in the same run, checks the continuity and order of |x_range|
+// and |char_range| fields.
+#if defined(OS_WIN)
+void CheckLineIntegrity(const std::vector<internal::Line>& lines,
+                        const ScopedVector<internal::TextRun>& runs) {
+  size_t previous_segment_line = 0;
+  const internal::LineSegment* previous_segment = NULL;
+
+  for (size_t i = 0; i < lines.size(); ++i) {
+    SCOPED_TRACE(base::StringPrintf("line %" PRIuS, i));
+    for (size_t j = 0; j < lines[i].segments.size(); ++j) {
+      SCOPED_TRACE(base::StringPrintf("segment %" PRIuS, j));
+      const internal::LineSegment* const segment = &lines[i].segments[j];

[Alexei Svitkine (slow), 2013/08/27 15:40:38]

Nit: I don't think we usually put const after the *. Please remove, here and on
the line below.

[ckocagil, 2013/08/28 19:16:26]

The const after the * makes the pointer itself const. There is already other
code in this file that declares const pointers. Should I remove all these
instances or keep them?

[Alexei Svitkine (slow), 2013/08/28 21:27:46]

The other ones only seem to do it for arrays of const char* strings, rather than
const pointers to an actual class instance. I'd keep those other ones, since
there's precedent (in this file and elsewhere in the codebase) for those, but
remove these ones (the const keywords after the * only).

[ckocagil, 2013/08/30 16:53:36]

Done.

+      internal::TextRun* const run = runs[segment->run];
+
+      if (!previous_segment) {
+        previous_segment = segment;
+      } else if (runs[previous_segment->run] != run) {
+        previous_segment = NULL;
+      } else {
+        SCOPED_TRACE(run->script_analysis.fRTL ? "RTL" : "LTR");
+        SCOPED_TRACE(base::StringPrintf(
+            "current x_range: [%" PRIuS ", %" PRIuS ")",

[msw, 2013/08/27 01:30:52]

nit: printing ui::Range ints with PRIuS (unsigned size) is wrong here and below.

[ckocagil, 2013/08/28 18:01:45]

Is it? ui::Range::start() and end() both return size_t. Is there a variadic arg
default conversion I'm overlooking? Anyway, I'll just use ui::Range::ToString
here since it's simpler.

[msw, 2013/08/30 00:42:28]

Hmm, I mistakenly thought they were ints; glad you improved it anyway.

+            segment->x_range.start(), segment->x_range.end()));
+        SCOPED_TRACE(base::StringPrintf(
+            "previous x_range: [%" PRIuS ", %" PRIuS ")",
+            previous_segment->x_range.start(),
+            previous_segment->x_range.end()));
+
+        EXPECT_EQ(previous_segment->char_range.end(),
+                  segment->char_range.start());
+        if (!run->script_analysis.fRTL)
+          EXPECT_EQ(previous_segment->x_range.end(), segment->x_range.start());
+        else if (i != previous_segment_line)
+          EXPECT_EQ(segment->x_range.end(), previous_segment->x_range.start());
+
+        previous_segment = segment;
+        previous_segment_line = i;
+      }
+    }
+  }
+}
+#endif  // defined(OS_WIN)
 
 }  // namespace
 
@@ -1294,7 +1339,7 @@ TEST_F(RenderTextTest, GetTextOffset) {
   Rect display_rect(font_size);
   render_text->SetDisplayRect(display_rect);
 
-  Vector2d offset = render_text->GetTextOffset();
+  Vector2d offset = render_text->GetLineOffset(0);
   EXPECT_TRUE(offset.IsZero());
 
   // Set display area's size greater than font size.
@@ -1303,30 +1348,30 @@ TEST_F(RenderTextTest, GetTextOffset) {
   render_text->SetDisplayRect(display_rect);
 
   // Check the default horizontal and vertical alignment.
-  offset = render_text->GetTextOffset();
+  offset = render_text->GetLineOffset(0);
   EXPECT_EQ(kEnlargement / 2, offset.y());
   EXPECT_EQ(0, offset.x());
 
   // Check explicitly setting the horizontal alignment.
   render_text->SetHorizontalAlignment(ALIGN_LEFT);
-  offset = render_text->GetTextOffset();
+  offset = render_text->GetLineOffset(0);
   EXPECT_EQ(0, offset.x());
   render_text->SetHorizontalAlignment(ALIGN_CENTER);
-  offset = render_text->GetTextOffset();
+  offset = render_text->GetLineOffset(0);
   EXPECT_EQ(kEnlargement / 2, offset.x());
   render_text->SetHorizontalAlignment(ALIGN_RIGHT);
-  offset = render_text->GetTextOffset();
+  offset = render_text->GetLineOffset(0);
   EXPECT_EQ(kEnlargement, offset.x());
 
   // Check explicitly setting the vertical alignment.
   render_text->SetVerticalAlignment(ALIGN_TOP);
-  offset = render_text->GetTextOffset();
+  offset = render_text->GetLineOffset(0);
   EXPECT_EQ(0, offset.y());
   render_text->SetVerticalAlignment(ALIGN_VCENTER);
-  offset = render_text->GetTextOffset();
+  offset = render_text->GetLineOffset(0);
   EXPECT_EQ(kEnlargement / 2, offset.y());
   render_text->SetVerticalAlignment(ALIGN_BOTTOM);
-  offset = render_text->GetTextOffset();
+  offset = render_text->GetLineOffset(0);
   EXPECT_EQ(kEnlargement, offset.y());
 
   SetRTL(was_rtl);
@@ -1334,7 +1379,7 @@ TEST_F(RenderTextTest, GetTextOffset) {
 
 TEST_F(RenderTextTest, GetTextOffsetHorizontalDefaultInRTL) {
   // This only checks the default horizontal alignment in RTL mode; all other
-  // GetTextOffset() attributes are checked by the test above.
+  // GetLineOffset(0) attributes are checked by the test above.
   const bool was_rtl = base::i18n::IsRTL();
   SetRTL(true);
   scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
@@ -1345,7 +1390,7 @@ TEST_F(RenderTextTest, GetTextOffsetHorizontalDefaultInRTL) {
                        render_text->GetStringSize().height());
   Rect display_rect(font_size);
   render_text->SetDisplayRect(display_rect);
-  Vector2d offset = render_text->GetTextOffset();
+  Vector2d offset = render_text->GetLineOffset(0);
   EXPECT_EQ(kEnlargement, offset.x());
   SetRTL(was_rtl);
 }
@@ -1650,5 +1695,59 @@ TEST_F(RenderTextTest, SelectionKeepsLigatures) {
     render_text->MoveCursorTo(SelectionModel(0, CURSOR_FORWARD));
   }
 }
+
+#if defined(OS_WIN)
+TEST_F(RenderTextTest, Multiline_MinWidth) {
+  const wchar_t* kTestStrings[] = { kWeak, kLtr, kLtrRtl, kLtrRtlLtr, kRtl,
+                                    kRtlLtr, kRtlLtrRtl };
+
+  scoped_ptr<RenderTextWin> render_text(
+      static_cast<RenderTextWin*>(RenderText::CreateInstance()));
+  render_text->SetDisplayRect(Rect(Point(), Size(1, 1000)));
+  render_text->SetMultiline(true);
+  Canvas canvas;
+
+  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);

[msw, 2013/08/27 01:30:52]

Is it sufficient to EnsureLayout instead of calling Draw here and below?

[ckocagil, 2013/08/28 18:01:45]

Without Draw() we won't hit the DrawVisualText() DCHECKs. I'm not sure whether
EnsureLayout is good enough.

+    CheckLineIntegrity(render_text->lines_, render_text->runs_);
+  }
+}
+
+TEST_F(RenderTextTest, Multiline_NormalWidth) {
+  const struct {
+    wchar_t* text;
+    ui::Range first_line;
+    ui::Range second_line;
+  } kTestStrings[] = {
+    { L"abc defg hijkl mnop qrstuv wxyz", ui::Range(0, 15), ui::Range(15, 31) },

[msw, 2013/08/27 01:30:52]

nit: these test cases could be covered with 50px width and half-length strings.

[ckocagil, 2013/08/28 18:01:45]

Done.

+    { L"qwertyuiopasdfghjklzxcvbnm", ui::Range(0, 17), ui::Range(17, 26) },
+    { L"\x0647\x0646\x0627\x0643 \x0627\x0644\x062A\x0641\x0627\x062D \x0648"
+      L"\x0627\x0644\x0628\x0637\x0627\x0637\x0627 \x0641\x064A",
+      ui::Range(0, 21), ui::Range(21, 23) }
+  };
+
+  scoped_ptr<RenderTextWin> render_text(
+      static_cast<RenderTextWin*>(RenderText::CreateInstance()));
+  render_text->SetDisplayRect(Rect(Point(), Size(100, 1000)));
+  render_text->SetMultiline(true);
+  Canvas canvas;
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kTestStrings); ++i) {
+    SCOPED_TRACE(base::StringPrintf("kTestStrings[%" PRIuS "]", i));
+    render_text->SetText(WideToUTF16(kTestStrings[i].text));
+    render_text->Draw(&canvas);
+    CheckLineIntegrity(render_text->lines_, render_text->runs_);
+    ASSERT_EQ(2U, render_text->lines_.size());
+    ASSERT_EQ(1U, render_text->lines_[0].segments.size());
+    EXPECT_EQ(kTestStrings[i].first_line,
+              render_text->lines_[0].segments[0].char_range);
+    ASSERT_EQ(1U, render_text->lines_[1].segments.size());
+    EXPECT_EQ(kTestStrings[i].second_line,
+              render_text->lines_[1].segments[0].char_range);
+  }
+}
+#endif  // defined(OS_WIN)
 
 }  // namespace gfx