[LargeIconService] Allow decoding of images in the service

components/favicon/core/large_icon_service_unittest.cc

Index: components/favicon/core/large_icon_service_unittest.cc
diff --git a/components/favicon/core/large_icon_service_unittest.cc b/components/favicon/core/large_icon_service_unittest.cc
index fd9cfa4df6bd53efd5ccef0cea22f9f95c75f7b6..763e8e6ad95253dc6d9d65b7a32fadd82ff5afe0 100644
--- a/components/favicon/core/large_icon_service_unittest.cc
+++ b/components/favicon/core/large_icon_service_unittest.cc
@@ -29,6 +29,7 @@
 #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 {
@@ -103,37 +104,9 @@ class LargeIconServiceTest : public testing::Test {
       : 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_;
@@ -141,132 +114,11 @@ class LargeIconServiceTest : public testing::Test {
   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"
@@ -380,5 +232,183 @@ TEST_F(LargeIconServiceTest, ShoutNotGetFromGoogleServerIfUnavailable) {
   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