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 174 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:
+  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 {
  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 = 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())));
 
     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);
   }
 
+  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) {
     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();
   }
 
+  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_;
   // 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_;
+  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)
+      .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