[Media Feeds] Move conversion into fetcher

As part of this bug the fetcher will need to
access the parsed data from the feed. This
moves the conversion into the fetcher.

BUG=1084303

Change-Id: I4a06dd9d0f46ceccc7df35b6376a16bb5548b227
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2208084
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771104}

chrome/browser/media/feeds/media_feeds_converter.cc

@@ -1343,7 +1343,7 @@ bool MediaFeedsConverter::ConvertMediaFeedImpl(
   }
 
   auto* provider = GetProperty(entity.get(), schema_org::property::kProvider);
-  if (!ValidateProvider(*provider, result)) {
+  if (!provider || !ValidateProvider(*provider, result)) {
     Log("Invalid provider.");
     return false;
   }

chrome/browser/media/feeds/media_feeds_fetcher.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/media/feeds/media_feeds_fetcher.h"
 
 #include "base/metrics/histogram_functions.h"
+#include "chrome/browser/media/feeds/media_feeds_converter.h"
 #include "components/schema_org/common/metadata.mojom.h"
 #include "components/schema_org/extractor.h"
 #include "components/schema_org/schema_org_entity_names.h"
@@ -18,6 +19,35 @@
 
 namespace media_feeds {
 
+namespace {
+
+media_feeds::mojom::FetchResult GetFetchResult(
+    MediaFeedsFetcher::Status status) {
+  switch (status) {
+    case MediaFeedsFetcher::Status::kOk:
+      return media_feeds::mojom::FetchResult::kSuccess;
+    case MediaFeedsFetcher::Status::kInvalidFeedData:
+    case MediaFeedsFetcher::Status::kRequestFailed:
+      return media_feeds::mojom::FetchResult::kFailedBackendError;
+    case MediaFeedsFetcher::Status::kNotFound:
+      return media_feeds::mojom::FetchResult::kFailedNetworkError;
+    default:
+      return media_feeds::mojom::FetchResult::kNone;
+  }
+}
+
+media_history::MediaHistoryKeyedService::MediaFeedFetchResult BuildResult(
+    MediaFeedsFetcher::Status status,
+    bool was_fetched_via_cache) {
+  media_history::MediaHistoryKeyedService::MediaFeedFetchResult result;
+  result.status = GetFetchResult(status);
+  result.was_fetched_from_cache = was_fetched_via_cache;
+  result.gone = status == MediaFeedsFetcher::Status::kGone;
+  return result;
+}
+
+}  // namespace
+
 const char MediaFeedsFetcher::kFetchSizeKbHistogramName[] =
     "Media.Feeds.Fetch.Size";
 
@@ -105,7 +135,8 @@ void MediaFeedsFetcher::OnURLFetchComplete(
   DCHECK(request);
 
   if (request->NetError() != net::OK) {
-    std::move(callback).Run(nullptr, Status::kRequestFailed, false);
+    std::move(callback).Run(
+        BuildResult(Status::kRequestFailed, /*was_fetched_via_cache=*/false));
     return;
   }
 
@@ -120,18 +151,19 @@ void MediaFeedsFetcher::OnURLFetchComplete(
   }
 
   if (response_code == net::HTTP_GONE) {
-    std::move(callback).Run(nullptr, Status::kGone, was_fetched_via_cache);
+    std::move(callback).Run(BuildResult(Status::kGone, was_fetched_via_cache));
     return;
   }
 
   if (response_code != net::HTTP_OK) {
-    std::move(callback).Run(nullptr, Status::kRequestFailed,
-                            was_fetched_via_cache);
+    std::move(callback).Run(
+        BuildResult(Status::kRequestFailed, was_fetched_via_cache));
     return;
   }
 
   if (!feed_data || feed_data->empty()) {
-    std::move(callback).Run(nullptr, Status::kNotFound, was_fetched_via_cache);
+    std::move(callback).Run(
+        BuildResult(Status::kNotFound, was_fetched_via_cache));
     return;
   }
 
@@ -153,13 +185,16 @@ void MediaFeedsFetcher::OnParseComplete(
     bool was_fetched_via_cache,
     schema_org::improved::mojom::EntityPtr parsed_entity) {
   if (!schema_org::ValidateEntity(parsed_entity.get())) {
-    std::move(callback).Run(nullptr, Status::kInvalidFeedData,
-                            was_fetched_via_cache);
+    std::move(callback).Run(
+        BuildResult(Status::kInvalidFeedData, was_fetched_via_cache));
     return;
   }
 
-  std::move(callback).Run(std::move(parsed_entity), Status::kOk,
-                          was_fetched_via_cache);
+  auto result = BuildResult(Status::kOk, was_fetched_via_cache);
+  if (!media_feeds_converter_.ConvertMediaFeed(parsed_entity, &result))
+    result.status = media_feeds::mojom::FetchResult::kInvalidFeed;
+
+  std::move(callback).Run(std::move(result));
 }
 
 }  // namespace media_feeds

chrome/browser/media/feeds/media_feeds_fetcher.h

@@ -6,6 +6,8 @@
 #define CHROME_BROWSER_MEDIA_FEEDS_MEDIA_FEEDS_FETCHER_H_
 
 #include "base/threading/thread_checker.h"
+#include "chrome/browser/media/feeds/media_feeds_converter.h"
+#include "chrome/browser/media/history/media_history_keyed_service.h"
 #include "components/schema_org/common/improved_metadata.mojom.h"
 #include "components/schema_org/extractor.h"
 #include "url/gurl.h"
@@ -30,10 +32,8 @@ class MediaFeedsFetcher {
     kGone,
   };
 
-  using MediaFeedCallback =
-      base::OnceCallback<void(const schema_org::improved::mojom::EntityPtr&,
-                              Status,
-                              /*was_fetched_via_cache=*/bool)>;
+  using MediaFeedCallback = base::OnceCallback<void(
+      media_history::MediaHistoryKeyedService::MediaFeedFetchResult)>;
   explicit MediaFeedsFetcher(
       scoped_refptr<::network::SharedURLLoaderFactory> url_loader_factory);
   ~MediaFeedsFetcher();
@@ -60,6 +60,8 @@ class MediaFeedsFetcher {
   // is pending, and will be reset by |OnURLFetchComplete| or if cancelled.
   std::unique_ptr<::network::SimpleURLLoader> pending_request_;
 
+  MediaFeedsConverter media_feeds_converter_;
+
   schema_org::Extractor extractor_;
 
   base::ThreadChecker thread_checker_;

chrome/browser/media/feeds/media_feeds_fetcher_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/test/bind_test_util.h"
 #include "base/test/mock_callback.h"
 #include "base/test/task_environment.h"
+#include "chrome/browser/media/history/media_history_keyed_service.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "components/schema_org/common/metadata.mojom.h"
 #include "components/schema_org/schema_org_entity_names.h"
@@ -28,6 +29,29 @@ using testing::_;
 
 const char kTestUrl[] = "https://www.google.com";
 
+const char kTestData[] = R"END({
+    "@context": "https://schema.org",
+    "@type": "CompleteDataFeed",
+    "dataFeedElement": [],
+    "provider": {
+      "@type": "Organization",
+      "name": "Media Feeds Developers",
+      "logo": [{
+        "@type": "ImageObject",
+        "width": 1113,
+        "height": 245,
+        "url": "https://wicg.github.io/media-feeds/data/logo_white.png",
+        "additionalProperty": {
+          "@type": "PropertyValue",
+          "name": "contentAttributes",
+          "value": ["forDarkBackground", "hasTitle", "transparentBackground"]
+        }
+      }]
+    }
+})END";
+
+const char kTestFeedName[] = "Media Feeds Developers";
+
 class MediaFeedsFetcherTest : public ChromeRenderViewHostTestHarness {
  public:
   MediaFeedsFetcherTest() = default;
@@ -122,102 +146,66 @@ TEST_F(MediaFeedsFetcherTest, SucceedsOnBasicFetch) {
   GURL site_with_cookies(kTestUrl);
   ASSERT_TRUE(SetCookie(profile(), site_with_cookies, "testing"));
 
-  base::MockCallback<MediaFeedsFetcher::MediaFeedCallback> callback;
-
-  schema_org::improved::mojom::EntityPtr expected =
-      schema_org::improved::mojom::Entity::New();
-  expected->type = schema_org::entity::kCompleteDataFeed;
-  schema_org::improved::mojom::PropertyPtr property =
-      schema_org::improved::mojom::Property::New();
-  property->name = "name";
-  property->values = schema_org::improved::mojom::Values::New();
-  property->values->string_values.push_back("Media Site");
-  expected->properties.push_back(std::move(property));
-
-  schema_org::improved::mojom::EntityPtr out;
-
   fetcher()->FetchFeed(
       GURL("https://www.google.com"), false,
       base::BindLambdaForTesting(
-          [&](const schema_org::improved::mojom::EntityPtr& response,
-              MediaFeedsFetcher::Status status, bool was_fetched_via_cache) {
-            EXPECT_EQ(status, MediaFeedsFetcher::Status::kOk);
-            EXPECT_FALSE(was_fetched_via_cache);
-            out = response.Clone();
+          [&](media_history::MediaHistoryKeyedService::MediaFeedFetchResult
+                  result) {
+            EXPECT_EQ(result.status, mojom::FetchResult::kSuccess);
+            EXPECT_FALSE(result.was_fetched_from_cache);
+            EXPECT_FALSE(result.gone);
+            EXPECT_EQ(kTestFeedName, result.display_name);
           }));
 
   WaitForRequest();
-  ASSERT_TRUE(RespondToFetch(
-      "{\"@type\":\"CompleteDataFeed\",\"name\":\"Media Site\"}"));
-
-  EXPECT_EQ(out, expected);
+  ASSERT_TRUE(RespondToFetch(kTestData));
 }
 
 TEST_F(MediaFeedsFetcherTest, SucceedsOnBasicFetch_ForceCache) {
   GURL site_with_cookies(kTestUrl);
   ASSERT_TRUE(SetCookie(profile(), site_with_cookies, "testing"));
 
-  base::MockCallback<MediaFeedsFetcher::MediaFeedCallback> callback;
-
-  schema_org::improved::mojom::EntityPtr expected =
-      schema_org::improved::mojom::Entity::New();
-  expected->type = schema_org::entity::kCompleteDataFeed;
-  schema_org::improved::mojom::PropertyPtr property =
-      schema_org::improved::mojom::Property::New();
-  property->name = "name";
-  property->values = schema_org::improved::mojom::Values::New();
-  property->values->string_values.push_back("Media Site");
-  expected->properties.push_back(std::move(property));
-
-  schema_org::improved::mojom::EntityPtr out;
-
   fetcher()->FetchFeed(
       GURL("https://www.google.com"), true,
       base::BindLambdaForTesting(
-          [&](const schema_org::improved::mojom::EntityPtr& response,
-              MediaFeedsFetcher::Status status, bool was_fetched_via_cache) {
-            EXPECT_EQ(status, MediaFeedsFetcher::Status::kOk);
-            EXPECT_FALSE(was_fetched_via_cache);
-            out = response.Clone();
+          [&](media_history::MediaHistoryKeyedService::MediaFeedFetchResult
+                  result) {
+            EXPECT_EQ(result.status, mojom::FetchResult::kSuccess);
+            EXPECT_FALSE(result.was_fetched_from_cache);
+            EXPECT_FALSE(result.gone);
+            EXPECT_EQ(kTestFeedName, result.display_name);
           }));
 
   WaitForRequest();
-  ASSERT_TRUE(RespondToFetch(
-      "{\"@type\":\"CompleteDataFeed\",\"name\":\"Media Site\"}"));
-
-  EXPECT_EQ(out, expected);
+  ASSERT_TRUE(RespondToFetch(kTestData));
 }
 
 TEST_F(MediaFeedsFetcherTest, SucceedsFetchFromCache) {
-  base::MockCallback<MediaFeedsFetcher::MediaFeedCallback> callback;
-
   fetcher()->FetchFeed(
       GURL("https://www.google.com"), false,
       base::BindLambdaForTesting(
-          [&](const schema_org::improved::mojom::EntityPtr& response,
-              MediaFeedsFetcher::Status status, bool was_fetched_via_cache) {
-            EXPECT_EQ(status, MediaFeedsFetcher::Status::kOk);
-            EXPECT_TRUE(was_fetched_via_cache);
-            EXPECT_TRUE(response);
+          [&](media_history::MediaHistoryKeyedService::MediaFeedFetchResult
+                  result) {
+            EXPECT_EQ(result.status, mojom::FetchResult::kSuccess);
+            EXPECT_TRUE(result.was_fetched_from_cache);
+            EXPECT_FALSE(result.gone);
+            EXPECT_EQ(kTestFeedName, result.display_name);
           }));
 
   WaitForRequest();
   ASSERT_TRUE(
-      RespondToFetch("{\"@type\":\"CompleteDataFeed\",\"name\":\"Media Site\"}",
-                     net::HttpStatusCode::HTTP_OK, net::OK, true));
+      RespondToFetch(kTestData, net::HttpStatusCode::HTTP_OK, net::OK, true));
 }
 
 TEST_F(MediaFeedsFetcherTest, ReturnsFailedResponseCode) {
-  base::MockCallback<MediaFeedsFetcher::MediaFeedCallback> callback;
-
   fetcher()->FetchFeed(
       GURL("https://www.google.com"), false,
       base::BindLambdaForTesting(
-          [&](const schema_org::improved::mojom::EntityPtr& response,
-              MediaFeedsFetcher::Status status, bool was_fetched_via_cache) {
-            EXPECT_EQ(status, MediaFeedsFetcher::Status::kRequestFailed);
-            EXPECT_FALSE(was_fetched_via_cache);
-            EXPECT_FALSE(response);
+          [&](media_history::MediaHistoryKeyedService::MediaFeedFetchResult
+                  result) {
+            EXPECT_EQ(result.status, mojom::FetchResult::kFailedBackendError);
+            EXPECT_FALSE(result.was_fetched_from_cache);
+            EXPECT_FALSE(result.gone);
           }));
 
   WaitForRequest();
@@ -230,11 +218,11 @@ TEST_F(MediaFeedsFetcherTest, ReturnsGone) {
   fetcher()->FetchFeed(
       GURL("https://www.google.com"), false,
       base::BindLambdaForTesting(
-          [&](const schema_org::improved::mojom::EntityPtr& response,
-              MediaFeedsFetcher::Status status, bool was_fetched_via_cache) {
-            EXPECT_EQ(status, MediaFeedsFetcher::Status::kGone);
-            EXPECT_FALSE(was_fetched_via_cache);
-            EXPECT_FALSE(response);
+          [&](media_history::MediaHistoryKeyedService::MediaFeedFetchResult
+                  result) {
+            EXPECT_EQ(result.status, mojom::FetchResult::kNone);
+            EXPECT_FALSE(result.was_fetched_from_cache);
+            EXPECT_TRUE(result.gone);
           }));
 
   WaitForRequest();
@@ -242,16 +230,14 @@ TEST_F(MediaFeedsFetcherTest, ReturnsGone) {
 }
 
 TEST_F(MediaFeedsFetcherTest, ReturnsNetError) {
-  base::MockCallback<MediaFeedsFetcher::MediaFeedCallback> callback;
-
   fetcher()->FetchFeed(
       GURL("https://www.google.com"), false,
       base::BindLambdaForTesting(
-          [&](const schema_org::improved::mojom::EntityPtr& response,
-              MediaFeedsFetcher::Status status, bool was_fetched_via_cache) {
-            EXPECT_EQ(status, MediaFeedsFetcher::Status::kRequestFailed);
-            EXPECT_FALSE(was_fetched_via_cache);
-            EXPECT_FALSE(response);
+          [&](media_history::MediaHistoryKeyedService::MediaFeedFetchResult
+                  result) {
+            EXPECT_EQ(result.status, mojom::FetchResult::kFailedBackendError);
+            EXPECT_FALSE(result.was_fetched_from_cache);
+            EXPECT_FALSE(result.gone);
           }));
 
   WaitForRequest();
@@ -259,16 +245,14 @@ TEST_F(MediaFeedsFetcherTest, ReturnsNetError) {
 }
 
 TEST_F(MediaFeedsFetcherTest, ReturnsErrFileNotFoundForEmptyFeedData) {
-  base::MockCallback<MediaFeedsFetcher::MediaFeedCallback> callback;
-
   fetcher()->FetchFeed(
       GURL("https://www.google.com"), false,
       base::BindLambdaForTesting(
-          [&](const schema_org::improved::mojom::EntityPtr& response,
-              MediaFeedsFetcher::Status status, bool was_fetched_via_cache) {
-            EXPECT_EQ(status, MediaFeedsFetcher::Status::kNotFound);
-            EXPECT_FALSE(was_fetched_via_cache);
-            EXPECT_FALSE(response);
+          [&](media_history::MediaHistoryKeyedService::MediaFeedFetchResult
+                  result) {
+            EXPECT_EQ(result.status, mojom::FetchResult::kFailedNetworkError);
+            EXPECT_FALSE(result.was_fetched_from_cache);
+            EXPECT_FALSE(result.gone);
           }));
 
   WaitForRequest();
@@ -276,16 +260,14 @@ TEST_F(MediaFeedsFetcherTest, ReturnsErrFileNotFoundForEmptyFeedData) {
 }
 
 TEST_F(MediaFeedsFetcherTest, ReturnsErrFailedForBadEntityData) {
-  base::MockCallback<MediaFeedsFetcher::MediaFeedCallback> callback;
-
   fetcher()->FetchFeed(
       GURL("https://www.google.com"), false,
       base::BindLambdaForTesting(
-          [&](const schema_org::improved::mojom::EntityPtr& response,
-              MediaFeedsFetcher::Status status, bool was_fetched_via_cache) {
-            EXPECT_EQ(status, MediaFeedsFetcher::Status::kInvalidFeedData);
-            EXPECT_FALSE(was_fetched_via_cache);
-            EXPECT_FALSE(response);
+          [&](media_history::MediaHistoryKeyedService::MediaFeedFetchResult
+                  result) {
+            EXPECT_EQ(result.status, mojom::FetchResult::kFailedBackendError);
+            EXPECT_FALSE(result.was_fetched_from_cache);
+            EXPECT_FALSE(result.gone);
           }));
 
   WaitForRequest();

chrome/browser/media/feeds/media_feeds_service.cc

@@ -47,21 +47,6 @@ GURL Normalize(const GURL& url) {
   return url.ReplaceComponents(replacements);
 }
 
-media_feeds::mojom::FetchResult GetFetchResult(
-    MediaFeedsFetcher::Status status) {
-  switch (status) {
-    case MediaFeedsFetcher::Status::kOk:
-      return media_feeds::mojom::FetchResult::kSuccess;
-    case MediaFeedsFetcher::Status::kInvalidFeedData:
-    case MediaFeedsFetcher::Status::kRequestFailed:
-      return media_feeds::mojom::FetchResult::kFailedBackendError;
-    case MediaFeedsFetcher::Status::kNotFound:
-      return media_feeds::mojom::FetchResult::kFailedNetworkError;
-    default:
-      return media_feeds::mojom::FetchResult::kNone;
-  }
-}
-
 class CookieChangeListener : public network::mojom::CookieChangeListener {
  public:
   using CookieCallback =
@@ -411,29 +396,19 @@ void MediaFeedsService::OnGotFetchDetails(
 void MediaFeedsService::OnFetchResponse(
     int64_t feed_id,
     base::Optional<base::UnguessableToken> reset_token,
-    const schema_org::improved::mojom::EntityPtr& response,
-    MediaFeedsFetcher::Status status,
-    bool was_fetched_via_cache) {
+    media_history::MediaHistoryKeyedService::MediaFeedFetchResult result) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
-  if (status == MediaFeedsFetcher::Status::kGone) {
+  if (result.gone) {
     GetMediaHistoryService()->DeleteMediaFeed(
         feed_id, base::BindOnce(&MediaFeedsService::OnCompleteFetch,
                                 weak_factory_.GetWeakPtr(), feed_id, false));
     return;
   }
 
-  media_history::MediaHistoryKeyedService::MediaFeedFetchResult result;
   result.feed_id = feed_id;
-  result.status = GetFetchResult(status);
-  result.was_fetched_from_cache = was_fetched_via_cache;
   result.reset_token = reset_token;
 
-  if (result.status == media_feeds::mojom::FetchResult::kSuccess &&
-      !media_feeds_converter_.ConvertMediaFeed(response, &result)) {
-    result.status = media_feeds::mojom::FetchResult::kInvalidFeed;
-  }
-
   const bool has_items = !result.items.empty();
 
   GetMediaHistoryService()->StoreMediaFeedFetchResult(

chrome/browser/media/feeds/media_feeds_service.h

@@ -98,11 +98,10 @@ class MediaFeedsService : public KeyedService {
           media_history::MediaHistoryKeyedService::MediaFeedFetchDetails>
           details);
 
-  void OnFetchResponse(int64_t feed_id,
+  void OnFetchResponse(
+      int64_t feed_id,
       base::Optional<base::UnguessableToken> reset_token,
-                       const schema_org::improved::mojom::EntityPtr& response,
-                       MediaFeedsFetcher::Status status,
-                       bool was_fetched_via_cache);
+      media_history::MediaHistoryKeyedService::MediaFeedFetchResult result);
 
   void OnCompleteFetch(const int64_t feed_id, const bool has_items);
 
@@ -160,8 +159,6 @@ class MediaFeedsService : public KeyedService {
 
   base::OnceClosure cookie_change_callback_;
 
-  MediaFeedsConverter media_feeds_converter_;
-
   std::unique_ptr<safe_search_api::URLChecker> safe_search_url_checker_;
   Profile* const profile_;
 

chrome/browser/media/history/media_history_keyed_service.h

@@ -137,6 +137,9 @@ class MediaHistoryKeyedService : public KeyedService,
     // Logs about any errors that may have occurred while fetching or converting
     // the feed data.
     std::string error_logs;
+
+    // If true then the backend returned a 410 Gone error.
+    bool gone = false;
   };
   // Replaces the media items in |result.feed_id|. This will delete any old feed
   // items and store the new ones in |result.items|. This will also update the