Ensure PopularSite icon availability in ntp_tiles.

components/ntp_tiles/icon_cacher_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.h"
+
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/favicon/core/favicon_client.h"
+#include "components/history/core/browser/history_database_params.h"
+#include "components/history/core/browser/history_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/image/image_unittest_util.h"
+
+using ::testing::_;
+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));
+};
+
+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_(nullptr, &history_service_) {

[Marc Treib, 2016/10/12 09:11:28]

/*favicon_client=*/nullptr

[sfiera, 2016/10/13 09:06:46]

Done.

+    CHECK(history_dir_.CreateUniqueTempDir());

[Marc Treib, 2016/10/12 09:11:28]

Can this be ASSERT_TRUE rather than CHECK, so the test will fail gracefully
rather than crashing?

[sfiera, 2016/10/13 09:06:46]

Not easily; ASSERT_TRUE() doesn't work well outside TEST_F() functions. Here, it
would cause the constructor to return early, but I think the test would proceed,
even though it would ultimately report failure.

[Marc Treib, 2016/10/13 12:31:16]

Alright, fair enough.

[Bernhard Bauer, 2016/10/13 13:20:09]

I'm not 100% sure -- ASSERT not only returns from the current function, but it
records a "fatal" failure", and the test framework might just check for those
before running the test. It would be worth trying it out?

[sfiera, 2016/10/14 06:27:32]

Tried it. Apparently ASSERT_* does return something—a failure message or
something like that.

+    CHECK(history_service_.Init(
+        history::HistoryDatabaseParams(history_dir_.GetPath(), 0, 0)));

[Marc Treib, 2016/10/12 09:11:28]

misaligned?

[sfiera, 2016/10/13 09:06:46]

Indentation, you mean? I've run git cl format, so I think it approves.

[Marc Treib, 2016/10/13 12:31:16]

Yes, I meant indentation, which looks a bit weird to me. But I'll submit to our
overlord git cl format :)

[Bernhard Bauer, 2016/10/13 13:20:09]

Just FTR, that is not actually how it works: clang-format does not override the
style guide (or a reviewer's reasonable interpretation thereof), even if it
might appear easier to just always rely on its output. With that being said, in
this case the indentation looks fine to me.

[sfiera, 2016/10/14 06:27:32]

No, that is how it works:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
"By policy, Clang's formatting of code should always be accepted in code
reviews."

Of course, there are things that clang-format doesn't care about (like line
120), so using it doesn't mean you can ignore questions of formatting entirely.
Its indentation should always be acceptable, though.

+  }
+
+  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) {
+    base::CancelableTaskTracker tracker;
+    bool is_cached;
+    base::RunLoop loop;
+    favicon::GetFaviconImageForPageURL(
+        &favicon_service_, url, icon_type,
+        base::Bind(
+            [](bool* is_cached, base::RunLoop* loop,
+               const favicon_base::FaviconImageResult& result) {
+              *is_cached = !result.image.IsEmpty();
+              loop->Quit();
+            },
+            &is_cached, &loop),
+        &tracker);
+    loop.Run();
+    return is_cached;
+  }
+
+  base::MessageLoop message_loop_;
+  PopularSites::Site site_;
+  MockImageFetcher* const image_fetcher_;
+  base::ScopedTempDir history_dir_;
+  history::HistoryService history_service_;
+  favicon::FaviconService favicon_service_;
+};
+
+template <typename Fn>
+base::Callback<Fn> BindMockFunction(MockFunction<Fn>* function) {
+  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;
+  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);
+
+  IconCacher cacher(&favicon_service_, base::WrapUnique(image_fetcher_));

[Marc Treib, 2016/10/12 09:11:28]

The WrapUnique is quite unsafe - it needs to happen exactly once per test,
otherwise we'll either leak memory or delete more than once.
Can you make the ImageFetcher a local variable in the test? Then I'd be more
comfortable with retaining the raw pointer after passing out ownership.

[sfiera, 2016/10/13 09:06:46]

Switched to a std::unique_ptr<> in the fixture and std::move(). The latter could
be done with a more explicit "TransferOwnership()" function if you'd prefer.

(originally the IconCacher was also owned by the fixture)

+  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, LargeNotCachedAndFetchSucceeded) {
+  MockFunction<void(bool)> 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));
+  }
+
+  IconCacher cacher(&favicon_service_, base::WrapUnique(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;
+  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));
+  }
+
+  IconCacher cacher(&favicon_service_, base::WrapUnique(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;
+  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));
+  }
+
+  IconCacher cacher(&favicon_service_, base::WrapUnique(image_fetcher_));
+  cacher.StartFetch(site_, BindMockFunction(&done));
+  loop.Run();
+  EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::FAVICON));
+  EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON));
+}
+
+}  // namespace
+}  // namespace ntp_tiles