Fix gfx::Canvas::DrawStringRectWithHalo

ui/compositor/layer_unittest.cc

Index: ui/compositor/layer_unittest.cc
diff --git a/ui/compositor/layer_unittest.cc b/ui/compositor/layer_unittest.cc
index d9f5a7a08dbf68dc6e5dcf3d1528975b9df6ab49..de4b221d4077b359f6eaf5df55ddda08e1b39193 100644
--- a/ui/compositor/layer_unittest.cc
+++ b/ui/compositor/layer_unittest.cc
@@ -19,6 +19,7 @@
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/layers/delegated_frame_provider.h"
 #include "cc/layers/delegated_frame_resource_collection.h"
@@ -42,10 +43,15 @@
 #include "ui/compositor/test/test_layers.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/font_list.h"
 #include "ui/gfx/gfx_paths.h"
 #include "ui/gfx/skia_util.h"
 
 using cc::MatchesPNGFile;
+using cc::WritePNGFile;
+
+using TestFunction = void(*)(gfx::Canvas*, const base::string16&,

[danakj, 2016/01/28 19:30:39]

Can you give this a better name? What is this function meant to do? What are
these arguments, some of them need names at the least.

+                             const gfx::FontList&, const gfx::Rect&);
 
 namespace ui {
 
@@ -88,6 +94,37 @@ class ColoredLayer : public Layer, public LayerDelegate {
   SkColor color_;
 };
 
+class DrawStringLayer : public Layer, public LayerDelegate {
+ public:
+  explicit DrawStringLayer(TestFunction func)
+      : Layer(LAYER_TEXTURED),
+        func_(func) {
+    set_delegate(this);
+  }
+
+  ~DrawStringLayer() override {}
+
+  // Overridden from LayerDelegate:
+  void OnPaintLayer(const ui::PaintContext& context) override {
+    ui::PaintRecorder recorder(context, size());
+    recorder.canvas()->DrawColor(SK_ColorBLUE);
+    const base::string16 text = base::ASCIIToUTF16("Plz, don't crash!");
+    func_(recorder.canvas(), text, font_list_, gfx::Rect(size()));

[danakj, 2016/01/28 19:30:39]

how about just making an enum for the various ways this can draw text, and here
do a switch on that enum? that'd be much easier to follow (no indirection)

you can pass the enum value to the constructor and store that on the class

+  }
+
+  void OnDelegatedFrameDamage(const gfx::Rect& damage_rect_in_dip) override {}
+
+  void OnDeviceScaleFactorChanged(float device_scale_factor) override {}
+
+  base::Closure PrepareForLayerBoundsChange() override {
+    return base::Closure();
+  }
+
+ private:
+  TestFunction func_;
+  gfx::FontList font_list_;
+};
+
 class LayerWithRealCompositorTest : public testing::Test {
  public:
   LayerWithRealCompositorTest() {
@@ -96,6 +133,7 @@ class LayerWithRealCompositorTest : public testing::Test {
     } else {
       LOG(ERROR) << "Could not open test data directory.";
     }
+    gfx::FontList::SetDefaultFontDescription("Arial,50px");
   }
   ~LayerWithRealCompositorTest() override {}
 
@@ -138,6 +176,12 @@ class LayerWithRealCompositorTest : public testing::Test {
     return layer;
   }
 
+  Layer* CreateDrawStringLayer(const gfx::Rect& bounds, TestFunction func) {
+    Layer* layer = new DrawStringLayer(func);
+    layer->SetBounds(bounds);
+    return layer;
+  }
+
   void DrawTree(Layer* root) {
     GetCompositor()->SetRootLayer(root);
     GetCompositor()->ScheduleDraw();
@@ -1219,6 +1263,84 @@ TEST_F(LayerWithRealCompositorTest, ModifyHierarchy) {
   EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img2, cc::ExactPixelComparator(true)));
 }
 
+TEST_F(LayerWithRealCompositorTest, CanvasDrawStringRectWithHalo) {
+  TestFunction draw_func =
+      [](gfx::Canvas* canvas,
+         const base::string16& text,
+         const gfx::FontList& font_list,
+         const gfx::Rect& rect) {
+           canvas->DrawStringRectWithHalo(
+               text, font_list, SK_ColorRED,
+               SK_ColorWHITE, rect, 0);
+      };
+  gfx::Size size(512, 512);
+  GetCompositor()->SetScaleAndSize(1.0f, size);
+  scoped_ptr<Layer> layer(CreateDrawStringLayer(gfx::Rect(size), draw_func));
+  DrawTree(layer.get());
+
+  SkBitmap bitmap;
+  ReadPixels(&bitmap);
+  ASSERT_FALSE(bitmap.empty());
+
+  base::FilePath ref_img =
+      test_data_directory().AppendASCII("string_with_halo.png");
+  WritePNGFile(bitmap, ref_img, true);

[danakj, 2016/01/28 19:30:39]

If you write the PNG every time, then it will always match? I think you want to
write it once, save the .png as part of the patch, and remove the Write from the
CL?

+  EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img, cc::ExactPixelComparator(true)));
+}
+
+TEST_F(LayerWithRealCompositorTest, CanvasDrawFadedString) {
+  TestFunction draw_func =
+      [](gfx::Canvas* canvas,
+         const base::string16& text,
+         const gfx::FontList& font_list,
+         const gfx::Rect& rect) {
+           canvas->DrawFadedString(
+               text, font_list, SK_ColorRED, rect, 0);
+      };
+  gfx::Size size(512, 512);
+  GetCompositor()->SetScaleAndSize(1.0f, size);
+  scoped_ptr<Layer> layer(CreateDrawStringLayer(gfx::Rect(size), draw_func));
+  DrawTree(layer.get());
+
+  SkBitmap bitmap;
+  ReadPixels(&bitmap);
+  ASSERT_FALSE(bitmap.empty());
+
+  base::FilePath ref_img =
+      test_data_directory().AppendASCII("string.png");

[danakj, 2016/01/28 19:30:39]

string_faded.png?

+  WritePNGFile(bitmap, ref_img, true);
+  EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img, cc::ExactPixelComparator(true)));
+}
+
+TEST_F(LayerWithRealCompositorTest, CanvasDrawStringRectWithShadows) {
+  TestFunction draw_func =
+      [](gfx::Canvas* canvas,
+         const base::string16& text,
+         const gfx::FontList& font_list,
+         const gfx::Rect& rect) {
+           gfx::ShadowValues shadows;
+           shadows.push_back(
+               gfx::ShadowValue(gfx::Vector2d(3, 3),
+                                2, SK_ColorRED));
+           canvas->DrawStringRectWithShadows(
+               text, font_list, SK_ColorRED,
+               rect, 0, 0, shadows);
+      };
+  gfx::Size size(512, 512);
+  GetCompositor()->SetScaleAndSize(1.0f, size);
+  scoped_ptr<Layer> layer(CreateDrawStringLayer(gfx::Rect(size), draw_func));
+  DrawTree(layer.get());
+
+  SkBitmap bitmap;
+  ReadPixels(&bitmap);
+  ASSERT_FALSE(bitmap.empty());
+
+  base::FilePath ref_img =
+      test_data_directory().AppendASCII("string_with_shadow.png");
+  WritePNGFile(bitmap, ref_img, true);
+  EXPECT_TRUE(MatchesPNGFile(bitmap, ref_img, cc::ExactPixelComparator(true)));
+}
+
 // Opacity is rendered correctly.
 // Checks that modifying the hierarchy correctly affects final composite.
 TEST_F(LayerWithRealCompositorTest, Opacity) {