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..4af24744a27bdd675b09b8600ed1b95d6a5393e0 100644
--- a/ui/gfx/render_text_unittest.cc
+++ b/ui/gfx/render_text_unittest.cc
@@ -78,6 +78,37 @@ void RunMoveCursorLeftRightTest(RenderText* render_text,
   render_text->MoveCursor(LINE_BREAK, direction, false);
   EXPECT_EQ(expected.back(), render_text->selection_model());
 }
+
+#if defined(OS_WIN)
+bool CheckLineIntegrity(const std::vector<internal::Line>& lines,

[msw, 2013/08/21 16:46:23]

nit: add a comment about what it actually verifies.

[ckocagil, 2013/08/23 21:45:34]

Done.

+                        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)
+    for (size_t j = 0; j < lines[i].segments.size(); ++j) {
+      const internal::LineSegment* const segment = &lines[i].segments[j];
+      internal::TextRun* const run = runs[segment->run];
+
+      if (!previous_segment || runs[previous_segment->run] != run) {
+        previous_segment = 0;

[msw, 2013/08/21 16:46:23]

What? Use NULL. Also, won't |previous_segment| *always* be NULL, since it's only
set to something valid in the else block (which can only be reached by
|previous_segment| being not NULL)?

[ckocagil, 2013/08/23 21:45:34]

Oops. Fixed the logic here and it uses NULL now. While fixing this test I
discovered some bugs and did a little refactoring on the way. Sorry for doing it
this late in the review process.

+      } else {
+        if (!run->script_analysis.fRTL) {
+          if (previous_segment->x_pos.end() != segment->x_pos.start())
+            return false;

[msw, 2013/08/21 16:46:23]

Consider turning these into EXPECT_EQ or ASSERT_EQ and using SCOPED_TRACE to
give context to the line number and segment number, then this function can drop
the bool return value:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

[ckocagil, 2013/08/23 21:45:34]

Done.

+        } else if (i != previous_segment_line) {
+          if (segment->x_pos.end() != previous_segment->x_pos.start())
+            return false;
+        }
+
+        previous_segment = segment;
+        previous_segment_line = i;
+      }
+    }
+
+  return true;
+}
+#endif  // defined(OS_WIN)
 
 }  // namespace
 
@@ -1294,7 +1325,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 +1334,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 +1365,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 +1376,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 +1681,25 @@ TEST_F(RenderTextTest, SelectionKeepsLigatures) {
     render_text->MoveCursorTo(SelectionModel(0, CURSOR_FORWARD));
   }
 }
+
+#if defined(OS_WIN)
+TEST_F(RenderTextTest, Multiline) {

[msw, 2013/08/21 16:46:23]

Add some more tests for the approximate size and/or line count with:
1) A huge display rect and text with '\n', '\r', '\r\n', etc.
(will it respect line breaks in the text if wrapping isn't *needed*?)
2) A reasonable display rect and strings of various length.
(does wrapping work reasonably well for common strings?)
3) A reasonable display rect and strings with very long words.
(how are long words, perhaps with hyphen/punctuation, handled?)
4) A reasonable display rect and LTR/RTL/BiDi/Complex strings.
(do line breaks around LTR/RTL in each directionality work ok, etc.)
And probably more, but that seems like a good start...

[ckocagil, 2013/08/23 21:45:34]

I added some new test cases. Note that I can't add 1) yet because line break
characters aren't implemented yet. I'm trying to come up with more test cases
and other ways of testing this class.

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

Hmm, I think base::i18n::BreakIterator should handle newline characters. Also,
you might look at break_iterator_unittest.cc or existing strings used in
multi-line labels for test ideas. I think you're off to a good start for
testing, but this definitely needs more test cases and varied expectations. My
first thought is that you need to thoroughly test multi-line cases where each
line has more than one segment. It's ok to leave some testing ideas in a TODO or
bug comment for a followup CL.

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

BreakIterator only gives positions that are suitable for breaking lines at, it
doesn't tell where the newline characters are. I was planning on adding newline
support in another CL. I'm adding a bug comment to extend the tests.

+  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());

[msw, 2013/08/21 16:46:23]

Why does this set an empty display rect? Wouldn't that be an edge case?

[ckocagil, 2013/08/23 21:45:34]

An empty (or 1-pixel wide) display rect forces RenderTextWin to put every
character in a new line. This generates the maximum number of lines, which is
hard for the class to do correctly and a good case to test.

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

Cool, 1px makes more sense; I figure 0px should perhaps not break lines.

+  render_text->SetMultiline(true);
+  Canvas canvas;
+
+  for (size_t i = 0; i < arraysize(kTestStrings); ++i) {
+    render_text->SetText(WideToUTF16(kTestStrings[i]));
+    render_text->Draw(&canvas);
+    EXPECT_TRUE(CheckLineIntegrity(render_text->lines_, render_text->runs_)) <<
+        "For kTestStrings[" << i << "]";

[msw, 2013/08/21 16:46:23]

Use SCOPED_TRACE here too, instead of piping output to EXPECT_TRUE, to give each
CheckLineIntegrity call its context.

[ckocagil, 2013/08/23 21:45:34]

Done.

+  }
+}
+#endif  // defined(OS_WIN)
 
 }  // namespace gfx