[Doodle] Move image fetching from LogoBridge to DoodleService

components/doodle/doodle_service_unittest.cc

Index: components/doodle/doodle_service_unittest.cc
diff --git a/components/doodle/doodle_service_unittest.cc b/components/doodle/doodle_service_unittest.cc
index 1fefbc10fca2f50071ab180006d1d76a568bab80..b665e583bc2094d3ad702b0b59229609eaa0154a 100644
--- a/components/doodle/doodle_service_unittest.cc
+++ b/components/doodle/doodle_service_unittest.cc
@@ -12,16 +12,26 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/test/histogram_tester.h"
+#include "base/test/mock_callback.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
+#include "components/image_fetcher/core/image_fetcher.h"
+#include "components/image_fetcher/core/request_metadata.h"
 #include "components/prefs/testing_pref_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/image/image.h"
+#include "ui/gfx/image/image_unittest_util.h"
 
+using image_fetcher::ImageFetcher;
+using image_fetcher::RequestMetadata;
+using testing::_;
 using testing::Eq;
 using testing::StrictMock;
+using testing::Not;
 
 namespace doodle {
 
@@ -60,10 +70,75 @@ class MockDoodleObserver : public DoodleService::Observer {
   MOCK_METHOD1(OnDoodleConfigRevalidated, void(bool));
 };
 
+class FakeImageFetcher : public ImageFetcher {
+ public:
+  FakeImageFetcher() = default;
+  ~FakeImageFetcher() override {
+    // To make sure we didn't receive an unexpected image request.
+    CHECK(!HasPendingRequest());

[fhorschig, 2017/05/15 13:54:35]

s/CHECK/DCHECK?
Also: do we really want a crash here?
ASSERT_TRUE'ing that on TearDown would have the test fail (because the tested
code failed) and not crashing (which would imply the testing code is broken).

[Marc Treib, 2017/05/15 14:25:22]

CHECK or DCHECK doesn't really make a difference, since the trybots build with
DCHECK_ALWAYS_ON anyway. But sure, done, for consistency.

You're right that ASSERT would be better than (D)CHECK everywhere here, but
ASSERT doesn't work in all contexts, it only works in methods returning void.
And when used outside the test body itself, it might not do what you think - it
doesn't cancel the rest of the test.

Anyway. I've made this one an ASSERT in DestroyService. Just TearDown wouldn't
check if a test re-creates the service in the middle.

+  }
+
+  void SetImageFetcherDelegate(image_fetcher::ImageFetcherDelegate*) override {

[fhorschig, 2017/05/15 13:54:35]

Why not "// Ignored"?
(I am not sure, a test should _crash_ just because this function was used.)

[Marc Treib, 2017/05/15 14:25:22]

Well, if someone calls this, then they expect something that the fake doesn't
support. I'd say crashing the test is okay; I certainly want to be notified
about it in some way.

+    NOTREACHED();
+  }
+
+  void SetDataUseServiceName(DataUseServiceName) override {
+    // Ignored.
+  }
+
+  void SetImageDownloadLimit(base::Optional<int64_t>) override {
+    // Ignored.
+  }
+
+  void SetDesiredImageFrameSize(const gfx::Size&) override { NOTREACHED(); }

[fhorschig, 2017/05/15 13:54:35]

Again, why not "// Ignored"?

[Marc Treib, 2017/05/15 14:25:22]

Eh.. same argument as above, though I guess here it doesn't really matter. So,
done.

+
+  void StartOrQueueNetworkRequest(
+      const std::string& id,
+      const GURL& url,
+      const ImageFetcherCallback& callback) override {
+    // For simplicity, the fake doesn't support multiple concurrent requests.
+    CHECK(!HasPendingRequest());

[fhorschig, 2017/05/15 13:54:35]

nit: s/CHECK/DCHECK?

[Marc Treib, 2017/05/15 14:25:22]

Done.

+
+    pending_id_ = id;
+    pending_url_ = url;
+    pending_callback_ = callback;
+  }
+
+  image_fetcher::ImageDecoder* GetImageDecoder() override {
+    NOTREACHED();
+    return nullptr;
+  }
+
+  bool HasPendingRequest() const { return !pending_callback_.is_null(); }
+
+  const GURL& pending_url() const { return pending_url_; }
+
+  void RespondToPendingRequest(const gfx::Image& image) {
+    CHECK(HasPendingRequest());

[fhorschig, 2017/05/15 13:54:35]

nit: s/CHECK/DCHECK?

[Marc Treib, 2017/05/15 14:25:22]

Done.

+
+    RequestMetadata metadata;
+    metadata.http_response_code = 200;
+    pending_callback_.Run(pending_id_, image, metadata);
+
+    pending_id_.clear();
+    pending_url_ = GURL();
+    pending_callback_.Reset();
+  }
+
+ private:
+  std::string pending_id_;
+  GURL pending_url_;
+  ImageFetcherCallback pending_callback_;
+};
+
 DoodleConfig CreateConfig(DoodleType type) {
   return DoodleConfig(type, DoodleImage(GURL("https://doodle.com/image.jpg")));
 }
 
+MATCHER(IsEmptyImage, "") {
+  return arg.IsEmpty();
+}
+
 }  // namespace
 
 class DoodleServiceTest : public testing::Test {
@@ -83,7 +158,13 @@ class DoodleServiceTest : public testing::Test {
     RecreateServiceWithZeroRefreshInterval();
   }
 
-  void DestroyService() { service_ = nullptr; }
+  void DestroyService() {
+    fetcher_ = nullptr;
+    expiry_timer_ = nullptr;
+    image_fetcher_ = nullptr;

[fhorschig, 2017/05/15 13:54:35]

This could be a good place to ASSERT HasPendingRequest ... if so, renaming this
to "VerifyAndDestroy..." or something might make sense.

[Marc Treib, 2017/05/15 14:25:22]

Yup, moved the ASSERT here. IMO the name is fine though.

+
+    service_ = nullptr;
+  }
 
   void RecreateServiceWithZeroRefreshInterval() {
     RecreateService(/*min_refresh_interval=*/base::TimeDelta());
@@ -97,14 +178,18 @@ class DoodleServiceTest : public testing::Test {
     auto fetcher = base::MakeUnique<FakeDoodleFetcher>();
     fetcher_ = fetcher.get();
 
+    auto image_fetcher = base::MakeUnique<FakeImageFetcher>();
+    image_fetcher_ = image_fetcher.get();
+
     service_ = base::MakeUnique<DoodleService>(
         &pref_service_, std::move(fetcher), std::move(expiry_timer),
         task_runner_->GetMockClock(), task_runner_->GetMockTickClock(),
-        refresh_interval);
+        refresh_interval, std::move(image_fetcher));
   }
 
   DoodleService* service() { return service_.get(); }
   FakeDoodleFetcher* fetcher() { return fetcher_; }
+  FakeImageFetcher* image_fetcher() { return image_fetcher_; }
 
   base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); }
 
@@ -120,6 +205,7 @@ class DoodleServiceTest : public testing::Test {
   // Weak, owned by the service.
   FakeDoodleFetcher* fetcher_;
   base::OneShotTimer* expiry_timer_;
+  FakeImageFetcher* image_fetcher_;
 };
 
 TEST_F(DoodleServiceTest, FetchesConfigOnRefresh) {
@@ -597,4 +683,53 @@ TEST_F(DoodleServiceTest, DoesNotRecordMetricsWhenConfigExpires) {
   histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
 }
 
+TEST_F(DoodleServiceTest, GetImageWithEmptyConfigReturnsImmediately) {
+  ASSERT_THAT(service()->config(), Eq(base::nullopt));
+
+  base::MockCallback<DoodleService::ImageCallback> callback;
+  EXPECT_CALL(callback, Run(IsEmptyImage()));
+
+  service()->GetImage(callback.Get());

[fhorschig, 2017/05/15 13:54:35]

ASSERT_FALSE(HasPendingRequest) might ensure this doesn't accidentally pass if
it doesn't return immediately but fails instead.

[Marc Treib, 2017/05/15 14:25:22]

Er.. what?

Anyway, that assert happens during TearDown now.

+}
+
+TEST_F(DoodleServiceTest, GetImageFetchesLargeImage) {
+  service()->Refresh();
+  DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
+                               base::TimeDelta::FromHours(1), config);
+  ASSERT_THAT(service()->config(), Eq(config));
+
+  base::MockCallback<DoodleService::ImageCallback> callback;
+  service()->GetImage(callback.Get());
+
+  EXPECT_EQ(config.large_image.url, image_fetcher()->pending_url());
+
+  EXPECT_CALL(callback, Run(Not(IsEmptyImage())));
+  gfx::Image image = gfx::test::CreateImage(1, 1);
+  image_fetcher()->RespondToPendingRequest(image);

[fhorschig, 2017/05/15 13:54:35]

ASSERT_TRUE(HasPendingRequest) before that would have the test fail and not
crash.

[Marc Treib, 2017/05/15 14:25:22]

Done.

+}
+
+TEST_F(DoodleServiceTest, GetImageFetchesCTAImage) {
+  service()->Refresh();
+  DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
+  // Set a CTA image, which should take precedence over the regular image.
+  config.large_image.is_animated_gif = true;
+  config.large_cta_image = DoodleImage(GURL("https://doodle.com/cta.jpg"));
+  config.large_cta_image->is_cta = true;
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
+                               base::TimeDelta::FromHours(1), config);
+  ASSERT_THAT(service()->config(), Eq(config));
+
+  base::MockCallback<DoodleService::ImageCallback> callback;
+  service()->GetImage(callback.Get());
+
+  // If the doodle has a CTA image, that should loaded instead of the regular
+  // large image.
+  EXPECT_EQ(config.large_cta_image->url, image_fetcher()->pending_url());
+
+  EXPECT_CALL(callback, Run(Not(IsEmptyImage())));
+  gfx::Image image = gfx::test::CreateImage(1, 1);
+  image_fetcher()->RespondToPendingRequest(image);

[fhorschig, 2017/05/15 13:54:35]

again ASSERT_FALSE(HasPendingRequest)?

[Marc Treib, 2017/05/15 14:25:22]

ASSERT_TRUE :)  but done.

+}
+
 }  // namespace doodle