Fix gfx::Canvas::DrawStringRectWithHalo

ui/aura/painting_window_unittest.cc

+// Copyright (c) 2016 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 <stddef.h>
+#include <stdint.h>
+
+#include "base/strings/utf_string_conversions.h"
+#include "cc/output/copy_output_request.h"
+#include "cc/output/copy_output_result.h"
+#include "ui/aura/test/test_windows.h"
+#include "ui/aura/window.h"
+#include "ui/compositor/paint_recorder.h"
+#include "ui/compositor/test/context_factories_for_test.h"
+#include "ui/compositor/test/draw_waiter_for_test.h"
+#include "ui/gfx/font_list.h"
+
+namespace aura {
+namespace {
+
+typedef void(*TestFunction)(gfx::Canvas*, const base::string16&,

[danakj, 2016/01/26 19:41:53]

using, not typedef

+                            const gfx::FontList&, const gfx::Rect&);
+
+class FontTextPaintingWindowDelegate : public aura::test::TestWindowDelegate {
+ public:
+  FontTextPaintingWindowDelegate(const gfx::Size& window_size,
+                                 TestFunction func)
+      : window_size_(window_size), func_(func) {
+  }
+
+  ~FontTextPaintingWindowDelegate() override {}
+
+  void OnPaint(const ui::PaintContext& context) override {
+    ui::PaintRecorder recorder(context, window_size_);
+    recorder.canvas()->DrawColor(SK_ColorBLUE);
+    const base::string16 text = base::ASCIIToUTF16("Plz, don't crash!");
+    func_(recorder.canvas(), text, font_list_, gfx::Rect(window_size_));
+  }
+
+ private:
+  gfx::Size window_size_;
+  gfx::FontList font_list_;
+  TestFunction func_;
+
+  DISALLOW_COPY_AND_ASSIGN(FontTextPaintingWindowDelegate);
+};
+
+}  // namespace
+
+class PaintingWindowTest : public testing::Test,
+                           public testing::WithParamInterface<TestFunction> {
+ public:
+  PaintingWindowTest() {}
+  ~PaintingWindowTest() override {}
+
+  void SetUp() override {
+    testing::Test::SetUp();
+
+    // The ContextFactory must exist before any Compositors are created.
+    // Snapshot test tests real drawing and readback, so needs pixel output.

[danakj, 2016/01/26 19:41:53]

"Snapshot test tests"? I see no Snapshot tests/classes in this file.

+    bool enable_pixel_output = true;
+    ui::ContextFactory* context_factory =
+        ui::InitializeContextFactoryForTests(enable_pixel_output);
+
+    helper_.reset(
+        new aura::test::AuraTestHelper(base::MessageLoopForUI::current()));
+    helper_->SetUp(context_factory);
+    gfx::FontList::SetDefaultFontDescription("Arial,Times New Roman,13px");
+  }
+
+  void TearDown() override {
+    test_window_.reset();
+    delegate_.reset();
+    helper_->RunAllPendingInMessageLoop();
+    helper_->TearDown();
+    ui::TerminateContextFactoryForTests();
+    testing::Test::TearDown();
+  }
+
+ protected:
+  aura::Window* test_window() { return test_window_.get(); }
+  aura::Window* root_window() { return helper_->root_window(); }
+  aura::TestScreen* test_screen() { return helper_->test_screen(); }
+
+  void WaitForDraw() {
+    helper_->host()->compositor()->ScheduleDraw();
+    ui::DrawWaiterForTest::WaitForCompositingEnded(
+        helper_->host()->compositor());
+  }
+
+  void SetupTestWindow(const gfx::Rect& window_bounds) {
+    delegate_.reset(
+        new FontTextPaintingWindowDelegate(window_bounds.size(), GetParam()));
+    test_window_.reset(aura::test::CreateTestWindowWithDelegate(
+        delegate_.get(), 0, window_bounds, root_window()));
+  }
+
+  void ReadPixels(SkBitmap* bitmap, gfx::Rect source_rect) {
+    scoped_refptr<ReadbackHolder> holder(new ReadbackHolder);
+    scoped_ptr<cc::CopyOutputRequest> request =
+        cc::CopyOutputRequest::CreateBitmapRequest(
+            base::Bind(&ReadbackHolder::OutputRequestCallback, holder));
+    request->set_area(source_rect);
+
+    helper_->host()->compositor()->root_layer()->RequestCopyOfOutput(
+        std::move(request));
+
+    // Wait for copy response.  This needs to wait as the compositor could
+    // be in the middle of a draw right now, and the commit with the
+    // copy output request may not be done on the first draw.
+    for (int i = 0; i < 2; i++) {
+      helper_->host()->compositor()->ScheduleFullRedraw();
+      WaitForDraw();
+    }
+
+    // Waits for the callback to finish run and return result.
+    holder->WaitForReadback();
+
+    *bitmap = holder->result();
+  }
+
+ private:
+  class ReadbackHolder : public base::RefCountedThreadSafe<ReadbackHolder> {
+   public:
+    ReadbackHolder() : run_loop_(new base::RunLoop) {}
+
+    void OutputRequestCallback(scoped_ptr<cc::CopyOutputResult> result) {
+      result_ = result->TakeBitmap();
+      run_loop_->Quit();
+    }
+
+    void WaitForReadback() { run_loop_->Run(); }
+
+    const SkBitmap& result() const { return *result_; }
+
+   private:
+    friend class base::RefCountedThreadSafe<ReadbackHolder>;
+
+    virtual ~ReadbackHolder() {}
+
+    scoped_ptr<SkBitmap> result_;
+    scoped_ptr<base::RunLoop> run_loop_;
+  };
+
+  scoped_ptr<aura::test::AuraTestHelper> helper_;
+  scoped_ptr<aura::Window> test_window_;
+  scoped_ptr<FontTextPaintingWindowDelegate> delegate_;
+  std::vector<unsigned char> png_representation_;
+  base::MessageLoopForUI message_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(PaintingWindowTest);
+};
+
+TEST_P(PaintingWindowTest, DrawString) {
+  SetupTestWindow(root_window()->bounds());
+  WaitForDraw();

[danakj, 2016/01/26 19:41:53]

why wait for draw? SetupTestWindow already did?

+
+  SkBitmap bitmap;
+  ReadPixels(&bitmap, root_window()->bounds());
+  ASSERT_FALSE(bitmap.empty());
+
+  size_t count_red = 0;
+  size_t count_blue = 0;
+
+  SkAutoLockPixels lock(bitmap);
+  for (int x = 0; x < bitmap.width(); x++) {
+    for (int y = 0; y < bitmap.height(); y++) {
+      SkColor actual_color = bitmap.getColor(x, y);
+      if (actual_color == SK_ColorRED) {
+        ++count_red;
+      } else if (actual_color == SK_ColorBLUE) {
+        ++count_blue;
+      } else {
+        EXPECT_EQ(0, SkColorGetG(actual_color));
+      }
+    }
+  }
+
+  EXPECT_NE(0u, count_blue);

[danakj, 2016/01/26 19:41:53]

Few things about these tests..

1) I'm not a big fan of unit tests which don't test something meaningful beyond
"doesn't crash". It would be better if you want to test that text is being
drawn, that you compare to a png and make sure the text was actually drawn? If
that's too heavy handed is there something else you can test instead?

2) Why is this in ui/aura? It's not testing aura things. Can you put the test
closer to the code it is testing?

3) Maybe you don't need a whole Compositor and aura::Window etc to test this,
and just making a PaintContext and drawing a string of text would be enough? I
can't tell cuz I'm not sure what the crash was though so I'm not sure what
you're trying to test.

+  EXPECT_NE(0u, count_red);
+  EXPECT_GT(count_blue, count_red);
+}
+
+INSTANTIATE_TEST_CASE_P(DrawString,
+                        PaintingWindowTest,
+                        testing::Values(
+                            [](gfx::Canvas* canvas,
+                               const base::string16& text,
+                               const gfx::FontList& font_list,
+                               const gfx::Rect& rect) {
+                                 canvas->DrawStringRectWithHalo(
+                                     text, font_list, SK_ColorRED,
+                                     SK_ColorBLUE, rect, 0);
+                            },
+                            [](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::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(1, 1),
+                                                      0, SK_ColorRED));
+                                 canvas->DrawStringRectWithShadows(
+                                     text, font_list, SK_ColorRED,
+                                     rect, 0, 0, shadows);
+                            }
+                        ));
+
+
+}  // namespace aura