[RenderText] Adding vertical alignment options

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 42825579ec6cb0b68fe701de431a2c1444d5c1f7..fd15421029d523b4ee98d7ef8b9640e33ffab9e5 100644
--- a/ui/gfx/render_text_unittest.cc
+++ b/ui/gfx/render_text_unittest.cc
@@ -202,6 +202,27 @@ class TestRectangleBuffer {
     }
   }
 
+  void EnsureSolidBorder(SkColor color, const Rect& rect) const {
+    {
+      SCOPED_TRACE("EnsureSolidBorder Top Side");
+      EnsureSolidRect(SK_ColorWHITE, 0, 0, stride_, rect.y());
+    }
+    {
+      SCOPED_TRACE("EnsureSolidBorder Bottom Side");
+      EnsureSolidRect(SK_ColorWHITE, 0, rect.y() + rect.height(), stride_,

[msw, 2015/03/26 22:55:45]

rect.y() + rect.height() == rect.bottom()

[dschuyler, 2015/03/27 21:20:35]

Done.

+                      row_count_ - rect.y() - rect.height());

[msw, 2015/03/26 22:55:45]

- rect.y() - rect.height() == - rect.bottom()

[dschuyler, 2015/03/27 21:20:35]

Done.

+    }
+    {
+      SCOPED_TRACE("EnsureSolidBorder Left Side");
+      EnsureSolidRect(SK_ColorWHITE, 0, rect.y(), rect.x(), rect.height());
+    }
+    {
+      SCOPED_TRACE("EnsureSolidBorder Right Side");
+      EnsureSolidRect(SK_ColorWHITE, rect.x() + rect.width(), rect.y(),

[msw, 2015/03/26 22:55:45]

rect.x() + rect.width() == rect.right()

[dschuyler, 2015/03/27 21:20:35]

Done.

+                      stride_ - rect.x() - rect.width(), rect.height());

[msw, 2015/03/26 22:55:45]

- rect.x() - rect.width() == - rect.right()

[dschuyler, 2015/03/27 21:20:34]

Done.

+    }
+  }
+
  private:
   const wchar_t* string_;
   const SkColor* buffer_;
@@ -2592,6 +2613,66 @@ TEST_F(RenderTextTest, StringFitsOwnWidth) {
   EXPECT_EQ(kString, render_text->GetDisplayText());
 }
 
+TEST_F(RenderTextTest, VerticalAlignement) {

[msw, 2015/03/26 22:55:45]

nit: "Alignment"

[dschuyler, 2015/03/27 21:20:35]

Done.

+  SCOPED_TRACE("VerticalAlignement");

[msw, 2015/03/26 22:55:45]

What's the use of this SCOPED_TRACE? The failing test should be readily
identifiable without this. (otherwise, ditto nit: "Alignment")

[dschuyler, 2015/03/27 21:20:34]

Done.

+  scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
+  const wchar_t* string = L"g Hello g";
+  render_text->SetText(WideToUTF16(string));
+  render_text->ApplyStyle(BOLD, true, Range(0, 3));
+  const Size kCanvasSize(300, 50);
+  const int kTestSize = 10;
+  const Size string_size = render_text->GetStringSize();
+  const int kCenterBegin = (kCanvasSize.width() - string_size.width()) / 2;
+  const int kRightBegin = kCanvasSize.width() - string_size.width() - kTestSize;
+  const int kMiddleBegin = (kCanvasSize.height() - string_size.height()) / 2;
+  const int kBottomBegin =
+      kCanvasSize.height() - string_size.height() - kTestSize;
+
+  struct Tests {
+    HorizontalAlignment horizontal;
+    VerticalAlignment vertical;
+    int x;
+    int y;
+  } const kTests[] = {
+      {ALIGN_LEFT, VALIGN_TOP, kTestSize, kTestSize},

[msw, 2015/03/26 22:55:45]

nit: outdent all the these lines by 2 spaces

[dschuyler, 2015/03/27 21:20:35]

Afaik, it's an outstanding bug in cl format.  I.e. this is the git cl format of
this code :(

+      {ALIGN_LEFT, VALIGN_MIDDLE, kTestSize, kMiddleBegin},
+      {ALIGN_LEFT, VALIGN_BOTTOM, kTestSize, kBottomBegin},
+      {ALIGN_CENTER, VALIGN_TOP, kCenterBegin, kTestSize},
+      {ALIGN_CENTER, VALIGN_MIDDLE, kCenterBegin, kMiddleBegin},
+      {ALIGN_CENTER, VALIGN_BOTTOM, kCenterBegin, kBottomBegin},
+      {ALIGN_RIGHT, VALIGN_TOP, kRightBegin, kTestSize},
+      {ALIGN_RIGHT, VALIGN_MIDDLE, kRightBegin, kMiddleBegin},
+      {ALIGN_RIGHT, VALIGN_BOTTOM, kRightBegin, kBottomBegin},
+  };
+
+  skia::RefPtr<SkSurface> surface = skia::AdoptRef(

[msw, 2015/03/26 22:55:45]

I think painting the text and checking the painted pixels is an overly complex
and expensive way of checking alignment calculations. This test should check
GetAlignmentOffset() (or GetLineOffset()) values for each of the settings,
similar to the GetTextOffset or Multiline_HorizontalAlignment tests.

You can probably just extend the GetTextOffset test with vertical alignments,
and add a Multiline_VerticalAlignment test (or check the same info for multiline
RenderText in the same GetTextOffset test)?

[dschuyler, 2015/03/27 21:20:34]

I put in some tests for the get alignment offset.  They don't seem to catch
errors on Mac where the text still draws in the wrong place.  Though maybe the
pixel testing should be an entirely separate test from a set/get alignment test?
 I.e. This kind of test does seem useful to do - are you suggesting that it be
done as a separate test?  (That separate test would also need to set alignment
values to see the issues).

[msw, 2015/03/30 19:19:12]

How many ms do the pixel tests take on the bots? I suspect their value
diminishes with the cost of each new test. I don't think we ultimately want to
do this expensive testing on every variation of settings that affect text
rendering. Just checking the alignment/line offset here should suffice, as long
as some other test(s) perform a pixel check with non-zero line offsets
(TextDoesntClip setting the display rect should suffice). ie. We should have
separate tests for (1) checking alignment offsets given the alignment setting
and (2) for checking that the text paints respecting some alignment offset(s).

[dschuyler, 2015/03/31 18:38:14]

Done.

+      SkSurface::NewRasterN32Premul(kCanvasSize.width(), kCanvasSize.height()));
+  scoped_ptr<Canvas> canvas(
+      Canvas::CreateCanvasWithoutScaling(surface->getCanvas(), 1.0f));
+  render_text->SetColor(SK_ColorBLACK);
+  Rect bounds = Rect(kTestSize, kTestSize, kCanvasSize.width() - kTestSize * 2,
+                     kCanvasSize.height() - kTestSize * 2);
+  render_text->SetDisplayRect(bounds);
+  // Allow the RenderText to paint outside of its display rect.
+  render_text->set_clip_to_display_rect(false);
+  const uint32* buffer =
+      static_cast<const uint32*>(surface->peekPixels(nullptr, nullptr));
+  ASSERT_NE(nullptr, buffer);
+  TestRectangleBuffer rect_buffer(string, buffer, kCanvasSize.width(),
+                                  kCanvasSize.height());
+  ASSERT_LE(string_size.width() + kTestSize * 2, kCanvasSize.width());
+  for (const auto& test : kTests) {
+    surface->getCanvas()->clear(SK_ColorWHITE);
+    render_text->SetHorizontalAlignment(test.horizontal);
+    render_text->SetVerticalAlignment(test.vertical);
+    Rect expected_rect(test.x, test.y, string_size.width(),
+                       string_size.height());
+    expected_rect.Inset(-1, -1);
+    render_text->Draw(canvas.get());
+    rect_buffer.EnsureSolidBorder(SK_ColorWHITE, expected_rect);
+  }
+}
+
 // TODO(derat): Figure out why this fails on Windows: http://crbug.com/427184
 #if !defined(OS_WIN)
 // Ensure that RenderText examines all of the fonts in its FontList before
@@ -2699,12 +2780,13 @@ TEST_F(RenderTextTest, TextDoesntClip) {
     ASSERT_LE(string_size.width() + kTestSize * 2, kCanvasSize.width());
 
     render_text->Draw(canvas.get());
-    ASSERT_LT(string_size.width() + kTestSize, kCanvasSize.width());
     const uint32* buffer =
         static_cast<const uint32*>(surface->peekPixels(nullptr, nullptr));
     ASSERT_NE(nullptr, buffer);
     TestRectangleBuffer rect_buffer(string, buffer, kCanvasSize.width(),
                                     kCanvasSize.height());
+    // TODO(dschuyler): After platform issued are resolved below, change to

[msw, 2015/03/26 22:55:45]

nit: "issues"

[dschuyler, 2015/03/27 21:20:34]

Done.

+    // using EnsureSolidBorder() rather than EnsureSolidRect().
     {
 #if !defined(OS_CHROMEOS)
       // TODO(dschuyler): On ChromeOS text draws above the GetStringSize rect.
@@ -2754,6 +2836,7 @@ TEST_F(RenderTextTest, TextDoesntClip) {
 // Ensure that the text will clip to the display rect. Draws to a canvas and
 // checks whether any pixel beyond the bounding rectangle is colored.
 TEST_F(RenderTextTest, TextDoesClip) {
+  SCOPED_TRACE("TextDoesClip");

[msw, 2015/03/26 22:55:45]

Ditto, what's the use of this SCOPED_TRACE? The failing test should be readily
identifiable without this.

   const wchar_t* kTestStrings[] = {L"TEST", L"W", L"WWWW", L"gAXAXWWWW"};
   const Size kCanvasSize(300, 50);
   const int kTestSize = 10;
@@ -2772,37 +2855,17 @@ TEST_F(RenderTextTest, TextDoesClip) {
     const Size string_size = render_text->GetStringSize();
     int fake_width = string_size.width() / 2;
     int fake_height = string_size.height() / 2;
-    render_text->SetDisplayRect(
-        Rect(kTestSize, kTestSize, fake_width, fake_height));
+    const Rect bounds(kTestSize, kTestSize, fake_width, fake_height);
+    render_text->SetDisplayRect(bounds);
     render_text->set_clip_to_display_rect(true);
+    ASSERT_LE(string_size.width() + kTestSize * 2, kCanvasSize.width());
     render_text->Draw(canvas.get());
-    ASSERT_LT(string_size.width() + kTestSize, kCanvasSize.width());
     const uint32* buffer =
         static_cast<const uint32*>(surface->peekPixels(nullptr, nullptr));
     ASSERT_NE(nullptr, buffer);
     TestRectangleBuffer rect_buffer(string, buffer, kCanvasSize.width(),
                                     kCanvasSize.height());
-    {
-      SCOPED_TRACE("TextDoesClip Top Side");
-      rect_buffer.EnsureSolidRect(SK_ColorWHITE, 0, 0, kCanvasSize.width(),
-                                  kTestSize);
-    }
-
-    {
-      SCOPED_TRACE("TextDoesClip Bottom Side");
-      rect_buffer.EnsureSolidRect(SK_ColorWHITE, 0, kTestSize + fake_height,
-                                  kCanvasSize.width(), kTestSize);
-    }
-    {
-      SCOPED_TRACE("TextDoesClip Left Side");
-      rect_buffer.EnsureSolidRect(SK_ColorWHITE, 0, kTestSize, kTestSize,
-                                  fake_height);
-    }
-    {
-      SCOPED_TRACE("TextDoesClip Right Side");
-      rect_buffer.EnsureSolidRect(SK_ColorWHITE, kTestSize + fake_width,
-                                  kTestSize, kTestSize, fake_height);
-    }
+    rect_buffer.EnsureSolidBorder(SK_ColorWHITE, bounds);
   }
 }