Extend LargeIconService to fetch missing favicons from a Google server

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 7e0b7dce22f8fe4b62c1afa9432d1e814f285379..bfd2a8f1ba24520c75a18e2adf5d1c25500f0c89 100644
--- a/components/favicon/core/large_icon_service_unittest.cc
+++ b/components/favicon/core/large_icon_service_unittest.cc
@@ -9,15 +9,19 @@
 
 #include "base/bind.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/task/cancelable_task_tracker.h"
+#include "base/test/mock_callback.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/favicon/core/favicon_client.h"
 #include "components/favicon/core/test/mock_favicon_service.h"
 #include "components/favicon_base/fallback_icon_style.h"
 #include "components/favicon_base/favicon_types.h"
+#include "components/image_fetcher/image_fetcher.h"
+#include "components/image_fetcher/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"
@@ -30,14 +34,35 @@
 namespace favicon {
 namespace {
 
+using testing::NiceMock;
+using testing::Return;
+using testing::SaveArg;
 using testing::_;
 
 const char kDummyUrl[] = "http://www.example.com";
 const char kDummyIconUrl[] = "http://www.example.com/touch_icon.png";
 const SkColor kTestColor = SK_ColorRED;
 
-favicon_base::FaviconRawBitmapResult CreateTestBitmap(
-    int w, int h, SkColor color) {
+ACTION_P(PostFetchReply, p0) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(arg2, arg0, p0, image_fetcher::RequestMetadata()));
+}
+
+ACTION_P(PostBoolReply, p0) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                base::Bind(arg4, p0));
+}
+
+SkBitmap CreateTestSkBitmap(int w, int h, SkColor color) {
+  SkBitmap bitmap;
+  bitmap.allocN32Pixels(w, h);
+  bitmap.eraseColor(color);
+  return bitmap;
+}
+
+favicon_base::FaviconRawBitmapResult CreateTestBitmapResult(int w,
+                                                            int h,
+                                                            SkColor color) {
   favicon_base::FaviconRawBitmapResult result;
   result.expired = false;
 
@@ -56,11 +81,27 @@ favicon_base::FaviconRawBitmapResult CreateTestBitmap(
   return result;
 }
 
+class MockImageFetcher : public image_fetcher::ImageFetcher {
+ public:
+  MOCK_METHOD1(SetImageFetcherDelegate,
+               void(image_fetcher::ImageFetcherDelegate* delegate));
+  MOCK_METHOD1(SetDataUseServiceName,
+               void(image_fetcher::ImageFetcher::DataUseServiceName name));
+  MOCK_METHOD1(SetDesiredImageFrameSize, void(const gfx::Size& size));
+  MOCK_METHOD3(StartOrQueueNetworkRequest,
+               void(const std::string&,
+                    const GURL&,
+                    const ImageFetcherCallback&));
+  MOCK_METHOD0(GetImageDecoder, image_fetcher::ImageDecoder*());
+};
+
 class LargeIconServiceTest : public testing::Test {
  public:
   LargeIconServiceTest()
-      : large_icon_service_(&mock_favicon_service_,
-                            base::ThreadTaskRunnerHandle::Get()),
+      : mock_image_fetcher_(new NiceMock<MockImageFetcher>()),
+        large_icon_service_(&mock_favicon_service_,
+                            base::ThreadTaskRunnerHandle::Get(),
+                            base::WrapUnique(mock_image_fetcher_)),
         is_callback_invoked_(false) {}
 
   ~LargeIconServiceTest() override {
@@ -95,7 +136,8 @@ class LargeIconServiceTest : public testing::Test {
  protected:
   base::MessageLoopForIO loop_;
 
-  testing::StrictMock<MockFaviconService> mock_favicon_service_;
+  NiceMock<MockImageFetcher>* mock_image_fetcher_;
+  testing::NiceMock<MockFaviconService> mock_favicon_service_;
   LargeIconService large_icon_service_;
   base::CancelableTaskTracker cancelable_task_tracker_;
 
@@ -110,8 +152,8 @@ class LargeIconServiceTest : public testing::Test {
 };
 
 TEST_F(LargeIconServiceTest, SameSize) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmap(24, 24, kTestColor));
-  expected_bitmap_ = CreateTestBitmap(24, 24, kTestColor);
+  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|
@@ -123,8 +165,8 @@ TEST_F(LargeIconServiceTest, SameSize) {
 }
 
 TEST_F(LargeIconServiceTest, ScaleDown) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmap(32, 32, kTestColor));
-  expected_bitmap_ = CreateTestBitmap(24, 24, kTestColor);
+  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)),
@@ -134,8 +176,8 @@ TEST_F(LargeIconServiceTest, ScaleDown) {
 }
 
 TEST_F(LargeIconServiceTest, ScaleUp) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmap(16, 16, kTestColor));
-  expected_bitmap_ = CreateTestBitmap(24, 24, kTestColor);
+  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.
@@ -148,8 +190,8 @@ TEST_F(LargeIconServiceTest, ScaleUp) {
 
 // |desired_size_in_pixel| == 0 means retrieve original image without scaling.
 TEST_F(LargeIconServiceTest, NoScale) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmap(24, 24, kTestColor));
-  expected_bitmap_ = CreateTestBitmap(24, 24, kTestColor);
+  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)),
@@ -159,7 +201,7 @@ TEST_F(LargeIconServiceTest, NoScale) {
 }
 
 TEST_F(LargeIconServiceTest, FallbackSinceIconTooSmall) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmap(16, 16, kTestColor));
+  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;
@@ -172,7 +214,7 @@ TEST_F(LargeIconServiceTest, FallbackSinceIconTooSmall) {
 }
 
 TEST_F(LargeIconServiceTest, FallbackSinceIconNotSquare) {
-  InjectMockResult(GURL(kDummyUrl), CreateTestBitmap(24, 32, kTestColor));
+  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;
@@ -211,7 +253,7 @@ TEST_F(LargeIconServiceTest, FallbackSinceIconMissingNoScale) {
 // 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), CreateTestBitmap(24, 24, kTestColor));
+  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;
@@ -223,5 +265,78 @@ TEST_F(LargeIconServiceTest, FallbackSinceTooPicky) {
   EXPECT_TRUE(is_callback_invoked_);
 }
 
+TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServer) {
+  const std::string expected_server_url =
+      "https://t0.gstatic.com/faviconV2?user=chrome&drop_404_icon=true"
+      "&url=http://www.example.com/&size=192&min_size=42&max_size=256";
+
+  EXPECT_CALL(mock_favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+
+  base::MockCallback<base::Callback<void(bool success)>> callback;
+  EXPECT_CALL(*mock_image_fetcher_,
+              StartOrQueueNetworkRequest(expected_server_url,

[pkotwicz, 2017/03/21 18:20:30]

I don't think that it is worth testing the id parameter passed to
StartOrQueueNetworkRequest()

[mastiz, 2017/03/22 09:46:53]

Done.

+                                         GURL(expected_server_url), _))
+      .WillOnce(PostFetchReply(gfx::Image::CreateFrom1xBitmap(
+          CreateTestSkBitmap(64, 64, kTestColor))));
+  EXPECT_CALL(mock_favicon_service_,
+              SetLastResortFavicons(GURL(kDummyUrl), GURL(expected_server_url),
+                                    favicon_base::IconType::TOUCH_ICON, _, _))
+      .WillOnce(PostBoolReply(true));
+  EXPECT_CALL(callback, Run(true));
+
+  large_icon_service_
+      .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
+          GURL(kDummyUrl), /*min_source_size_in_pixel=*/42, callback.Get());
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(LargeIconServiceTest, ShouldReportUnavailableIfFetchFromServerFails) {
+  const std::string expected_server_url =
+      "https://t0.gstatic.com/faviconV2?user=chrome&drop_404_icon=true"
+      "&url=http://www.example.com/&size=192&min_size=42&max_size=256";
+
+  EXPECT_CALL(mock_favicon_service_, SetLastResortFavicons(_, _, _, _, _))
+      .Times(0);
+
+  base::MockCallback<base::Callback<void(bool success)>> callback;
+  EXPECT_CALL(*mock_image_fetcher_,
+              StartOrQueueNetworkRequest(expected_server_url,
+                                         GURL(expected_server_url), _))
+      .WillOnce(PostFetchReply(gfx::Image()));
+  EXPECT_CALL(mock_favicon_service_,
+              UnableToDownloadFavicon(GURL(expected_server_url)));
+  EXPECT_CALL(callback, Run(false));
+
+  large_icon_service_
+      .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
+          GURL(kDummyUrl), /*min_source_size_in_pixel=*/42, callback.Get());
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(LargeIconServiceTest, ShoutNotGetFromGoogleServerIfUnavailable) {
+  ON_CALL(mock_favicon_service_,
+          WasUnableToDownloadFavicon(GURL(
+              "https://t0.gstatic.com/faviconV2?user=chrome&drop_404_icon=true"
+              "&url=http://www.example.com/&size=192&min_size=42"
+              "&max_size=256")))

[pkotwicz, 2017/03/21 18:20:30]

Can you make WasUnableToDownloadFavicon() return true for all URLs?

[mastiz, 2017/03/22 09:46:53]

Leaving unmodified because that would cause tests to pass if
WasUnableToDownload() was called with |page_url| instead of |icon_url|, which is
a likely mistake. 

We need to verify that the URL is equal to the one in
ShouldReportUnavailableIfFetchFromServerFails. Tests could be rearranged to do
this differently but I think this is a good enough and simple solution.

+      .WillByDefault(Return(true));
+
+  EXPECT_CALL(mock_favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+  EXPECT_CALL(*mock_image_fetcher_, StartOrQueueNetworkRequest(_, _, _))
+      .Times(0);
+  EXPECT_CALL(mock_favicon_service_, SetLastResortFavicons(_, _, _, _, _))
+      .Times(0);
+
+  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();
+}
+
 }  // namespace
 }  // namespace favicon