[LargeIconService] Allow decoding of images in the service

components/favicon/core/large_icon_service_unittest.cc

 // Copyright 2015 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 "components/favicon/core/large_icon_service.h"
 
 #include <deque>
 #include <memory>
 
 #include "base/bind.h"
@@ skipping 11 matching lines @@
 #include "components/favicon_base/favicon_types.h"
 #include "components/image_fetcher/core/image_fetcher.h"
 #include "components/image_fetcher/core/request_metadata.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image.h"
+#include "ui/gfx/image/image_skia.h"
 #include "url/gurl.h"
 
 namespace favicon {
 namespace {
 
 using testing::NiceMock;
 using testing::Return;
 using testing::SaveArg;
 using testing::_;
 
@@ skipping 54 matching lines @@
                     const ImageFetcherCallback&));
   MOCK_METHOD0(GetImageDecoder, image_fetcher::ImageDecoder*());
 };
 
 class LargeIconServiceTest : public testing::Test {
  public:
   LargeIconServiceTest()
       : mock_image_fetcher_(new NiceMock<MockImageFetcher>()),
         large_icon_service_(&mock_favicon_service_,
                             base::ThreadTaskRunnerHandle::Get(),
-                            base::WrapUnique(mock_image_fetcher_)),
-        is_callback_invoked_(false) {}
+                            base::WrapUnique(mock_image_fetcher_)) {}
 
-  ~LargeIconServiceTest() override {
-  }
-
-  void ResultCallback(const favicon_base::LargeIconResult& result) {
-    is_callback_invoked_ = true;
-
-    // Checking presence and absence of results.
-    EXPECT_EQ(expected_bitmap_.is_valid(), result.bitmap.is_valid());
-    EXPECT_EQ(expected_fallback_icon_style_ != nullptr,
-              result.fallback_icon_style != nullptr);
-
-    if (expected_bitmap_.is_valid()) {
-      EXPECT_EQ(expected_bitmap_.pixel_size, result.bitmap.pixel_size);
-      // Not actually checking bitmap content.
-    }
-    if (expected_fallback_icon_style_.get()) {
-      EXPECT_EQ(*expected_fallback_icon_style_,
-                *result.fallback_icon_style);
-    }
-  }
-
-  void InjectMockResult(
-      const GURL& page_url,
-      const favicon_base::FaviconRawBitmapResult& mock_result) {
-    EXPECT_CALL(mock_favicon_service_,
-                GetLargestRawFaviconForPageURL(page_url, _, _, _, _))
-        .WillOnce(PostReply<5>(mock_result));
-  }
+  ~LargeIconServiceTest() override {}
 
  protected:
   base::MessageLoopForIO loop_;
 
   NiceMock<MockImageFetcher>* mock_image_fetcher_;
   testing::NiceMock<MockFaviconService> mock_favicon_service_;
   LargeIconService large_icon_service_;
-  base::CancelableTaskTracker cancelable_task_tracker_;
-
-  favicon_base::FaviconRawBitmapResult expected_bitmap_;
-  std::unique_ptr<favicon_base::FallbackIconStyle>
-      expected_fallback_icon_style_;
-
-  bool is_callback_invoked_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(LargeIconServiceTest);
 };
 
-TEST_F(LargeIconServiceTest, SameSize) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(24, 24, kTestColor));
-  expected_bitmap_ = CreateTestBitmapResult(24, 24, kTestColor);
-  large_icon_service_.GetLargeIconOrFallbackStyle(
-      GURL(kDummyUrl),
-      24,  // |min_source_size_in_pixel|
-      24,  // |desired_size_in_pixel|
-      base::Bind(&LargeIconServiceTest::ResultCallback, base::Unretained(this)),
-      &cancelable_task_tracker_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(is_callback_invoked_);
-}
-
-TEST_F(LargeIconServiceTest, ScaleDown) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(32, 32, kTestColor));
-  expected_bitmap_ = CreateTestBitmapResult(24, 24, kTestColor);
-  large_icon_service_.GetLargeIconOrFallbackStyle(
-      GURL(kDummyUrl), 24, 24,
-      base::Bind(&LargeIconServiceTest::ResultCallback, base::Unretained(this)),
-      &cancelable_task_tracker_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(is_callback_invoked_);
-}
-
-TEST_F(LargeIconServiceTest, ScaleUp) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(16, 16, kTestColor));
-  expected_bitmap_ = CreateTestBitmapResult(24, 24, kTestColor);
-  large_icon_service_.GetLargeIconOrFallbackStyle(
-      GURL(kDummyUrl),
-      14,  // Lowered requirement so stored bitmap is admitted.
-      24,
-      base::Bind(&LargeIconServiceTest::ResultCallback, base::Unretained(this)),
-      &cancelable_task_tracker_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(is_callback_invoked_);
-}
-
-// |desired_size_in_pixel| == 0 means retrieve original image without scaling.
-TEST_F(LargeIconServiceTest, NoScale) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(24, 24, kTestColor));
-  expected_bitmap_ = CreateTestBitmapResult(24, 24, kTestColor);
-  large_icon_service_.GetLargeIconOrFallbackStyle(
-      GURL(kDummyUrl), 16, 0,
-      base::Bind(&LargeIconServiceTest::ResultCallback, base::Unretained(this)),
-      &cancelable_task_tracker_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(is_callback_invoked_);
-}
-
-TEST_F(LargeIconServiceTest, FallbackSinceIconTooSmall) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(16, 16, kTestColor));
-  expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
-  expected_fallback_icon_style_->background_color = kTestColor;
-  expected_fallback_icon_style_->is_default_background_color = false;
-  large_icon_service_.GetLargeIconOrFallbackStyle(
-      GURL(kDummyUrl), 24, 24,
-      base::Bind(&LargeIconServiceTest::ResultCallback, base::Unretained(this)),
-      &cancelable_task_tracker_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(is_callback_invoked_);
-}
-
-TEST_F(LargeIconServiceTest, FallbackSinceIconNotSquare) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(24, 32, kTestColor));
-  expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
-  expected_fallback_icon_style_->background_color = kTestColor;
-  expected_fallback_icon_style_->is_default_background_color = false;
-  large_icon_service_.GetLargeIconOrFallbackStyle(
-      GURL(kDummyUrl), 24, 24,
-      base::Bind(&LargeIconServiceTest::ResultCallback, base::Unretained(this)),
-      &cancelable_task_tracker_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(is_callback_invoked_);
-}
-
-TEST_F(LargeIconServiceTest, FallbackSinceIconMissing) {
-  InjectMockResult(GURL(kDummyUrl), favicon_base::FaviconRawBitmapResult());
-  // Expect default fallback style, including background.
-  expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
-  large_icon_service_.GetLargeIconOrFallbackStyle(
-      GURL(kDummyUrl), 24, 24,
-      base::Bind(&LargeIconServiceTest::ResultCallback, base::Unretained(this)),
-      &cancelable_task_tracker_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(is_callback_invoked_);
-}
-
-TEST_F(LargeIconServiceTest, FallbackSinceIconMissingNoScale) {
-  InjectMockResult(GURL(kDummyUrl), favicon_base::FaviconRawBitmapResult());
-  // Expect default fallback style, including background.
-  expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
-  large_icon_service_.GetLargeIconOrFallbackStyle(
-      GURL(kDummyUrl), 24, 0,
-      base::Bind(&LargeIconServiceTest::ResultCallback, base::Unretained(this)),
-      &cancelable_task_tracker_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(is_callback_invoked_);
-}
-
-// Oddball case where we demand a high resolution icon to scale down. Generates
-// fallback even though an icon with the final size is available.
-TEST_F(LargeIconServiceTest, FallbackSinceTooPicky) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(24, 24, kTestColor));
-  expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
-  expected_fallback_icon_style_->background_color = kTestColor;
-  expected_fallback_icon_style_->is_default_background_color = false;
-  large_icon_service_.GetLargeIconOrFallbackStyle(
-      GURL(kDummyUrl), 32, 24,
-      base::Bind(&LargeIconServiceTest::ResultCallback, base::Unretained(this)),
-      &cancelable_task_tracker_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(is_callback_invoked_);
-}
-
 TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServer) {
   const GURL kExpectedServerUrl(
       "https://t0.gstatic.com/faviconV2?user=chrome&drop_404_icon=true"
       "&size=192&min_size=42&max_size=256&fallback_opts=TYPE"
       "&url=http://www.example.com/");
 
   EXPECT_CALL(mock_favicon_service_, UnableToDownloadFavicon(_)).Times(0);
 
   base::MockCallback<base::Callback<void(bool success)>> callback;
   EXPECT_CALL(*mock_image_fetcher_,
@@ skipping 93 matching lines @@
 
   base::MockCallback<base::Callback<void(bool success)>> callback;
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           GURL(kDummyUrl), /*min_source_size_in_pixel=*/42, callback.Get());
 
   EXPECT_CALL(callback, Run(false));
   base::RunLoop().RunUntilIdle();
 }
 
+class LargeIconServiceGetterTest : public LargeIconServiceTest,
+                                   public ::testing::WithParamInterface<bool> {
+ public:
+  LargeIconServiceGetterTest()
+      : LargeIconServiceTest(), is_callback_invoked_(false) {}
+  ~LargeIconServiceGetterTest() override {}
+
+  void GetLargeIconOrFallbackStyleAndWaitForCallback(
+      const GURL& page_url,
+      int min_source_size_in_pixel,
+      int desired_size_in_pixel,
+      bool get_raw_bitmap) {

[pkotwicz, 2017/04/04 12:43:43]

- Maybe pass in |expected_bitmap| and |expected_bitmap| as parameters and call
this method RunTest()
- You should be able to remove the |get_raw_bitmap| parameter. And call
GetParam() within this method

[jkrcal, 2017/04/04 14:44:40]

As regards the first point: 
I want to have some expectations in the tests. I've rewritten the tests so that
the callback only stores the results and the tests now verify expectations. In
my personal opinion, it is much clearer now.

As regards the second point: done!

[pkotwicz, 2017/04/04 17:47:15]

I like your new version too!

[jkrcal, 2017/04/05 15:34:38]

Acknowledged.

+    if (get_raw_bitmap) {
+      large_icon_service_.GetLargeIconOrFallbackStyle(
+          page_url, min_source_size_in_pixel, desired_size_in_pixel,
+          base::Bind(&LargeIconServiceGetterTest::RawBitmapResultCallback,
+                     base::Unretained(this)),
+          &cancelable_task_tracker_);
+    } else {
+      large_icon_service_.GetLargeIconImageOrFallbackStyle(
+          page_url, min_source_size_in_pixel, desired_size_in_pixel,
+          base::Bind(&LargeIconServiceGetterTest::ImageResultCallback,
+                     base::Unretained(this)),
+          &cancelable_task_tracker_);
+    }
+    base::RunLoop().RunUntilIdle();

[pkotwicz, 2017/04/04 12:43:43]

Nit: Make this base::RunLoop().Run() instead and make RawBitmapResultCallback()
and ImageResultCallback() call the base::RunLoop::QuitClosure(). This will
enable you to get rid of |is_callback_invoked_|

[jkrcal, 2017/04/04 14:44:40]

I got rid of the |is_callback_invoked| without this change. I do not see any
other benefit of this change (subject to personal taste).

[pkotwicz, 2017/04/04 17:47:15]

Cool: looks like using the QuitClosure is unnecessary.

[jkrcal, 2017/04/05 15:34:38]

Acknowledged.

+  }
+
+  void RawBitmapResultCallback(const favicon_base::LargeIconResult& result) {
+    is_callback_invoked_ = true;
+
+    // Checking presence and absence of results.
+    EXPECT_EQ(expected_bitmap_.is_valid(), result.bitmap.is_valid());
+    EXPECT_EQ(expected_fallback_icon_style_ != nullptr,
+              result.fallback_icon_style != nullptr);
+
+    if (expected_bitmap_.is_valid()) {
+      EXPECT_EQ(expected_bitmap_.pixel_size, result.bitmap.pixel_size);
+      // Not actually checking bitmap content.
+    }
+    if (expected_fallback_icon_style_.get()) {
+      EXPECT_EQ(*expected_fallback_icon_style_, *result.fallback_icon_style);
+    }
+  }
+
+  void ImageResultCallback(const favicon_base::LargeIconImageResult& result) {
+    is_callback_invoked_ = true;
+
+    // Checking presence and absence of results.
+    EXPECT_EQ(expected_bitmap_.is_valid(), !result.image.IsEmpty());
+    EXPECT_EQ(expected_fallback_icon_style_ != nullptr,
+              result.fallback_icon_style != nullptr);
+
+    if (expected_bitmap_.is_valid()) {
+      EXPECT_EQ(expected_bitmap_.pixel_size,
+                result.image.ToImageSkia()->size());
+      // Not actually checking bitmap content.
+    }
+    if (expected_fallback_icon_style_.get()) {
+      EXPECT_EQ(*expected_fallback_icon_style_, *result.fallback_icon_style);
+    }

[pkotwicz, 2017/04/04 12:43:43]

Nit: Can you extract the common functionality of RawBitmapResultCallback() and
ImageResultCallback() into a shared function?

e.g.
void CheckCallback(bool is_bitmap_valid, const gfx::Size& bitmap_size,
favicon_base::FallbackIconStyle* fallback_style) {}

[jkrcal, 2017/04/04 14:44:40]

I extracted a common part of that.

+  }
+
+  void InjectMockResult(
+      const GURL& page_url,
+      const favicon_base::FaviconRawBitmapResult& mock_result) {
+    EXPECT_CALL(mock_favicon_service_,

[pkotwicz, 2017/04/04 12:43:43]

Can this be ON_CALL() and WillByDefault() instead?

[jkrcal, 2017/04/04 14:44:40]

Done.

+                GetLargestRawFaviconForPageURL(page_url, _, _, _, _))
+        .WillOnce(PostReply<5>(mock_result));
+  }
+
+ protected:
+  base::CancelableTaskTracker cancelable_task_tracker_;
+  favicon_base::FaviconRawBitmapResult expected_bitmap_;
+  std::unique_ptr<favicon_base::FallbackIconStyle>
+      expected_fallback_icon_style_;
+
+  bool is_callback_invoked_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(LargeIconServiceGetterTest);
+};
+
+TEST_P(LargeIconServiceGetterTest, SameSize) {
+  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(24, 24, kTestColor));
+  expected_bitmap_ = CreateTestBitmapResult(24, 24, kTestColor);
+  GetLargeIconOrFallbackStyleAndWaitForCallback(
+      GURL(kDummyUrl),
+      24,  // |min_source_size_in_pixel|
+      24,  // |desired_size_in_pixel|
+      GetParam());
+  EXPECT_TRUE(is_callback_invoked_);
+}
+
+TEST_P(LargeIconServiceGetterTest, ScaleDown) {
+  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(32, 32, kTestColor));
+  expected_bitmap_ = CreateTestBitmapResult(24, 24, kTestColor);
+  GetLargeIconOrFallbackStyleAndWaitForCallback(GURL(kDummyUrl), 24, 24,
+                                                GetParam());
+  EXPECT_TRUE(is_callback_invoked_);
+}
+
+TEST_P(LargeIconServiceGetterTest, ScaleUp) {
+  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(16, 16, kTestColor));
+  expected_bitmap_ = CreateTestBitmapResult(24, 24, kTestColor);
+  GetLargeIconOrFallbackStyleAndWaitForCallback(
+      GURL(kDummyUrl),
+      14,  // Lowered requirement so stored bitmap is admitted.
+      24, GetParam());
+  EXPECT_TRUE(is_callback_invoked_);
+}
+
+// |desired_size_in_pixel| == 0 means retrieve original image without scaling.
+TEST_P(LargeIconServiceGetterTest, NoScale) {
+  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(24, 24, kTestColor));
+  expected_bitmap_ = CreateTestBitmapResult(24, 24, kTestColor);
+  GetLargeIconOrFallbackStyleAndWaitForCallback(GURL(kDummyUrl), 16, 0,
+                                                GetParam());
+  EXPECT_TRUE(is_callback_invoked_);
+}
+
+TEST_P(LargeIconServiceGetterTest, FallbackSinceIconTooSmall) {
+  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(16, 16, kTestColor));
+  expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
+  expected_fallback_icon_style_->background_color = kTestColor;
+  expected_fallback_icon_style_->is_default_background_color = false;
+  GetLargeIconOrFallbackStyleAndWaitForCallback(GURL(kDummyUrl), 24, 24,
+                                                GetParam());
+  EXPECT_TRUE(is_callback_invoked_);
+}
+
+TEST_P(LargeIconServiceGetterTest, FallbackSinceIconNotSquare) {
+  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(24, 32, kTestColor));
+  expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
+  expected_fallback_icon_style_->background_color = kTestColor;
+  expected_fallback_icon_style_->is_default_background_color = false;
+  GetLargeIconOrFallbackStyleAndWaitForCallback(GURL(kDummyUrl), 24, 24,
+                                                GetParam());
+  EXPECT_TRUE(is_callback_invoked_);
+}
+
+TEST_P(LargeIconServiceGetterTest, FallbackSinceIconMissing) {
+  InjectMockResult(GURL(kDummyUrl), favicon_base::FaviconRawBitmapResult());
+  // Expect default fallback style, including background.
+  expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
+  GetLargeIconOrFallbackStyleAndWaitForCallback(GURL(kDummyUrl), 24, 24,
+                                                GetParam());
+  EXPECT_TRUE(is_callback_invoked_);
+}
+
+TEST_P(LargeIconServiceGetterTest, FallbackSinceIconMissingNoScale) {
+  InjectMockResult(GURL(kDummyUrl), favicon_base::FaviconRawBitmapResult());
+  // Expect default fallback style, including background.
+  expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
+  GetLargeIconOrFallbackStyleAndWaitForCallback(GURL(kDummyUrl), 24, 0,
+                                                GetParam());
+  EXPECT_TRUE(is_callback_invoked_);
+}
+
+// Oddball case where we demand a high resolution icon to scale down. Generates
+// fallback even though an icon with the final size is available.
+TEST_P(LargeIconServiceGetterTest, FallbackSinceTooPicky) {
+  InjectMockResult(GURL(kDummyUrl), CreateTestBitmapResult(24, 24, kTestColor));
+  expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
+  expected_fallback_icon_style_->background_color = kTestColor;
+  expected_fallback_icon_style_->is_default_background_color = false;
+  GetLargeIconOrFallbackStyleAndWaitForCallback(GURL(kDummyUrl), 32, 24,
+                                                GetParam());
+  EXPECT_TRUE(is_callback_invoked_);
+}
+
+// Every test will appear with suffix /0 (param false) and /1 (param true), e.g.
+//  LargeIconServiceGetterTest.FallbackSinceTooPicky/0: get image.
+//  LargeIconServiceGetterTest.FallbackSinceTooPicky/1: get raw bitmap.
+INSTANTIATE_TEST_CASE_P(,  // Empty instatiation name.
+                        LargeIconServiceGetterTest,
+                        ::testing::Values(false, true));
+
 }  // namespace
 }  // namespace favicon