Add baked-in favicons for default popular sites on NTP

components/ntp_tiles/icon_cacher_impl_unittest.cc

 // Copyright 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 "components/ntp_tiles/icon_cacher_impl.h"
 
+#include <set>
 #include <utility>
 
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
+#include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/favicon/core/favicon_client.h"
 #include "components/favicon/core/favicon_service_impl.h"
 #include "components/favicon/core/favicon_util.h"
 #include "components/history/core/browser/history_database_params.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/image_fetcher/image_fetcher.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image_unittest_util.h"
 
 using ::testing::_;
+using ::testing::Eq;
 using ::testing::InSequence;
 using ::testing::MockFunction;
 using ::testing::Return;
 
 namespace ntp_tiles {
 namespace {
 
 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_METHOD3(StartOrQueueNetworkRequest,
                void(const std::string& id,
                     const GURL& image_url,
                     base::Callback<void(const std::string& id,
                                         const gfx::Image& image)> callback));
 };
 
+// This class provides methods to inject an image resource where a real resource
+// would be necessary otherwise. All other methods have return values that allow
+// the normal implementation to proceed.
+class FakeResourceDelegate : public ui::ResourceBundle::Delegate {
+ public:
+  ~FakeResourceDelegate() override {}
+  base::FilePath GetPathForResourcePack(const base::FilePath& pack_path,
+                                        ui::ScaleFactor scale_factor) override {
+    return pack_path;  // Continue Loading pack file.

[mastiz, 2017/03/01 08:48:12]

Nit: lowercase "loading". Similar occurrences follow.

[fhorschig, 2017/03/01 20:57:35]

Done.

+  }
+
+  base::FilePath GetPathForLocalePack(const base::FilePath& pack_path,
+                                      const std::string& locale) override {
+    return pack_path;  // Continue Loading pack file.
+  }
+
+  // Returns a non-empty image for any resource and logs the resource id.
+  gfx::Image GetImageNamed(int resource_id) override {
+    requested_resources.insert(resource_id);
+    return gfx::test::CreateImage(64, 64);
+  }
+
+  gfx::Image GetNativeImageNamed(int resource_id) override {
+    return gfx::Image();  // Attempt retrieval of the default resource.
+  }
+
+  base::RefCountedMemory* LoadDataResourceBytes(
+      int resource_id,
+      ui::ScaleFactor scale_factor) override {
+    return nullptr;  // Attempt retrieval of the default resource.
+  }
+
+  bool GetRawDataResource(int resource_id,
+                          ui::ScaleFactor scale_factor,
+                          base::StringPiece* value) override {
+    return false;  // Attempt retrieval of the default resource.
+  }
+
+  bool GetLocalizedString(int message_id, base::string16* value) override {
+    return false;  // Attempt retrieval of the default resource.
+  }
+
+  // Returns whether a resource was requested at least once.
+  bool HasProvidedResource(int resource_id) {

[mastiz, 2017/03/01 08:48:12]

Have you considered using a mock instead? 

This would make most sense of only one or two fake functions above are exercised
(or even if more are used, if the default return values would suffice, which is
what you're doing anyway -- then NiceMock).

It could look like this:
EXPECT_CALL(mock_resource_delegate, GetImageNamed(12345))
    .WillOnce(Return(gfx::test::CreateImage(64, 64)));

[fhorschig, 2017/03/01 20:57:35]

I didn't. But now I did. I tried a complete Mock and a partial Mock. 
Both started to hang (for reasons I cannot explain because it only
occurred on release build tests on Android). Likely connected to the 
registering in the constructor.
I reverted after more than an hour of debugging.

[mastiz, 2017/03/02 11:05:39]

I can help you with that debugging, it could save ~50 LoC of boilterplate.

[fhorschig, 2017/03/02 11:56:24]

Thanks, it did.

+    return requested_resources.find(resource_id) != requested_resources.end();
+  }
+
+ private:
+  std::set<int> requested_resources;
+};
+
 class IconCacherTest : public ::testing::Test {
  protected:
   IconCacherTest()
       : site_(base::string16(),  // title, unused
               GURL("http://url.google/"),
               GURL("http://url.google/icon.png"),
               GURL("http://url.google/favicon.ico"),
               GURL()),  // thumbnail, unused
         image_fetcher_(new ::testing::StrictMock<MockImageFetcher>),
-        favicon_service_(/*favicon_client=*/nullptr, &history_service_) {
+        favicon_service_(/*favicon_client=*/nullptr, &history_service_),
+        task_runner_(new base::TestSimpleTaskRunner()) {
     CHECK(history_dir_.CreateUniqueTempDir());
     CHECK(history_service_.Init(
         history::HistoryDatabaseParams(history_dir_.GetPath(), 0, 0)));
+    if (ui::ResourceBundle::HasSharedInstance()) {
+      ui::ResourceBundle::CleanupSharedInstance();
+    }
   }
 
   void PreloadIcon(const GURL& url,
                    const GURL& icon_url,
                    favicon_base::IconType icon_type,
                    int width,
                    int height) {
     favicon_service_.SetFavicons(url, icon_url, icon_type,
                                  gfx::test::CreateImage(width, height));
   }
 
   bool IconIsCachedFor(const GURL& url, favicon_base::IconType icon_type) {
+    return !GetCachedIconFor(url, icon_type).IsEmpty();
+  }
+
+  gfx::Image GetCachedIconFor(const GURL& url,
+                              favicon_base::IconType icon_type) {
     base::CancelableTaskTracker tracker;
-    bool is_cached;
+    gfx::Image image;
     base::RunLoop loop;
     favicon::GetFaviconImageForPageURL(
         &favicon_service_, url, icon_type,
         base::Bind(
-            [](bool* is_cached, base::RunLoop* loop,
+            [](gfx::Image* image, base::RunLoop* loop,
                const favicon_base::FaviconImageResult& result) {
-              *is_cached = !result.image.IsEmpty();
+              *image = result.image;
               loop->Quit();
             },
-            &is_cached, &loop),
+            &image, &loop),
         &tracker);
     loop.Run();
-    return is_cached;
+    return image;
   }
 
+  void WaitForTasksToFinish() { task_runner_->RunUntilIdle(); }
+
   base::MessageLoop message_loop_;
   PopularSites::Site site_;
   std::unique_ptr<MockImageFetcher> image_fetcher_;
   base::ScopedTempDir history_dir_;
   history::HistoryService history_service_;
   favicon::FaviconServiceImpl favicon_service_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
 };
 
 template <typename Fn>
 base::Callback<Fn> BindMockFunction(MockFunction<Fn>* function) {

[mastiz, 2017/03/02 11:05:39]

Optional: there's MockCallback which is probably what these tests should use.

[fhorschig, 2017/03/02 11:56:24]

Done.

   return base::Bind(&MockFunction<Fn>::Call, base::Unretained(function));
 }
 
 ACTION(FailFetch) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(arg2, arg0, gfx::Image()));
 }
 
 ACTION_P2(PassFetch, width, height) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(arg2, arg0, gfx::test::CreateImage(width, height)));
 }
 
 ACTION_P(Quit, run_loop) {
   run_loop->Quit();
 }
 
 TEST_F(IconCacherTest, LargeCached) {
-  MockFunction<void(bool)> done;
+  MockFunction<void()> done;
+  EXPECT_CALL(done, Call()).Times(0);
   base::RunLoop loop;
   {
     InSequence s;
     EXPECT_CALL(*image_fetcher_,
                 SetDataUseServiceName(
                     data_use_measurement::DataUseUserData::NTP_TILES));
-    EXPECT_CALL(done, Call(false)).WillOnce(Quit(&loop));
   }
   PreloadIcon(site_.url, site_.large_icon_url, favicon_base::TOUCH_ICON, 128,
               128);
-
   IconCacherImpl cacher(&favicon_service_, std::move(image_fetcher_));
   cacher.StartFetch(site_, BindMockFunction(&done));
-  loop.Run();
+  WaitForTasksToFinish();
   EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::FAVICON));
   EXPECT_TRUE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON));
 }
 
 TEST_F(IconCacherTest, LargeNotCachedAndFetchSucceeded) {
-  MockFunction<void(bool)> done;
+  MockFunction<void()> done;
   base::RunLoop loop;
   {
     InSequence s;
     EXPECT_CALL(*image_fetcher_,
                 SetDataUseServiceName(
                     data_use_measurement::DataUseUserData::NTP_TILES));
     EXPECT_CALL(*image_fetcher_,
                 StartOrQueueNetworkRequest(_, site_.large_icon_url, _))
         .WillOnce(PassFetch(128, 128));
-    EXPECT_CALL(done, Call(true)).WillOnce(Quit(&loop));
+    EXPECT_CALL(done, Call()).WillOnce(Quit(&loop));
   }
 
   IconCacherImpl cacher(&favicon_service_, std::move(image_fetcher_));
   cacher.StartFetch(site_, BindMockFunction(&done));
   loop.Run();
   EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::FAVICON));
   EXPECT_TRUE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON));
 }
 
 TEST_F(IconCacherTest, SmallNotCachedAndFetchSucceeded) {
   site_.large_icon_url = GURL();
 
-  MockFunction<void(bool)> done;
+  MockFunction<void()> done;
   base::RunLoop loop;
   {
     InSequence s;
     EXPECT_CALL(*image_fetcher_,
                 SetDataUseServiceName(
                     data_use_measurement::DataUseUserData::NTP_TILES));
     EXPECT_CALL(*image_fetcher_,
                 StartOrQueueNetworkRequest(_, site_.favicon_url, _))
         .WillOnce(PassFetch(128, 128));
-    EXPECT_CALL(done, Call(true)).WillOnce(Quit(&loop));
+    EXPECT_CALL(done, Call()).WillOnce(Quit(&loop));
   }
 
   IconCacherImpl cacher(&favicon_service_, std::move(image_fetcher_));
   cacher.StartFetch(site_, BindMockFunction(&done));
   loop.Run();
   EXPECT_TRUE(IconIsCachedFor(site_.url, favicon_base::FAVICON));
   EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON));
 }
 
 TEST_F(IconCacherTest, LargeNotCachedAndFetchFailed) {
-  MockFunction<void(bool)> done;
+  MockFunction<void()> done;
+  EXPECT_CALL(done, Call()).Times(0);
   base::RunLoop loop;
   {
     InSequence s;
     EXPECT_CALL(*image_fetcher_,
                 SetDataUseServiceName(
                     data_use_measurement::DataUseUserData::NTP_TILES));
     EXPECT_CALL(*image_fetcher_,
                 StartOrQueueNetworkRequest(_, site_.large_icon_url, _))
         .WillOnce(FailFetch());
-    EXPECT_CALL(done, Call(false)).WillOnce(Quit(&loop));
   }
 
   IconCacherImpl cacher(&favicon_service_, std::move(image_fetcher_));
   cacher.StartFetch(site_, BindMockFunction(&done));
-  loop.Run();
+  WaitForTasksToFinish();
   EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::FAVICON));
   EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON));
 }
 
+TEST_F(IconCacherTest, ProvidesDefaultIconAndSucceedsWithFetching) {
+  FakeResourceDelegate fake_resource;
+  ui::ResourceBundle::InitSharedInstanceWithLocale(
+      "en-US", &fake_resource, ui::ResourceBundle::LOAD_COMMON_RESOURCES);
+  MockFunction<void()> icon_available;
+  base::RunLoop loop_for_static;
+  base::RunLoop loop_for_fetch;
+  {
+    InSequence s;
+    EXPECT_CALL(*image_fetcher_,
+                SetDataUseServiceName(
+                    data_use_measurement::DataUseUserData::NTP_TILES));
+    EXPECT_CALL(icon_available, Call()).WillOnce(Quit(&loop_for_static));
+    EXPECT_CALL(*image_fetcher_,
+                StartOrQueueNetworkRequest(_, site_.large_icon_url, _))
+        .WillOnce(PassFetch(128, 128));
+    EXPECT_CALL(icon_available, Call()).WillOnce(Quit(&loop_for_fetch));
+  }
+
+  IconCacherImpl cacher(&favicon_service_, std::move(image_fetcher_));
+  site_.default_resource_id = 12345;
+  cacher.StartFetch(site_, BindMockFunction(&icon_available));
+
+  loop_for_static.Run();  // Wait for the default image.
+  EXPECT_TRUE(fake_resource.HasProvidedResource(12345));
+  EXPECT_THAT(GetCachedIconFor(site_.url, favicon_base::TOUCH_ICON).Size(),
+              Eq(gfx::Size(64, 64)));  // Compares dimensions, not objects.
+
+  // Let the fetcher continue and wait for the second call of the callback.
+  loop_for_fetch.Run();  // Wait for the updated image.
+  EXPECT_THAT(GetCachedIconFor(site_.url, favicon_base::TOUCH_ICON).Size(),
+              Eq(gfx::Size(128, 128)));  // Compares dimensions, not objects.
+}
+
 }  // namespace
 }  // namespace ntp_tiles