Add request throttler to thumbnail fetching for articles on mobile NTP

components/ntp_snippets/ntp_snippets_service_unittest.cc

 // Copyright 2015 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_snippets/ntp_snippets_service.h"
 
 #include <memory>
+#include <utility>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/json/json_reader.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/histogram_tester.h"
 #include "base/threading/thread_task_runner_handle.h"
 #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"
 #include "components/ntp_snippets/ntp_snippets_fetcher.h"
 #include "components/ntp_snippets/ntp_snippets_scheduler.h"
 #include "components/ntp_snippets/ntp_snippets_test_utils.h"
 #include "components/ntp_snippets/switches.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
 #include "components/signin/core/browser/fake_signin_manager.h"
 #include "google_apis/google_api_keys.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #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;
 using testing::IsEmpty;
 using testing::Mock;
 using testing::Return;
 using testing::SizeIs;
 using testing::StartsWith;
 using testing::_;
 
@@ skipping 10 matching lines @@
     "https://chromereader-pa.googleapis.com/v1/fetch?key=%s";
 
 const char kSnippetUrl[] = "http://localhost/foobar";
 const char kSnippetTitle[] = "Title";
 const char kSnippetText[] = "Snippet";
 const char kSnippetSalientImage[] = "http://localhost/salient_image";
 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));
   return out_time;
 }
 
 base::Time GetDefaultExpirationTime() {
   return base::Time::Now() + base::TimeDelta::FromHours(1);
 }
 
@@ skipping 102 matching lines @@
   // fail to parse as snippets.
   size_t pos = json_str.find("\"url\"");
   if (pos == std::string::npos) {
     NOTREACHED();
     return std::string();
   }
   json_str[pos + 1] = 'x';
   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,
     const ntp_snippets::NTPSnippetsFetcher::ErrorCallback& error_callback) {
   base::JSONReader json_reader;
   std::unique_ptr<base::Value> value = json_reader.ReadToValue(json);
   if (value) {
     success_callback.Run(std::move(value));
   } else {
     error_callback.Run(json_reader.GetErrorMessage());
@@ skipping 52 matching lines @@
     EXPECT_EQ(new_status, CategoryStatus::AVAILABLE_LOADING);
     run_loop_.Quit();
   }
 
   MockProviderObserver* observer_;
   base::RunLoop run_loop_;
 
   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 {
  public:
   NTPSnippetsServiceTest()
       : fake_url_fetcher_factory_(
             /*default_factory=*/&failing_url_fetcher_factory_),
         test_url_(base::StringPrintf(kTestContentSnippetsServerFormat,
                                      google_apis::GetAPIKey().c_str())) {
     NTPSnippetsService::RegisterProfilePrefs(pref_service()->registry());
@@ skipping 42 matching lines @@
         base::MakeUnique<NTPSnippetsFetcher>(
             fake_signin_manager(), fake_token_service_.get(),
             std::move(request_context_getter), pref_service(),
             base::Bind(&ParseJson),
             /*is_stable_channel=*/true);
 
     fake_signin_manager()->SignIn("foo@bar.com");
     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())));
 
     WaitForDBLoad(&observer_, service_.get());
   }
 
   std::string MakeUniqueID(const std::string& within_category_id) {
     return service()->MakeUniqueID(articles_category(), within_category_id);
   }
 
   Category articles_category() {
     return category_factory_.FromKnownCategory(KnownCategories::ARTICLES);
   }
 
  protected:
   const GURL& test_url() { return test_url_; }
   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) {
     fake_url_fetcher_factory_.SetFakeResponse(test_url_, json, net::HTTP_OK,
                                               net::URLRequestStatus::SUCCESS);
   }
 
   void LoadFromJSONString(const std::string& json) {
     SetUpFetchResponse(json);
     service()->FetchSnippets(true);
     base::RunLoop().RunUntilIdle();
   }
 
  private:
   base::MessageLoop message_loop_;
   FailingFakeURLFetcherFactory failing_url_fetcher_factory_;
   // Instantiation of factory automatically sets itself as URLFetcher's factory.
   net::FakeURLFetcherFactory fake_url_fetcher_factory_;
   const GURL test_url_;
   std::unique_ptr<OAuth2TokenService> fake_token_service_;
   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_;
 
   base::ScopedTempDir database_dir_;
 
   DISALLOW_COPY_AND_ASSIGN(NTPSnippetsServiceTest);
 };
 
 TEST_F(NTPSnippetsServiceTest, ScheduleOnStart) {
   // SetUp() checks that Schedule is called.
@@ skipping 490 matching lines @@
               OnCategoryStatusChanged(_, _, CategoryStatus::AVAILABLE_LOADING));
   EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1);
   service()->OnDisabledReasonChanged(DisabledReason::NONE);
   EXPECT_CALL(observer(),
               OnCategoryStatusChanged(_, _, CategoryStatus::AVAILABLE));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(NTPSnippetsService::State::READY, service()->state_);
   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