Revert "Adding network fetcher for fetching images."

This reverts commit 7be16f97.

Reason for revert: Lack of policy::cookies_allowed, policy::cookies_store in NetworkTrafficAnnotation.
Bug: 1115855

Original change's description:
> Adding network fetcher for fetching images.
> 
> Change-Id: If4d19d6ba2367f7770c4e77c8f885767171bebe3
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2350148
> Commit-Queue: Jonathan Freed <freedjm@chromium.org>
> Reviewed-by: Dan H <harringtond@chromium.org>
> Reviewed-by: Ian Wells <iwells@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#797494}

TBR=freedjm@chromium.org,iwells@chromium.org,harringtond@chromium.org

Change-Id: I3fee310723f20f394e177d8a1b5d08c27687991e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2353971Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797584}

components/feed/core/v2/BUILD.gn

@@ -23,8 +23,6 @@ source_set("feed_core_v2") {
     "feed_store.h",
     "feed_stream.cc",
     "feed_stream.h",
-    "image_fetcher.cc",
-    "image_fetcher.h",
     "metrics_reporter.cc",
     "metrics_reporter.h",
     "offline_page_spy.cc",
@@ -104,7 +102,6 @@ source_set("core_unit_tests") {
     "feed_network_impl_unittest.cc",
     "feed_store_unittest.cc",
     "feed_stream_unittest.cc",
-    "image_fetcher_unittest.cc",
     "metrics_reporter_unittest.cc",
     "proto_util_unittest.cc",
     "protocol_translator_unittest.cc",

components/feed/core/v2/feed_stream.cc

@@ -22,7 +22,6 @@
 #include "components/feed/core/v2/enums.h"
 #include "components/feed/core/v2/feed_network.h"
 #include "components/feed/core/v2/feed_store.h"
-#include "components/feed/core/v2/image_fetcher.h"
 #include "components/feed/core/v2/metrics_reporter.h"
 #include "components/feed/core/v2/offline_page_spy.h"
 #include "components/feed/core/v2/prefs.h"
@@ -132,7 +131,6 @@ FeedStream::FeedStream(RefreshTaskScheduler* refresh_task_scheduler,
                        Delegate* delegate,
                        PrefService* profile_prefs,
                        FeedNetwork* feed_network,
-                       ImageFetcher* image_fetcher,
                        FeedStore* feed_store,
                        offline_pages::PrefetchService* prefetch_service,
                        offline_pages::OfflinePageModel* offline_page_model,
@@ -145,7 +143,6 @@ FeedStream::FeedStream(RefreshTaskScheduler* refresh_task_scheduler,
       delegate_(delegate),
       profile_prefs_(profile_prefs),
       feed_network_(feed_network),
-      image_fetcher_(image_fetcher),
       store_(feed_store),
       clock_(clock),
       tick_clock_(tick_clock),
@@ -599,12 +596,6 @@ void FeedStream::FinishClearAll() {
   delegate_->ClearAll();
 }
 
-void FeedStream::FetchImage(
-    const GURL& url,
-    base::OnceCallback<void(std::unique_ptr<std::string>)> callback) {
-  image_fetcher_->Fetch(url, std::move(callback));
-}
-
 void FeedStream::UploadAction(
     feedwire::FeedAction action,
     bool upload_now,

components/feed/core/v2/feed_stream.h

@@ -43,7 +43,6 @@ class PrefetchService;
 namespace feed {
 class FeedNetwork;
 class FeedStore;
-class ImageFetcher;
 class MetricsReporter;
 class OfflinePageSpy;
 class RefreshTaskScheduler;
@@ -103,7 +102,6 @@ class FeedStream : public FeedStreamApi,
              Delegate* delegate,
              PrefService* profile_prefs,
              FeedNetwork* feed_network,
-             ImageFetcher* image_fetcher,
              FeedStore* feed_store,
              offline_pages::PrefetchService* prefetch_service,
              offline_pages::OfflinePageModel* offline_page_model,
@@ -126,9 +124,6 @@ class FeedStream : public FeedStreamApi,
   bool IsArticlesListVisible() override;
   std::string GetClientInstanceId() override;
   void ExecuteRefreshTask() override;
-  void FetchImage(
-      const GURL& url,
-      base::OnceCallback<void(std::unique_ptr<std::string>)> callback) override;
   void LoadMore(SurfaceId surface_id,
                 base::OnceCallback<void(bool)> callback) override;
   void ExecuteOperations(
@@ -296,7 +291,6 @@ class FeedStream : public FeedStreamApi,
   Delegate* delegate_;
   PrefService* profile_prefs_;  // May be null.
   FeedNetwork* feed_network_;
-  ImageFetcher* image_fetcher_;
   FeedStore* store_;
   const base::Clock* clock_;
   const base::TickClock* tick_clock_;

components/feed/core/v2/feed_stream_unittest.cc

@@ -34,7 +34,6 @@
 #include "components/feed/core/shared_prefs/pref_names.h"
 #include "components/feed/core/v2/config.h"
 #include "components/feed/core/v2/feed_network.h"
-#include "components/feed/core/v2/image_fetcher.h"
 #include "components/feed/core/v2/metrics_reporter.h"
 #include "components/feed/core/v2/protocol_translator.h"
 #include "components/feed/core/v2/refresh_task_scheduler.h"
@@ -51,9 +50,6 @@
 #include "components/offline_pages/core/stub_offline_page_model.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/testing_pref_service.h"
-#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
-#include "services/network/test/test_url_loader_factory.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace feed {
@@ -258,20 +254,6 @@ class TestSurface : public FeedStream::SurfaceInterface {
   std::map<std::string, std::string> data_store_entries_;
 };
 
-class TestImageFetcher : public ImageFetcher {
- public:
-  explicit TestImageFetcher(
-      scoped_refptr<::network::SharedURLLoaderFactory> url_loader_factory)
-      : ImageFetcher(url_loader_factory) {}
-  void Fetch(const GURL& url,
-             base::OnceCallback<void(std::unique_ptr<std::string>)> callback)
-      override {
-    // Emulate a response.
-    auto response = std::make_unique<std::string>("dummyresponse");
-    std::move(callback).Run(std::move(response));
-  }
-};
-
 class TestFeedNetwork : public FeedNetwork {
  public:
   // FeedNetwork implementation.
@@ -544,12 +526,6 @@ class FeedStreamTest : public testing::Test, public FeedStream::Delegate {
     metrics_reporter_ = std::make_unique<TestMetricsReporter>(
         task_environment_.GetMockTickClock(), &profile_prefs_);
 
-    shared_url_loader_factory_ =
-        base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
-            &test_factory_);
-    image_fetcher_ =
-        std::make_unique<TestImageFetcher>(shared_url_loader_factory_);
-
     CHECK_EQ(kTestTimeEpoch, task_environment_.GetMockClock()->Now());
     CreateStream();
   }
@@ -585,9 +561,9 @@ class FeedStreamTest : public testing::Test, public FeedStream::Delegate {
     chrome_info.version = base::Version({99, 1, 9911, 2});
     stream_ = std::make_unique<FeedStream>(
         &refresh_scheduler_, metrics_reporter_.get(), this, &profile_prefs_,
-        &network_, image_fetcher_.get(), store_.get(), &prefetch_service_,
-        &offline_page_model_, task_environment_.GetMockClock(),
-        task_environment_.GetMockTickClock(), chrome_info);
+        &network_, store_.get(), &prefetch_service_, &offline_page_model_,
+        task_environment_.GetMockClock(), task_environment_.GetMockTickClock(),
+        chrome_info);
 
     WaitForIdleTaskQueue();  // Wait for any initialization.
     stream_->SetWireResponseTranslatorForTesting(&response_translator_);
@@ -648,9 +624,6 @@ class FeedStreamTest : public testing::Test, public FeedStream::Delegate {
   std::unique_ptr<TestMetricsReporter> metrics_reporter_;
   TestFeedNetwork network_;
   TestWireResponseTranslator response_translator_;
-  std::unique_ptr<TestImageFetcher> image_fetcher_;
-  network::TestURLLoaderFactory test_factory_;
-  scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
 
   std::unique_ptr<FeedStore> store_ = std::make_unique<FeedStore>(
       leveldb_proto::ProtoDatabaseProvider::GetUniqueDB<feedstore::Record>(
@@ -877,13 +850,6 @@ TEST_F(FeedStreamTest, DetachSurface) {
   EXPECT_FALSE(surface.update);
 }
 
-TEST_F(FeedStreamTest, FetchImage) {
-  CallbackReceiver<std::unique_ptr<std::string>> receiver;
-  stream_->FetchImage(GURL("https://example.com"), receiver.Bind());
-
-  EXPECT_EQ("dummyresponse", **receiver.GetResult());
-}
-
 TEST_F(FeedStreamTest, LoadFromNetwork) {
   stream_->GetMetadata()->SetConsistencyToken("token");
 

components/feed/core/v2/image_fetcher.cc

-// Copyright 2020 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/feed/core/v2/image_fetcher.h"
-
-#include "net/http/http_request_headers.h"
-#include "services/network/public/cpp/shared_url_loader_factory.h"
-#include "services/network/public/cpp/simple_url_loader.h"
-
-namespace feed {
-
-ImageFetcher::ImageFetcher(
-    scoped_refptr<::network::SharedURLLoaderFactory> url_loader_factory)
-    : url_loader_factory_(url_loader_factory) {}
-
-ImageFetcher::~ImageFetcher() = default;
-
-void ImageFetcher::Fetch(const GURL& url, ImageCallback callback) {
-  net::NetworkTrafficAnnotationTag traffic_annotation =
-      net::DefineNetworkTrafficAnnotation("interest_feedv2_image_send", R"(
-        semantics {
-          sender: "Feed Library"
-          description: "Images for articles in the feed."
-          trigger: "Triggered when viewing the feed on the NTP."
-          data: "Request for an image associated with a feed article."
-          destination: GOOGLE_OWNED_SERVICE
-        }
-        policy {
-          setting: "This can be disabled from the New Tab Page by collapsing "
-          "the articles section."
-          chrome_policy {
-            NTPContentSuggestionsEnabled {
-              policy_options {mode: MANDATORY}
-              NTPContentSuggestionsEnabled: false
-            }
-          }
-        })");
-  auto resource_request = std::make_unique<::network::ResourceRequest>();
-  resource_request->url = url;
-  resource_request->method = net::HttpRequestHeaders::kGetMethod;
-  resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
-
-  auto simple_loader = network::SimpleURLLoader::Create(
-      std::move(resource_request), traffic_annotation);
-  simple_loader->DownloadToString(
-      url_loader_factory_.get(),
-      base::BindOnce(&ImageFetcher::OnFetchComplete, weak_factory_.GetWeakPtr(),
-                     std::move(simple_loader), std::move(callback)),
-      network::SimpleURLLoader::kMaxBoundedStringDownloadSize);
-}
-
-void ImageFetcher::OnFetchComplete(
-    std::unique_ptr<network::SimpleURLLoader> simple_loader,
-    ImageCallback callback,
-    std::unique_ptr<std::string> response_data) {
-  std::move(callback).Run(std::move(response_data));
-}
-
-}  // namespace feed
\ No newline at end of file

components/feed/core/v2/image_fetcher.h

-// Copyright 2020 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.
-
-#ifndef COMPONENTS_FEED_CORE_V2_IMAGE_FETCHER_H_
-#define COMPONENTS_FEED_CORE_V2_IMAGE_FETCHER_H_
-
-#include "base/callback.h"
-#include "base/memory/scoped_refptr.h"
-#include "base/memory/weak_ptr.h"
-#include "url/gurl.h"
-
-namespace network {
-class SharedURLLoaderFactory;
-class SimpleURLLoader;
-}  // namespace network
-
-namespace feed {
-
-// Fetcher object to retrieve an image resource from a URL.
-class ImageFetcher {
- public:
-  using ImageCallback = base::OnceCallback<void(std::unique_ptr<std::string>)>;
-  explicit ImageFetcher(
-      scoped_refptr<::network::SharedURLLoaderFactory> url_loader_factory);
-  virtual ~ImageFetcher();
-  ImageFetcher(const ImageFetcher&) = delete;
-  ImageFetcher& operator=(const ImageFetcher&) = delete;
-
-  virtual void Fetch(const GURL& url, ImageCallback callback);
-
- private:
-  // Called when fetch request completes.
-  void OnFetchComplete(std::unique_ptr<network::SimpleURLLoader> simple_loader,
-                       ImageCallback callback,
-                       std::unique_ptr<std::string> response_data);
-
-  const scoped_refptr<::network::SharedURLLoaderFactory> url_loader_factory_;
-  base::WeakPtrFactory<ImageFetcher> weak_factory_{this};
-};
-
-}  // namespace feed
-
-#endif  // COMPONENTS_FEED_CORE_V2_IMAGE_FETCHER_H_
\ No newline at end of file

components/feed/core/v2/image_fetcher_unittest.cc

-// Copyright 2020 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/feed/core/v2/image_fetcher.h"
-
-#include <memory>
-#include <utility>
-
-#include "base/bind.h"
-#include "base/strings/string_number_conversions.h"
-#include "base/strings/string_split.h"
-#include "base/test/bind_test_util.h"
-#include "base/test/task_environment.h"
-#include "components/feed/core/v2/test/callback_receiver.h"
-#include "net/http/http_response_headers.h"
-#include "net/http/http_status_code.h"
-#include "net/http/http_util.h"
-#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
-#include "services/network/public/cpp/url_loader_completion_status.h"
-#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
-#include "services/network/test/test_url_loader_factory.h"
-#include "testing/gmock/include/gmock/gmock.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "third_party/protobuf/src/google/protobuf/io/coded_stream.h"
-#include "third_party/protobuf/src/google/protobuf/io/zero_copy_stream_impl_lite.h"
-#include "url/gurl.h"
-
-using ::testing::HasSubstr;
-
-namespace feed {
-namespace {
-
-class ImageFetcherTest : public testing::Test {
- public:
-  ImageFetcherTest() = default;
-  ImageFetcherTest(ImageFetcherTest&) = delete;
-  ImageFetcherTest& operator=(const ImageFetcherTest&) = delete;
-  ~ImageFetcherTest() override = default;
-
-  void SetUp() override {
-    shared_url_loader_factory_ =
-        base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
-            &test_factory_);
-    image_fetcher_ = std::make_unique<ImageFetcher>(shared_url_loader_factory_);
-  }
-
-  ImageFetcher* image_fetcher() { return image_fetcher_.get(); }
-
-  void Respond(const GURL& url,
-               const std::string& response_string,
-               net::HttpStatusCode code = net::HTTP_OK,
-               network::URLLoaderCompletionStatus status =
-                   network::URLLoaderCompletionStatus()) {
-    auto head = network::mojom::URLResponseHead::New();
-    if (code >= 0) {
-      head->headers = base::MakeRefCounted<net::HttpResponseHeaders>(
-          "HTTP/1.1 " + base::NumberToString(code));
-      status.decoded_body_length = response_string.length();
-    }
-
-    test_factory_.AddResponse(url, std::move(head), response_string, status);
-  }
-
-  network::ResourceRequest RespondToRequest(const std::string& response_string,
-                                            net::HttpStatusCode code) {
-    task_environment_.RunUntilIdle();
-    network::TestURLLoaderFactory::PendingRequest* pending_request =
-        test_factory_.GetPendingRequest(0);
-    CHECK(pending_request);
-    network::ResourceRequest resource_request = pending_request->request;
-    Respond(pending_request->request.url, response_string, code);
-    task_environment_.FastForwardUntilNoTasksRemain();
-    return resource_request;
-  }
-
- private:
-  std::unique_ptr<ImageFetcher> image_fetcher_;
-  network::TestURLLoaderFactory test_factory_;
-  scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
-  base::test::TaskEnvironment task_environment_{
-      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
-};
-
-TEST_F(ImageFetcherTest, SendRequestSendsValidRequest) {
-  CallbackReceiver<std::unique_ptr<std::string>> receiver;
-  image_fetcher()->Fetch(GURL("https://example.com"), receiver.Bind());
-  network::ResourceRequest resource_request =
-      RespondToRequest("", net::HTTP_OK);
-
-  EXPECT_EQ(GURL("https://example.com"), resource_request.url);
-  EXPECT_EQ("GET", resource_request.method);
-}
-
-TEST_F(ImageFetcherTest, SendRequestValidResponse) {
-  CallbackReceiver<std::unique_ptr<std::string>> receiver;
-  image_fetcher()->Fetch(GURL("https://example.com"), receiver.Bind());
-  RespondToRequest("example_response", net::HTTP_OK);
-
-  ASSERT_TRUE(receiver.GetResult());
-  EXPECT_THAT(**receiver.GetResult(), HasSubstr("example_response"));
-}
-
-TEST_F(ImageFetcherTest, SendSequentialRequestsValidResponses) {
-  CallbackReceiver<std::unique_ptr<std::string>> receiver1;
-  image_fetcher()->Fetch(GURL("https://example1.com"), receiver1.Bind());
-  RespondToRequest("example1_response", net::HTTP_OK);
-
-  CallbackReceiver<std::unique_ptr<std::string>> receiver2;
-  image_fetcher()->Fetch(GURL("https://example2.com"), receiver2.Bind());
-  RespondToRequest("example2_response", net::HTTP_OK);
-
-  ASSERT_TRUE(receiver1.GetResult());
-  EXPECT_THAT(**receiver1.GetResult(), HasSubstr("example1_response"));
-  ASSERT_TRUE(receiver2.GetResult());
-  EXPECT_THAT(**receiver2.GetResult(), HasSubstr("example2_response"));
-}
-
-TEST_F(ImageFetcherTest, SendParallelRequestsValidResponses) {
-  CallbackReceiver<std::unique_ptr<std::string>> receiver1;
-  image_fetcher()->Fetch(GURL("https://example1.com"), receiver1.Bind());
-  CallbackReceiver<std::unique_ptr<std::string>> receiver2;
-  image_fetcher()->Fetch(GURL("https://example2.com"), receiver2.Bind());
-
-  RespondToRequest("example1_response", net::HTTP_OK);
-  RespondToRequest("example2_response", net::HTTP_OK);
-
-  ASSERT_TRUE(receiver1.GetResult());
-  EXPECT_THAT(**receiver1.GetResult(), HasSubstr("example1_response"));
-  ASSERT_TRUE(receiver2.GetResult());
-  EXPECT_THAT(**receiver2.GetResult(), HasSubstr("example2_response"));
-}
-
-}  // namespace
-}  // namespace feed

components/feed/core/v2/public/feed_service.cc

@@ -13,7 +13,6 @@
 #include "components/feed/core/v2/feed_network_impl.h"
 #include "components/feed/core/v2/feed_store.h"
 #include "components/feed/core/v2/feed_stream.h"
-#include "components/feed/core/v2/image_fetcher.h"
 #include "components/feed/core/v2/metrics_reporter.h"
 #include "components/feed/core/v2/refresh_task_scheduler.h"
 #include "components/feed/feed_feature_list.h"
@@ -173,15 +172,13 @@ FeedService::FeedService(
   feed_network_ = std::make_unique<FeedNetworkImpl>(
       network_delegate_.get(), identity_manager, api_key, url_loader_factory,
       base::DefaultTickClock::GetInstance(), profile_prefs);
-  image_fetcher_ = std::make_unique<ImageFetcher>(url_loader_factory);
   store_ = std::make_unique<FeedStore>(std::move(database));
 
   stream_ = std::make_unique<FeedStream>(
       refresh_task_scheduler_.get(), metrics_reporter_.get(),
-      stream_delegate_.get(), profile_prefs, feed_network_.get(),
-      image_fetcher_.get(), store_.get(), prefetch_service, offline_page_model,
-      base::DefaultClock::GetInstance(), base::DefaultTickClock::GetInstance(),
-      chrome_info);
+      stream_delegate_.get(), profile_prefs, feed_network_.get(), store_.get(),
+      prefetch_service, offline_page_model, base::DefaultClock::GetInstance(),
+      base::DefaultTickClock::GetInstance(), chrome_info);
 
   history_observer_ = std::make_unique<HistoryObserverImpl>(
       history_service, static_cast<FeedStream*>(stream_.get()));

components/feed/core/v2/public/feed_service.h

@@ -48,7 +48,6 @@ class MetricsReporter;
 class FeedNetwork;
 class FeedStore;
 class FeedStream;
-class ImageFetcher;
 
 namespace internal {
 bool ShouldClearFeed(const history::DeletionInfo& deletion_info);
@@ -118,7 +117,6 @@ class FeedService : public KeyedService {
   std::unique_ptr<MetricsReporter> metrics_reporter_;
   std::unique_ptr<NetworkDelegateImpl> network_delegate_;
   std::unique_ptr<FeedNetwork> feed_network_;
-  std::unique_ptr<ImageFetcher> image_fetcher_;
   std::unique_ptr<FeedStore> store_;
   std::unique_ptr<RefreshTaskScheduler> refresh_task_scheduler_;
   std::unique_ptr<HistoryObserverImpl> history_observer_;

components/feed/core/v2/public/feed_stream_api.h

@@ -13,7 +13,6 @@
 #include "base/strings/string_piece_forward.h"
 #include "base/time/time.h"
 #include "components/feed/core/v2/public/types.h"
-#include "url/gurl.h"
 
 namespace feedui {
 class StreamUpdate;
@@ -72,12 +71,6 @@ class FeedStreamApi {
   virtual void LoadMore(SurfaceId surface,
                         base::OnceCallback<void(bool)> callback) = 0;
 
-  // Request to fetch and image for use in the feed. Calls |callback|
-  // with the network response when complete.
-  virtual void FetchImage(
-      const GURL& url,
-      base::OnceCallback<void(std::unique_ptr<std::string>)> callback) = 0;
-
   // Apply |operations| to the stream model. Does nothing if the model is not
   // yet loaded.
   virtual void ExecuteOperations(