[ash-md] Reduce thread hopping for cheap wallpaper color extraction.

components/wallpaper/wallpaper_color_calculator_unittest.cc

Index: components/wallpaper/wallpaper_color_calculator_unittest.cc
diff --git a/components/wallpaper/wallpaper_color_calculator_unittest.cc b/components/wallpaper/wallpaper_color_calculator_unittest.cc
index 31268f70e3be1b2f14f7d31586a4dc40d8dfe25e..148c527088edf736ebaf00b308076b7872265de8 100644
--- a/components/wallpaper/wallpaper_color_calculator_unittest.cc
+++ b/components/wallpaper/wallpaper_color_calculator_unittest.cc
@@ -8,6 +8,7 @@
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/test/histogram_tester.h"
 #include "base/test/null_task_runner.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -27,6 +28,18 @@ namespace {
 
 const SkColor kDefaultColor = SK_ColorTRANSPARENT;
 
+const SkColor kGray = SkColorSetRGB(10, 10, 10);
+
+const SkColor kVibrantGreen = SkColorSetRGB(25, 200, 25);
+
+// Image size that causes the WallpaperColorCalculator to synchronously extract
+// the prominent color.
+constexpr gfx::Size kSyncImageSize = gfx::Size(5, 5);
+
+// Image size that causes the WallpaperColorCalculator to asynchronously extract
+// the prominent color.
+constexpr gfx::Size kAsyncImageSize = gfx::Size(50, 50);
+
 class TestWallpaperColorCalculatorObserver
     : public WallpaperColorCalculatorObserver {
  public:
@@ -45,6 +58,24 @@ class TestWallpaperColorCalculatorObserver
   DISALLOW_COPY_AND_ASSIGN(TestWallpaperColorCalculatorObserver);
 };
 
+// Returns an image that will yield a color when processing it with
+// color_utils::CalculateProminentColorOfBitmap() using the LumaRange::NORMAL
+// and SaturationRange::VIBRANT values.
+gfx::ImageSkia CreateColorProducingImage(const gfx::Size& size) {
+  gfx::Canvas canvas(size, 1.0f, true);
+  canvas.DrawColor(kGray);
+  canvas.FillRect(gfx::Rect(0, 1, size.height(), 1), kVibrantGreen);
+  return gfx::ImageSkia::CreateFrom1xBitmap(canvas.GetBitmap());
+}
+
+// Returns an image that will not yield a color when processing it with
+// color_utils::CalculateProminentColorOfBitmap().
+gfx::ImageSkia CreateNonColorProducingImage(const gfx::Size& size) {
+  gfx::Canvas canvas(size, 1.0f, true);
+  canvas.DrawColor(kGray);
+  return gfx::ImageSkia::CreateFrom1xBitmap(canvas.GetBitmap());
+}
+
 }  // namespace
 
 class WallPaperColorCalculatorTest : public testing::Test {
@@ -53,19 +84,26 @@ class WallPaperColorCalculatorTest : public testing::Test {
   ~WallPaperColorCalculatorTest() override;
 
  protected:
+  // Installs the given |task_runner| globally and on the |calculator_| instance
+  // if it exists.
   void InstallTaskRunner(
       scoped_refptr<base::SingleThreadTaskRunner> task_runner);
 
-  gfx::ImageSkia image_;
+  // Creates a new |calculator_| for the given |image| and installs the current
+  // |task_runner_|.
+  void CreateCalculator(const gfx::ImageSkia& image);
 
   std::unique_ptr<WallpaperColorCalculator> calculator_;
 
+  // Required for asynchronous calculations.
   scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
 
   TestWallpaperColorCalculatorObserver observer_;
 
+  base::HistogramTester histograms_;
+
  private:
-  // Required by PostTaskAndReplyImpl.
+  // Required for asynchronous calculations, e.g. by PostTaskAndReplyImpl.
   std::unique_ptr<base::ThreadTaskRunnerHandle> task_runner_handle_;
 
   DISALLOW_COPY_AND_ASSIGN(WallPaperColorCalculatorTest);
@@ -73,22 +111,6 @@ class WallPaperColorCalculatorTest : public testing::Test {
 
 WallPaperColorCalculatorTest::WallPaperColorCalculatorTest()
     : task_runner_(new base::TestMockTimeTaskRunner()) {
-  // Creates an |image_| that will yield a non-default prominent color.
-  const gfx::Size kImageSize(300, 200);
-  const SkColor kGray = SkColorSetRGB(10, 10, 10);
-  const SkColor kVibrantGreen = SkColorSetRGB(25, 200, 25);
-
-  gfx::Canvas canvas(kImageSize, 1.0f, true);
-  canvas.FillRect(gfx::Rect(kImageSize), kGray);
-  canvas.FillRect(gfx::Rect(0, 1, 300, 1), kVibrantGreen);
-
-  image_ = gfx::ImageSkia::CreateFrom1xBitmap(canvas.GetBitmap());
-
-  calculator_ = base::MakeUnique<WallpaperColorCalculator>(
-      image_, color_utils::LumaRange::NORMAL,
-      color_utils::SaturationRange::VIBRANT, nullptr);
-  calculator_->AddObserver(&observer_);
-
   InstallTaskRunner(task_runner_);

[Evan Stade, 2017/04/25 16:24:39]

why do you call this here? Doesn't it need to be called after CreateCalculator,
which hasn't been called yet?

(I would actually make CreateCalculator with async size part of the ctor, and
then just make sure if a test calls CreateCalculator again it re-makes it
without any issue.)

[bruthig, 2017/04/25 18:16:41]

The order of CreateCalculator() and InstallTaskRunner() shouldn't matter.
CreateCalculator() already supports this.

FTR I made the ctor construct a calculator with an async, color yielding image.

 }
 
@@ -99,20 +121,73 @@ void WallPaperColorCalculatorTest::InstallTaskRunner(
   task_runner_handle_.reset();
   task_runner_handle_ =
       base::MakeUnique<base::ThreadTaskRunnerHandle>(task_runner);
-  calculator_->SetTaskRunnerForTest(task_runner);
+  if (calculator_)
+    calculator_->SetTaskRunnerForTest(task_runner);
+}
+
+void WallPaperColorCalculatorTest::CreateCalculator(
+    const gfx::ImageSkia& image) {
+  calculator_ = base::MakeUnique<WallpaperColorCalculator>(
+      image, color_utils::LumaRange::NORMAL,
+      color_utils::SaturationRange::VIBRANT, task_runner_);
+  calculator_->AddObserver(&observer_);
+}
+
+// Used to group the asynchronous calculation tests.
+typedef WallPaperColorCalculatorTest WallPaperColorCalculatorAsyncTest;
+
+TEST_F(WallPaperColorCalculatorAsyncTest, MetricsForSuccessfulExtraction) {
+  CreateCalculator(CreateColorProducingImage(kAsyncImageSize));
+
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtractionResult", 0);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.Durations", 0);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.UserDelay", 0);
+
+  EXPECT_TRUE(calculator_->StartCalculation());
+  task_runner_->RunUntilIdle();
+
+  histograms_.ExpectUniqueSample("Ash.Wallpaper.ColorExtractionResult", true,
+                                 1);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.Durations", 1);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.UserDelay", 1);
+}
+
+TEST_F(WallPaperColorCalculatorAsyncTest, MetricsWhenPostingTaskFails) {
+  CreateCalculator(CreateColorProducingImage(kAsyncImageSize));
+
+  scoped_refptr<base::NullTaskRunner> task_runner = new base::NullTaskRunner();
+  InstallTaskRunner(task_runner);
+
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtractionResult", 0);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.Durations", 0);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.UserDelay", 0);
+
+  EXPECT_FALSE(calculator_->StartCalculation());
+  task_runner_->RunUntilIdle();
+
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtractionResult", 0);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.Durations", 0);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.UserDelay", 0);
 }
 
-TEST_F(WallPaperColorCalculatorTest,
+TEST_F(WallPaperColorCalculatorAsyncTest,
        StartCalculationReturnsFalseWhenPostingTaskFails) {

[Evan Stade, 2017/04/25 16:24:39]

nit: you're already testing this with the above test. This one is redundant.

[bruthig, 2017/04/25 18:16:41]

Removed.

+  CreateCalculator(CreateColorProducingImage(kAsyncImageSize));
+
   scoped_refptr<base::NullTaskRunner> task_runner = new base::NullTaskRunner();
   InstallTaskRunner(task_runner);
   calculator_->set_prominent_color_for_test(SK_ColorBLACK);
 
   EXPECT_FALSE(calculator_->StartCalculation());
+  task_runner_->RunUntilIdle();
+
   EXPECT_EQ(kDefaultColor, calculator_->prominent_color());
 }
 
-TEST_F(WallPaperColorCalculatorTest, ObserverNotifiedOnSuccessfulCalculation) {
+TEST_F(WallPaperColorCalculatorAsyncTest,
+       ObserverNotifiedOnSuccessfulCalculation) {
+  CreateCalculator(CreateColorProducingImage(kAsyncImageSize));
+
   EXPECT_FALSE(observer_.WasNotified());
 
   EXPECT_TRUE(calculator_->StartCalculation());
@@ -122,7 +197,9 @@ TEST_F(WallPaperColorCalculatorTest, ObserverNotifiedOnSuccessfulCalculation) {
   EXPECT_TRUE(observer_.WasNotified());
 }
 
-TEST_F(WallPaperColorCalculatorTest, ColorUpdatedOnSuccessfulCalculation) {
+TEST_F(WallPaperColorCalculatorAsyncTest, ColorUpdatedOnSuccessfulCalculation) {
+  CreateCalculator(CreateColorProducingImage(kAsyncImageSize));
+
   calculator_->set_prominent_color_for_test(kDefaultColor);
 
   EXPECT_TRUE(calculator_->StartCalculation());
@@ -132,8 +209,10 @@ TEST_F(WallPaperColorCalculatorTest, ColorUpdatedOnSuccessfulCalculation) {
   EXPECT_NE(kDefaultColor, calculator_->prominent_color());
 }
 
-TEST_F(WallPaperColorCalculatorTest,
+TEST_F(WallPaperColorCalculatorAsyncTest,
        NoCrashWhenCalculatorDestroyedBeforeTaskProcessing) {
+  CreateCalculator(CreateColorProducingImage(kAsyncImageSize));
+
   EXPECT_TRUE(calculator_->StartCalculation());
   calculator_.reset();
 
@@ -144,4 +223,38 @@ TEST_F(WallPaperColorCalculatorTest,
   EXPECT_FALSE(task_runner_->HasPendingTask());
 }
 
+// Used to group the synchronous calculation tests.
+typedef WallPaperColorCalculatorTest WallPaperColorCalculatorSyncTest;
+
+TEST_F(WallPaperColorCalculatorSyncTest, MetricsForSuccessfulExtraction) {
+  CreateCalculator(CreateColorProducingImage(kSyncImageSize));
+  calculator_->SetTaskRunnerForTest(nullptr);
+
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtractionResult", 0);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.Durations", 0);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.UserDelay", 0);
+
+  EXPECT_TRUE(calculator_->StartCalculation());
+
+  histograms_.ExpectUniqueSample("Ash.Wallpaper.ColorExtractionResult", true,
+                                 1);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.Durations", 1);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.UserDelay", 0);
+}
+
+TEST_F(WallPaperColorCalculatorSyncTest, MetricsForFailedExctraction) {
+  CreateCalculator(CreateNonColorProducingImage(gfx::Size(5, 5)));

[Evan Stade, 2017/04/25 16:24:39]

nit: use the constant?

[bruthig, 2017/04/25 18:16:41]

Whoops, done.

+
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtractionResult", 0);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.Durations", 0);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.UserDelay", 0);
+
+  EXPECT_TRUE(calculator_->StartCalculation());
+
+  histograms_.ExpectUniqueSample("Ash.Wallpaper.ColorExtractionResult", false,
+                                 1);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.Durations", 1);
+  histograms_.ExpectTotalCount("Ash.Wallpaper.ColorExtraction.UserDelay", 0);
+}
+
 }  // namespace wallpaper