NTPSnippetsService: Garbage collect orphaned images at startup.

components/ntp_snippets/remote/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/remote/ntp_snippets_service.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 208 matching lines @@
   // fail to parse as snippets.
   size_t pos = json_str.find("\"fullPageUrl\"");
   if (pos == std::string::npos) {
     NOTREACHED();
     return std::string();
   }
   json_str[pos + 1] = 'x';
   return json_str;
 }
 
+using ServeImageCallback = base::Callback<void(
+    const std::string&,
+    base::Callback<void(const std::string&, const gfx::Image&)>)>;
+
 void ServeOneByOneImage(
+    image_fetcher::ImageFetcherDelegate* notify,
     const std::string& id,
     base::Callback<void(const std::string&, const gfx::Image&)> callback) {

[Marc Treib, 2016/10/06 10:21:30]

Add a using declaration for this callback type? ImageFetchedCallback?

[tschumann, 2016/10/06 10:56:11]

not sure. IMO, this should be exposed by the ImageFetcher API -- not a big fan
of introducing aliases for other libraries.
Also, it's not a big burden in test code (once written ;-), and we also have it
in production code.

[Marc Treib, 2016/10/06 11:37:20]

Ah indeed, good point! If you want to do this, go ahead, I'm an owner of
components/image_fetcher too :)  but otherwise this is fine as-is.

[tschumann, 2016/10/06 12:21:44]

Added it to my TODO() list.

   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(callback, id, gfx::test::CreateImage(1, 1)));
+  notify->OnImageDataFetched(id, "1-by-1-image-data");
+}
+
+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));
@@ skipping 88 matching lines @@
 
  private:
   base::WaitableEvent loaded_;
   std::map<Category, CategoryStatus, Category::CompareByID> statuses_;
   std::map<Category, std::vector<ContentSuggestion>, Category::CompareByID>
       suggestions_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeContentSuggestionsProviderObserver);
 };
 
+class FakeImageDecoder : public image_fetcher::ImageDecoder {
+ public:
+  FakeImageDecoder() {}
+  ~FakeImageDecoder() override {}

[Marc Treib, 2016/10/06 10:21:30]

nit: = default

[tschumann, 2016/10/06 10:56:11]

Done.

+  void DecodeImage(
+      const std::string& image_data,
+      const image_fetcher::ImageDecodedCallback& callback) override {
+    callback.Run(decoded_image_);
+  }
+
+  void SetDecodedImage(const gfx::Image& image) { decoded_image_ = image; }
+
+ private:
+  gfx::Image decoded_image_;
+};
+
 }  // namespace
 
 class NTPSnippetsServiceTest : public ::testing::Test {
  public:
   NTPSnippetsServiceTest()
       : params_manager_(ntp_snippets::kStudyName,
                         {{"content_suggestions_backend",
                           kTestContentSuggestionsServerEndpoint}}),
         fake_url_fetcher_factory_(
             /*default_factory=*/&failing_url_fetcher_factory_),
         test_url_(kTestContentSuggestionsServerWithAPIKey),
         user_classifier_(/*pref_service=*/nullptr),
-        image_fetcher_(nullptr) {
+        image_fetcher_(nullptr),
+        image_decoder_(nullptr) {
     NTPSnippetsService::RegisterProfilePrefs(utils_.pref_service()->registry());
     RequestThrottler::RegisterProfilePrefs(utils_.pref_service()->registry());
 
     // Since no SuggestionsService is injected in tests, we need to force the
     // service to fetch from all hosts.
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
         switches::kDontRestrict);
     EXPECT_TRUE(database_dir_.CreateUniqueTempDir());
   }
 
@@ skipping 23 matching lines @@
         base::MakeUnique<NTPSnippetsFetcher>(
             utils_.fake_signin_manager(), fake_token_service_.get(),
             std::move(request_context_getter), utils_.pref_service(),
             &category_factory_, base::Bind(&ParseJson), kAPIKey);
 
     utils_.fake_signin_manager()->SignIn("foo@bar.com");
     snippets_fetcher->SetPersonalizationForTesting(
         NTPSnippetsFetcher::Personalization::kNonPersonal);
 
     auto image_fetcher = base::MakeUnique<NiceMock<MockImageFetcher>>();
+
     image_fetcher_ = image_fetcher.get();
+    EXPECT_CALL(*image_fetcher, SetImageFetcherDelegate(_));
+    auto image_decoder = base::MakeUnique<FakeImageDecoder>();
+    image_decoder_ = image_decoder.get();
     EXPECT_FALSE(observer_);
     observer_ = base::MakeUnique<FakeContentSuggestionsProviderObserver>();
     return base::MakeUnique<NTPSnippetsService>(
         observer_.get(), &category_factory_, utils_.pref_service(), nullptr,
         "fr", &user_classifier_, &scheduler_, std::move(snippets_fetcher),
-        std::move(image_fetcher), /*image_decoder=*/nullptr,
+        std::move(image_fetcher), std::move(image_decoder),
         base::MakeUnique<NTPSnippetsDatabase>(database_dir_.GetPath(),
                                               task_runner),
         base::MakeUnique<NTPSnippetsStatusService>(utils_.fake_signin_manager(),
                                                    utils_.pref_service()));
   }
 
   void WaitForSnippetsServiceInitialization(bool set_empty_response = true) {
     EXPECT_TRUE(observer_);
     EXPECT_FALSE(observer_->Loaded());
 
@@ skipping 24 matching lines @@
     return ContentSuggestion::ID(other_category(), id_within_category);
   }
 
   Category other_category() { return category_factory_.FromRemoteCategory(2); }
 
  protected:
   const GURL& test_url() { return test_url_; }
   FakeContentSuggestionsProviderObserver& observer() { return *observer_; }
   MockScheduler& mock_scheduler() { return scheduler_; }
   NiceMock<MockImageFetcher>* image_fetcher() { return image_fetcher_; }
+  FakeImageDecoder* image_decoder() { return image_decoder_; }
 
   // 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(NTPSnippetsService* service,
                           const std::string& json) {
     SetUpFetchResponse(json);
     service->FetchSnippets(true);
     base::RunLoop().RunUntilIdle();
   }
 
  private:
   variations::testing::VariationParamsManager params_manager_;
   test::NTPSnippetsTestUtils utils_;
   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_;
   UserClassifier user_classifier_;
   NiceMock<MockScheduler> scheduler_;
   std::unique_ptr<FakeContentSuggestionsProviderObserver> observer_;
   CategoryFactory category_factory_;
   NiceMock<MockImageFetcher>* image_fetcher_;
+  FakeImageDecoder* image_decoder_;
 
   base::ScopedTempDir database_dir_;
 
   DISALLOW_COPY_AND_ASSIGN(NTPSnippetsServiceTest);
 };
 
 TEST_F(NTPSnippetsServiceTest, ScheduleOnStart) {
   // We should get two |Schedule| calls: The first when initialization
   // completes, the second one after the automatic (since the service doesn't
   // have any data yet) fetch finishes.
@@ skipping 531 matching lines @@
   EXPECT_FALSE(service->GetSnippetsForTesting(articles_category()).empty());
 }
 
 TEST_F(NTPSnippetsServiceTest, ImageReturnedWithTheSameId) {
   auto service = MakeSnippetsService();
 
   LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
 
   gfx::Image image;
   MockFunction<void(const gfx::Image&)> image_fetched;
+  ServeImageCallback cb = base::Bind(&ServeOneByOneImage, service.get());
   {
     InSequence s;
     EXPECT_CALL(*image_fetcher(), StartOrQueueNetworkRequest(_, _, _))
-        .WillOnce(WithArgs<0, 2>(Invoke(ServeOneByOneImage)));
+        .WillOnce(WithArgs<0, 2>(Invoke(&cb, &ServeImageCallback::Run)));
     EXPECT_CALL(image_fetched, Call(_)).WillOnce(SaveArg<0>(&image));
   }
 
   service->FetchSuggestionImage(
       MakeArticleID(kSnippetUrl),
       base::Bind(&MockFunction<void(const gfx::Image&)>::Call,
                  base::Unretained(&image_fetched)));
   base::RunLoop().RunUntilIdle();
   // Check that the image by ServeOneByOneImage is really served.
   EXPECT_EQ(1, image.Width());
@@ skipping 53 matching lines @@
   EXPECT_THAT(service->GetSnippetsForTesting(articles_category()), SizeIs(1));
 
   // Sign in to check a transition from signed out to signed in.
   SetUpFetchResponse(GetTestJson({GetSnippetN(1), GetSnippetN(2)}));
   service->OnSnippetsStatusChanged(SnippetsStatus::ENABLED_AND_SIGNED_OUT,
                                    SnippetsStatus::ENABLED_AND_SIGNED_IN);
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(service->GetSnippetsForTesting(articles_category()), SizeIs(2));
 }
 
+namespace {
+
+gfx::Image FetchImage(NTPSnippetsService* service,
+                      const ContentSuggestion::ID& suggestion_id) {
+  gfx::Image result;
+  base::RunLoop run_loop;
+  service->FetchSuggestionImage(suggestion_id,
+                                base::Bind(
+                                    [](base::Closure signal, gfx::Image* output,
+                                       const gfx::Image& loaded) {
+                                      *output = loaded;
+                                      signal.Run();
+                                      LOG(INFO) << "signalling loop out";

[Marc Treib, 2016/10/06 10:21:30]

nit: remove the LOG

[tschumann, 2016/10/06 10:56:11]

Done.

+                                    },
+                                    run_loop.QuitClosure(), &result));
+  run_loop.Run();
+  return result;
+}
+
+}  // namespace
+
+TEST_F(NTPSnippetsServiceTest, ShouldClearOrphanedImages) {

[Marc Treib, 2016/10/06 10:21:30]

nit: ShouldClearOrphanedImagesOnRestart?

[tschumann, 2016/10/06 10:56:11]

Done.

+  auto service = MakeSnippetsService();
+
+  LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
+  ServeImageCallback cb = base::Bind(&ServeOneByOneImage, service.get());
+
+  EXPECT_CALL(*image_fetcher(), StartOrQueueNetworkRequest(_, _, _))
+      .WillOnce(WithArgs<0, 2>(Invoke(&cb, &ServeImageCallback::Run)));
+  image_decoder()->SetDecodedImage(gfx::test::CreateImage(1, 1));
+
+  gfx::Image image = FetchImage(service.get(), MakeArticleID(kSnippetUrl));
+  EXPECT_EQ(1, image.Width());
+  EXPECT_FALSE(image.IsEmpty());
+
+  // Send new suggestion which don't include the snippet referencing the image.
+  LoadFromJSONString(service.get(),
+                     GetTestJson({GetSnippetWithUrl(
+                         "http://something.com/pletely/unrelated")}));
+  // The image should still be available until a restart happens.
+  EXPECT_FALSE(FetchImage(service.get(), MakeArticleID(kSnippetUrl)).IsEmpty());
+  ResetSnippetsService(&service);
+  // After the restart, the image should be garbage collected.
+  EXPECT_TRUE(FetchImage(service.get(), MakeArticleID(kSnippetUrl)).IsEmpty());
+}
+
 }  // namespace ntp_snippets