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..9bdcea0c8382e71c0503121d60604bf71db16b3e 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));
@@ -191,6 +198,13 @@ std::string GetIncompleteSnippet() {
   return json_str;
 }
 
+void ServeEmptyImage(
+    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()));
+}
+
 void ParseJson(
     const std::string& json,
     const ntp_snippets::NTPSnippetsFetcher::SuccessCallback& success_callback,
@@ -263,6 +277,17 @@ class WaitForDBLoad {
   DISALLOW_COPY_AND_ASSIGN(WaitForDBLoad);
 };
 
+class MockImageFetcher : public ImageFetcher {
+ public:
+  MOCK_METHOD1(SetImageFetcherDelegate, void(ImageFetcherDelegate*));
+  MOCK_METHOD1(SetDataUseServiceName, void(DataUseServiceName));
+  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 +350,20 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
     snippets_fetcher->SetPersonalizationForTesting(
         NTPSnippetsFetcher::Personalization::kNonPersonal);
 
+    auto image_fetcher =

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

nit: no need to allocate the mock on the heap.
Simply put this in your member fields:
testing::NiceMock<MockImageFetcher> image_fetcher_;

[jkrcal, 2016/08/16 12:31:43]

I am a bit puzzled by this comment. 

I need to wrap it in a unique_ptr and pass it to the constructor of the
NTPSnippetsService. Can I even wrap a member field into a unique_ptr and pass it
by std::move? If I can, is it not against the spirit of unique_ptrs?

[Marc Treib, 2016/08/16 13:29:18]

I think Tim just didn't notice that you have to pass a unique_ptr :)
Passing a unique_ptr to something on the stack certainly isn't legal. (It would
get deleted twice)

Keeping a raw pointer around while passing ownership to the service also isn't
particularly clean, but that's a fairly common practice in tests and IMO
acceptable. Don't do that in real code though :)

[tschumann, 2016/08/16 14:56:55]

yes to both. Actually I realized this later and *thought* I removed that
comment... apparently not ;-) Sorry for the confusion.

+        base::MakeUnique<testing::NiceMock<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())));
 
@@ -353,6 +383,9 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
   NTPSnippetsService* service() { return service_.get(); }
   MockProviderObserver& observer() { return observer_; }
   MockScheduler& mock_scheduler() { return scheduler_; }
+  testing::NiceMock<MockImageFetcher>* image_fetcher() {
+    return image_fetcher_;
+  }
 
   // Provide the json to be returned by the fake fetcher.
   void SetUpFetchResponse(const std::string& json) {
@@ -376,6 +409,7 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase {
   MockScheduler scheduler_;
   MockProviderObserver observer_;
   CategoryFactory category_factory_;
+  testing::NiceMock<MockImageFetcher>* image_fetcher_;
   // Last so that the dependencies are deleted after the service.
   std::unique_ptr<NTPSnippetsService> service_;
 
@@ -886,4 +920,43 @@ TEST_F(NTPSnippetsServiceTest, StatusChanges) {
   EXPECT_FALSE(service()->GetSnippetsForTesting().empty());
 }
 
+TEST_F(NTPSnippetsServiceTest, ImageReturnedWithTheSameId) {
+  LoadFromJSONString(GetTestJson({GetSnippet()}));
+
+  EXPECT_CALL(*image_fetcher(), StartOrQueueNetworkRequest(_, _, _))
+      .Times(1)

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

Times(1) is implicit, you can remove that line.

Same in the other calls below.

[jkrcal, 2016/08/16 12:31:43]

Done.

+      .WillOnce(testing::WithArgs<0, 2>(Invoke(ServeEmptyImage)));
+  testing::MockFunction<void(const std::string&, const gfx::Image&)>
+      on_image_fetched_mock_function;

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

nit: feel free to pick shorter names (the test is short, no need to include the
type in the name), e.g. 'image_fetched'.

[jkrcal, 2016/08/16 12:31:43]

Done.

+  EXPECT_CALL(on_image_fetched_mock_function,
+              Call(MakeUniqueID(kSnippetUrl), _))
+      .Times(1);
+
+  service()->FetchSuggestionImage(
+      MakeUniqueID(kSnippetUrl),
+      base::Bind(&testing::MockFunction<void(const std::string&,
+                                              const gfx::Image&)>::Call,
+                 base::Unretained(&on_image_fetched_mock_function)));
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(NTPSnippetsServiceTest, EmptyImageReturnedForNonExistentId) {
+  gfx::Image image;
+  testing::MockFunction<void(const std::string&, const gfx::Image&)>
+      on_image_fetched_mock_function;
+  EXPECT_CALL(on_image_fetched_mock_function,
+              Call(MakeUniqueID(kSnippetUrl2), _))
+      .Times(1)
+      .WillOnce(testing::SaveArg<1>(&image));
+
+  service()->FetchSuggestionImage(
+      MakeUniqueID(kSnippetUrl2),
+      base::Bind(&testing::MockFunction<void(const std::string&,
+                                              const gfx::Image&)>::Call,
+                 base::Unretained(&on_image_fetched_mock_function)));
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(image.IsEmpty());
+}
+
 }  // namespace ntp_snippets