Add request throttler to thumbnail fetching for articles on mobile NTP

components/ntp_snippets/ntp_snippets_service_unittest.cc

Index: components/ntp_snippets/ntp_snippets_service_unittest.cc
diff --git a/components/ntp_snippets/ntp_snippets_service_unittest.cc b/components/ntp_snippets/ntp_snippets_service_unittest.cc
index f826aeaf7b3198c119b51888c1cdf6bf02953a1b..d383a09923fae50276f7523a664cf7b0be52cc5f 100644
--- a/components/ntp_snippets/ntp_snippets_service_unittest.cc
+++ b/components/ntp_snippets/ntp_snippets_service_unittest.cc
@@ -5,6 +5,7 @@
 #include "components/ntp_snippets/ntp_snippets_service.h"
 
 #include <memory>
+#include <utility>
 #include <vector>
 
 #include "base/command_line.h"
@@ -23,6 +24,7 @@
 #include "base/time/time.h"
 #include "components/image_fetcher/image_decoder.h"
 #include "components/image_fetcher/image_fetcher.h"
+#include "components/image_fetcher/image_fetcher_delegate.h"
 #include "components/ntp_snippets/category_factory.h"
 #include "components/ntp_snippets/ntp_snippet.h"
 #include "components/ntp_snippets/ntp_snippets_database.h"
@@ -38,7 +40,10 @@
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/image/image.h"
 
+using image_fetcher::ImageFetcher;
+using image_fetcher::ImageFetcherDelegate;
 using testing::ElementsAre;
 using testing::Eq;
 using testing::Invoke;
@@ -69,6 +74,8 @@ const char kSnippetPublisherName[] = "Foo News";
 const char kSnippetAmpUrl[] = "http://localhost/amp";
 const float kSnippetScore = 5.0;
 
+const char kSnippetUrl2[] = "http://foo.com/bar";
+
 base::Time GetDefaultCreationTime() {
   base::Time out_time;
   EXPECT_TRUE(base::Time::FromUTCExploded(kDefaultCreationTime, &out_time));
@@ -263,6 +270,19 @@ class WaitForDBLoad {
   DISALLOW_COPY_AND_ASSIGN(WaitForDBLoad);
 };
 
+class MockImageFetcher : public ImageFetcher {
+ public:
+  void SetImageFetcherDelegate(ImageFetcherDelegate* delegate) override {}

[tschumann, 2016/08/12 09:55:36]

let's not mix up Mocks with fake implementations.

If you go with the mock, you can make all methods MOCK_METHODs and then use a
::testing::NiceMock<MockImageFetcher>(). The nice mock (unlike the
::testing::StrictMock<>) allows unexpected calls and just defaults to doing
nothing.

[jkrcal, 2016/08/12 12:06:10]

Done.

[Marc Treib, 2016/08/12 12:10:59]

One alternative to make things more explicit: Use a StrictMock, but add
ON_CALL(...).WillBeDefault statements for uninteresting calls. (It's fine to
leave this as-is, just something to consider in general)

+  void SetDataUseServiceName(
+      DataUseServiceName data_use_service_name) override {}
+
+  MOCK_METHOD3(
+      StartOrQueueNetworkRequest,
+      void(const std::string&,
+           const GURL&,
+           base::Callback<void(const std::string&, const gfx::Image&)>));
+};
+
 }  // namespace
 
 class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
@@ -325,15 +345,19 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
     snippets_fetcher->SetPersonalizationForTesting(
         NTPSnippetsFetcher::Personalization::kNonPersonal);
 
+    auto image_fetcher = base::MakeUnique<MockImageFetcher>();
+    image_fetcher_ = image_fetcher.get();
+
     // Add an initial fetch response, as the service tries to fetch when there
     // is nothing in the DB.
     SetUpFetchResponse(GetTestJson({GetSnippet()}));
 
     service_.reset(new NTPSnippetsService(
         &observer_, &category_factory_, pref_service(), nullptr, "fr",
-        &scheduler_, std::move(snippets_fetcher), /*image_fetcher=*/nullptr,
-        /*image_fetcher=*/nullptr, base::MakeUnique<NTPSnippetsDatabase>(
-                                       database_dir_.path(), task_runner),
+        &scheduler_, std::move(snippets_fetcher),
+        std::move(image_fetcher), /*image_decoder=*/nullptr,
+        base::MakeUnique<NTPSnippetsDatabase>(database_dir_.path(),
+                                              task_runner),
         base::MakeUnique<NTPSnippetsStatusService>(fake_signin_manager(),
                                                    pref_service())));
 
@@ -348,11 +372,14 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
     return category_factory_.FromKnownCategory(KnownCategories::ARTICLES);
   }
 
+  MOCK_METHOD2(OnImageFetched, void(const std::string&, const gfx::Image&));

[tschumann, 2016/08/12 09:55:36]

while it might sound tempting, this is not how you should use MOCK_METHODs.
MOCK_METHODs are for mock classes only. If you need a mock function, there are
MockFunction for you ;-).
(see
https://codesearch.chromium.org/chromium/src/testing/gmock/test/gmock-actions...
for an example).

[jkrcal, 2016/08/12 12:06:10]

Whoo, that's fancy! Thanks!

+
  protected:
   const GURL& test_url() { return test_url_; }
   NTPSnippetsService* service() { return service_.get(); }
   MockProviderObserver& observer() { return observer_; }
   MockScheduler& mock_scheduler() { return scheduler_; }
+  MockImageFetcher* image_fetcher() { return image_fetcher_; }
 
   // Provide the json to be returned by the fake fetcher.
   void SetUpFetchResponse(const std::string& json) {
@@ -366,6 +393,13 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
     base::RunLoop().RunUntilIdle();
   }
 
+  static void ReturnEmptyImage(

[tschumann, 2016/08/12 09:55:36]

no need to define this inside the text fixture class. just make it a plain
function in an unnamed namespace).

nit on naming: this is not strictly 'return;'-ing. Rename to ServeEmptyImage()
or the like?

[jkrcal, 2016/08/12 12:06:09]

Done.

+      const std::string& id,
+      base::Callback<void(const std::string&, const gfx::Image&)> callback) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, id, gfx::Image()));
+  }
+
  private:
   base::MessageLoop message_loop_;
   FailingFakeURLFetcherFactory failing_url_fetcher_factory_;
@@ -376,6 +410,7 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
   MockScheduler scheduler_;
   MockProviderObserver observer_;
   CategoryFactory category_factory_;
+  MockImageFetcher* image_fetcher_;
   // Last so that the dependencies are deleted after the service.
   std::unique_ptr<NTPSnippetsService> service_;
 
@@ -886,4 +921,34 @@ TEST_F(NTPSnippetsServiceTest, StatusChanges) {
   EXPECT_FALSE(service()->GetSnippetsForTesting().empty());
 }
 
+TEST_F(NTPSnippetsServiceTest, ImageReturnedWithTheSameId) {
+  LoadFromJSONString(GetTestJson({GetSnippet()}));
+
+  EXPECT_CALL(*image_fetcher(), StartOrQueueNetworkRequest(_, _, _))
+      .Times(1)
+      .WillOnce(testing::WithArgs<0, 2>(Invoke(ReturnEmptyImage)));
+  EXPECT_CALL(*this, OnImageFetched(MakeUniqueID(kSnippetUrl), _)).Times(1);
+
+  service()->FetchSuggestionImage(
+      MakeUniqueID(kSnippetUrl),
+      base::Bind(&NTPSnippetsServiceTest::OnImageFetched,
+                 base::Unretained(this)));
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(NTPSnippetsServiceTest, EmptyImageReturnedForNonExistentId) {
+  gfx::Image image;
+  EXPECT_CALL(*this, OnImageFetched(MakeUniqueID(kSnippetUrl2), _))
+      .Times(1)
+      .WillOnce(testing::SaveArg<1>(&image));
+
+  service()->FetchSuggestionImage(
+      MakeUniqueID(kSnippetUrl2),
+      base::Bind(&NTPSnippetsServiceTest::OnImageFetched,
+                 base::Unretained(this)));
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(image.IsEmpty());

[tschumann, 2016/08/12 09:55:36]

hmm... do we have some test image? i guess the default constructed image would
already count as empty, no?

[jkrcal, 2016/08/12 12:06:09]

Well, this is a bit pointless, I agree. This is to document what should happen.
I was not sure how to test the non-trivial case in the other test above. 

Even if I had a test image, gfx::Image does not support "==" operator, so the
only thing I can anyway do is to check that the thing I serve and the thing I
become have the same address. Does it make sense?

[tschumann, 2016/08/12 12:37:09]

src/ui/gfx/image/image_unittest_util.h has some helpers that allow you to create
simple images (e.g. just with some dimension). In this test you could initialize
it with something like that. In the previous test, you could change
ServeEmptyImage()  to serve a provided image and then compare the dimensions.

+}
+
 }  // namespace ntp_snippets