[Media Feeds] Display error logs in media feeds UI.

Bug: 1081515
Change-Id: Id60a43f4a9d3093992670676627a05baac258b88
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2210795
Commit-Queue: Sam Bowen <sgbowen@google.com>
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771991}

chrome/browser/media/feeds/media_feeds_browsertest.cc

@@ -263,8 +263,10 @@ IN_PROC_BROWSER_TEST_F(MediaFeedsBrowserTest, DiscoverAndFetch) {
   EXPECT_EQ(1u, discovered_feeds.size());
 
   base::RunLoop run_loop;
-  GetMediaFeedsService()->FetchMediaFeed(discovered_feeds[0]->id,
-                                         run_loop.QuitClosure());
+  GetMediaFeedsService()->FetchMediaFeed(
+      discovered_feeds[0]->id,
+      base::BindLambdaForTesting(
+          [&](const std::string& ignored) { run_loop.Quit(); }));
   run_loop.Run();
   WaitForDB();
 
@@ -663,8 +665,10 @@ IN_PROC_BROWSER_TEST_F(MediaFeedsBrowserTest, DiscoverAndFetchMinimal) {
   EXPECT_EQ(1u, discovered_feeds.size());
 
   base::RunLoop run_loop;
-  GetMediaFeedsService()->FetchMediaFeed(discovered_feeds[0]->id,
-                                         run_loop.QuitClosure());
+  GetMediaFeedsService()->FetchMediaFeed(
+      discovered_feeds[0]->id,
+      base::BindLambdaForTesting(
+          [&](const std::string& ignored) { run_loop.Quit(); }));
   run_loop.Run();
   WaitForDB();
 
@@ -809,8 +813,10 @@ IN_PROC_BROWSER_TEST_F(MediaFeedsBrowserTest, ResetMediaFeed_OnNavigation) {
 
     // Fetch the feed.
     base::RunLoop run_loop;
-    GetMediaFeedsService()->FetchMediaFeed(feeds[0]->id,
-                                           run_loop.QuitClosure());
+    GetMediaFeedsService()->FetchMediaFeed(
+        feeds[0]->id,
+        base::BindLambdaForTesting(
+            [&](const std::string& ignored) { run_loop.Quit(); }));
     run_loop.Run();
     WaitForDB();
   }
@@ -868,8 +874,10 @@ IN_PROC_BROWSER_TEST_F(MediaFeedsBrowserTest,
 
     // Fetch the feed.
     base::RunLoop run_loop;
-    GetMediaFeedsService()->FetchMediaFeed(feeds[0]->id,
-                                           run_loop.QuitClosure());
+    GetMediaFeedsService()->FetchMediaFeed(
+        feeds[0]->id,
+        base::BindLambdaForTesting(
+            [&](const std::string& ignored) { run_loop.Quit(); }));
     run_loop.Run();
     WaitForDB();
   }
@@ -899,8 +907,10 @@ IN_PROC_BROWSER_TEST_F(MediaFeedsBrowserTest,
 
     // Fetch the feed.
     base::RunLoop run_loop;
-    GetMediaFeedsService()->FetchMediaFeed(feeds[0]->id,
-                                           run_loop.QuitClosure());
+    GetMediaFeedsService()->FetchMediaFeed(
+        feeds[0]->id,
+        base::BindLambdaForTesting(
+            [&](const std::string& ignored) { run_loop.Quit(); }));
     run_loop.Run();
     WaitForDB();
   }

chrome/browser/media/feeds/media_feeds_service.cc

@@ -187,7 +187,7 @@ void MediaFeedsService::SetSafeSearchCompletionCallbackForTest(
 }
 
 void MediaFeedsService::FetchMediaFeed(int64_t feed_id,
-                                       base::OnceClosure callback) {
+                                       FetchMediaFeedCallback callback) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
   // Skip the fetch if there is already an ongoing fetch for this feed. However,
@@ -352,7 +352,7 @@ bool MediaFeedsService::IsSafeSearchCheckingEnabled() const {
 
 MediaFeedsService::InflightFeedFetch::InflightFeedFetch(
     std::unique_ptr<MediaFeedsFetcher> fetcher,
-    base::OnceClosure callback)
+    FetchMediaFeedCallback callback)
     : fetcher(std::move(fetcher)) {
   callbacks.push_back(std::move(callback));
 }
@@ -378,7 +378,7 @@ void MediaFeedsService::OnGotFetchDetails(
   DCHECK(base::Contains(fetches_, feed_id));
 
   if (!details.has_value()) {
-    OnCompleteFetch(feed_id, false);
+    OnCompleteFetch(feed_id, false, "Missing feed fetch details");
     return;
   }
 
@@ -402,7 +402,9 @@ void MediaFeedsService::OnFetchResponse(
   if (result.gone) {
     GetMediaHistoryService()->DeleteMediaFeed(
         feed_id, base::BindOnce(&MediaFeedsService::OnCompleteFetch,
-                                weak_factory_.GetWeakPtr(), feed_id, false));
+                                weak_factory_.GetWeakPtr(), feed_id, false,
+                                "The server returned 410 Gone which resulted "
+                                "in the feed being deleted."));
     return;
   }
 
@@ -410,20 +412,23 @@ void MediaFeedsService::OnFetchResponse(
   result.reset_token = reset_token;
 
   const bool has_items = !result.items.empty();
+  std::string error_logs;
+  error_logs.swap(result.error_logs);
 
   GetMediaHistoryService()->StoreMediaFeedFetchResult(
-      std::move(result),
-      base::BindOnce(&MediaFeedsService::OnCompleteFetch,
-                     weak_factory_.GetWeakPtr(), feed_id, has_items));
+      std::move(result), base::BindOnce(&MediaFeedsService::OnCompleteFetch,
+                                        weak_factory_.GetWeakPtr(), feed_id,
+                                        has_items, error_logs));
 }
 
 void MediaFeedsService::OnCompleteFetch(const int64_t feed_id,
-                                        const bool has_items) {
+                                        const bool has_items,
+                                        const std::string& error_logs) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(base::Contains(fetches_, feed_id));
 
   for (auto& callback : fetches_.at(feed_id).callbacks) {
-    std::move(callback).Run();
+    std::move(callback).Run(error_logs);
   }
 
   fetches_.erase(feed_id);

chrome/browser/media/feeds/media_feeds_service.h

@@ -39,6 +39,9 @@ class MediaFeedsService : public KeyedService {
  public:
   static const char kSafeSearchResultHistogramName[];
 
+  using FetchMediaFeedCallback =
+      base::OnceCallback<void(const std::string& logs)>;
+
   explicit MediaFeedsService(Profile* profile);
   ~MediaFeedsService() override;
   MediaFeedsService(const MediaFeedsService& t) = delete;
@@ -67,8 +70,7 @@ class MediaFeedsService : public KeyedService {
   // Fetches a media feed with the given ID and then store it in the
   // feeds table in media history. Runs the given callback after storing. The
   // fetch will be skipped if another fetch is currently ongoing.
-  void FetchMediaFeed(int64_t feed_id,
-                      base::OnceClosure callback);
+  void FetchMediaFeed(int64_t feed_id, FetchMediaFeedCallback callback);
 
   // Stores a callback to be called once we have completed all inflight checks.
   void SetCookieChangeCallbackForTest(base::OnceClosure callback);
@@ -103,7 +105,9 @@ class MediaFeedsService : public KeyedService {
       base::Optional<base::UnguessableToken> reset_token,
       media_history::MediaHistoryKeyedService::MediaFeedFetchResult result);
 
-  void OnCompleteFetch(const int64_t feed_id, const bool has_items);
+  void OnCompleteFetch(const int64_t feed_id,
+                       const bool has_items,
+                       const std::string& error_logs);
 
   void OnSafeSearchPrefChanged();
 
@@ -121,13 +125,13 @@ class MediaFeedsService : public KeyedService {
 
   struct InflightFeedFetch {
     InflightFeedFetch(std::unique_ptr<MediaFeedsFetcher> fetcher,
-                      base::OnceClosure callback);
+                      FetchMediaFeedCallback callback);
     ~InflightFeedFetch();
     InflightFeedFetch(InflightFeedFetch&& t);
     InflightFeedFetch(const InflightFeedFetch&) = delete;
     InflightFeedFetch& operator=(const InflightFeedFetch&) = delete;
 
-    std::vector<base::OnceClosure> callbacks;
+    std::vector<FetchMediaFeedCallback> callbacks;
 
     std::unique_ptr<MediaFeedsFetcher> fetcher;
   };

chrome/browser/media/feeds/media_feeds_service_unittest.cc

@@ -341,7 +341,9 @@ TEST_F(MediaFeedsServiceTest, FetchFeed_Success) {
 
   // Fetch the Media Feed.
   base::RunLoop run_loop;
-  GetMediaFeedsService()->FetchMediaFeed(1, run_loop.QuitClosure());
+  GetMediaFeedsService()->FetchMediaFeed(
+      1, base::BindLambdaForTesting(
+             [&](const std::string& ignored) { run_loop.Quit(); }));
   WaitForDB();
   ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
   run_loop.Run();
@@ -366,7 +368,9 @@ TEST_F(MediaFeedsServiceTest, FetchFeed_SuccessFromCache) {
 
   // Fetch the Media Feed.
   base::RunLoop run_loop;
-  GetMediaFeedsService()->FetchMediaFeed(1, run_loop.QuitClosure());
+  GetMediaFeedsService()->FetchMediaFeed(
+      1, base::BindLambdaForTesting(
+             [&](const std::string& ignored) { run_loop.Quit(); }));
   WaitForDB();
   ASSERT_TRUE(RespondToPendingFeedFetch(feed_url, true));
   run_loop.Run();
@@ -391,7 +395,9 @@ TEST_F(MediaFeedsServiceTest, FetchFeed_BackendError) {
 
   // Fetch the Media Feed.
   base::RunLoop run_loop;
-  GetMediaFeedsService()->FetchMediaFeed(1, run_loop.QuitClosure());
+  GetMediaFeedsService()->FetchMediaFeed(
+      1, base::BindLambdaForTesting(
+             [&](const std::string& ignored) { run_loop.Quit(); }));
   WaitForDB();
   ASSERT_TRUE(RespondToPendingFeedFetchWithStatus(
       feed_url, net::HTTP_INTERNAL_SERVER_ERROR));
@@ -416,7 +422,9 @@ TEST_F(MediaFeedsServiceTest, FetchFeed_NotFoundError) {
 
   // Fetch the Media Feed.
   base::RunLoop run_loop;
-  GetMediaFeedsService()->FetchMediaFeed(1, run_loop.QuitClosure());
+  GetMediaFeedsService()->FetchMediaFeed(
+      1, base::BindLambdaForTesting(
+             [&](const std::string& ignored) { run_loop.Quit(); }));
   WaitForDB();
   ASSERT_TRUE(RespondToPendingFeedFetchWithStatus(feed_url, net::HTTP_OK));
   run_loop.Run();
@@ -941,7 +949,9 @@ TEST_F(MediaFeedsServiceTest, FetcherShouldTriggerSafeSearch) {
   {
     // Fetch the Media Feed.
     base::RunLoop run_loop;
-    GetMediaFeedsService()->FetchMediaFeed(1, run_loop.QuitClosure());
+    GetMediaFeedsService()->FetchMediaFeed(
+        1, base::BindLambdaForTesting(
+               [&](const std::string& ignored) { run_loop.Quit(); }));
     WaitForDB();
     ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
     run_loop.Run();
@@ -1039,11 +1049,15 @@ TEST_F(MediaFeedsServiceTest, FetcherShouldSupportMultipleFetchesForSameFeed) {
 
   // Fetch the same feed twice.
   base::RunLoop run_loop;
-  GetMediaFeedsService()->FetchMediaFeed(1, run_loop.QuitClosure());
+  GetMediaFeedsService()->FetchMediaFeed(
+      1, base::BindLambdaForTesting(
+             [&](const std::string& ignored) { run_loop.Quit(); }));
   WaitForDB();
 
   base::RunLoop run_loop_alt;
-  GetMediaFeedsService()->FetchMediaFeed(1, run_loop_alt.QuitClosure());
+  GetMediaFeedsService()->FetchMediaFeed(
+      1, base::BindLambdaForTesting(
+             [&](const std::string& ignored) { run_loop_alt.Quit(); }));
   WaitForDB();
 
   // Respond and make sure both run loop quit closures were called.
@@ -1081,7 +1095,9 @@ TEST_F(MediaFeedsServiceTest, FetcherShouldHandleReset) {
 
   // Start fetching the feed but do not resolve the request.
   base::RunLoop run_loop;
-  GetMediaFeedsService()->FetchMediaFeed(1, run_loop.QuitClosure());
+  GetMediaFeedsService()->FetchMediaFeed(
+      1, base::BindLambdaForTesting(
+             [&](const std::string& ignored) { run_loop.Quit(); }));
   WaitForDB();
 
   // The last request was successful so we can hit the cache.
@@ -1124,7 +1140,9 @@ TEST_F(MediaFeedsServiceTest, FetcherShouldHandleReset) {
 
   // Start fetching the feed but do not resolve the request.
   base::RunLoop run_loop_alt;
-  GetMediaFeedsService()->FetchMediaFeed(1, run_loop_alt.QuitClosure());
+  GetMediaFeedsService()->FetchMediaFeed(
+      1, base::BindLambdaForTesting(
+             [&](const std::string& ignored) { run_loop_alt.Quit(); }));
   WaitForDB();
 
   // The last request failed so we should not hit the cache.

chrome/browser/media/feeds/media_feeds_store.mojom

@@ -437,6 +437,7 @@ interface MediaFeedsStore {
 
   // Fetch the details of a previously-discovered media feed and store them in
   // media history. The feed ID is the unique ID for the feed from the Media
-  // History store.
-  FetchMediaFeed(int64 feed_id) => ();
+  // History store. The logs contain information about any problems fetching or
+  // parsing the feed.
+  FetchMediaFeed(int64 feed_id) => (string logs);
 };

chrome/browser/media/history/media_history_keyed_service.h

@@ -135,7 +135,7 @@ class MediaHistoryKeyedService : public KeyedService,
     std::string cookie_name_filter;
 
     // Logs about any errors that may have occurred while fetching or converting
-    // the feed data.
+    // the feed data. New-line delimited human-readable text.
     std::string error_logs;
 
     // If true then the backend returned a 410 Gone error.

chrome/browser/resources/media/media_feeds.html

@@ -80,6 +80,10 @@
       content: '▼';
       position: absolute;
     }
+
+    div#fetch-logs-content {
+      color: red;
+    }
   </style>
 </head>
 <body>
@@ -236,6 +240,10 @@
       <tbody>
       </tbody>
     </table>
+    <div id="fetch-logs">
+      <h3>Fetch logs:</h3>
+      <div id="fetch-logs-content"></div>
+    </div>
   </div>
 </body>
 </html>

chrome/browser/resources/media/media_feeds.js

@@ -53,6 +53,10 @@ class MediaFeedsTableDelegate {
 
       a.addEventListener('click', () => {
         showFeedContents(dataRow);
+
+        // Clear old logs and hide the area from display.
+        $('fetch-logs').style.display = 'none';
+        $('fetch-logs-content').textContent = '';
       });
 
       td.appendChild(document.createElement('br'));
@@ -66,6 +70,9 @@ class MediaFeedsTableDelegate {
         store.fetchMediaFeed(dataRow.id).then(response => {
           updateFeedsTable();
           showFeedContents(dataRow);
+
+          $('fetch-logs').style.display = 'block';
+          $('fetch-logs-content').textContent = response.logs;
         });
       });
     }