feedv2: Add network client interface to XSurface, plus bridge to native

- Add NetworkResponse type to types.h and make ImageFetcher use it
- Unrelated to image fetching, add implementations for logError and logWarning so incoming log calls aren't silently dropped by XSurface's empty default implementations

Bug: 1044139
Change-Id: Ic5ee9dc10c256a3362a2999dd3c766df66f2f09d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2347404Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Mohamed Heikal <mheikal@chromium.org>
Reviewed-by: Dan H <harringtond@chromium.org>
Commit-Queue: Ian Wells <iwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798763}

chrome/android/BUILD.gn

@@ -3192,6 +3192,7 @@ generate_jni("chrome_jni_headers") {
       "feed/core/java/src/org/chromium/chrome/browser/feed/FeedNetworkBridge.java",
       "feed/core/java/src/org/chromium/chrome/browser/feed/FeedOfflineBridge.java",
       "feed/core/java/src/org/chromium/chrome/browser/feed/FeedSchedulerBridge.java",
+      "feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedImageFetchClient.java",
       "feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedServiceBridge.java",
       "feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedStreamSurface.java",
     ]

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedImageFetchClient.java

+// 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.
+
+package org.chromium.chrome.browser.feed.v2;
+
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.JNINamespace;
+import org.chromium.base.annotations.NativeMethods;
+import org.chromium.chrome.browser.xsurface.ImageFetchClient;
+
+/**
+ * Implementation of xsurface's ImageFetchClient. Calls through to the native network stack.
+ */
+@JNINamespace("feed")
+public class FeedImageFetchClient implements ImageFetchClient {
+    private static class HttpResponseImpl implements ImageFetchClient.HttpResponse {
+        private int mStatus;
+        private byte[] mBody;
+
+        public HttpResponseImpl(int status, byte[] body) {
+            mStatus = status;
+            mBody = body;
+        }
+
+        @Override
+        public int status() {
+            return mStatus;
+        }
+
+        @Override
+        public byte[] body() {
+            return mBody;
+        }
+    }
+
+    @Override
+    public void sendRequest(String url, ImageFetchClient.HttpResponseConsumer responseConsumer) {
+        FeedImageFetchClientJni.get().sendRequest(url, responseConsumer);
+    }
+
+    @CalledByNative
+    public static void onHttpResponse(
+            ImageFetchClient.HttpResponseConsumer responseConsumer, int status, byte[] body) {
+        responseConsumer.requestComplete(new HttpResponseImpl(status, body));
+    }
+
+    @NativeMethods
+    interface Natives {
+        void sendRequest(String url, ImageFetchClient.HttpResponseConsumer responseConsumer);
+    }
+}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedStreamSurface.java

@@ -41,6 +41,7 @@ import org.chromium.chrome.browser.ui.messages.snackbar.Snackbar;
 import org.chromium.chrome.browser.ui.messages.snackbar.SnackbarManager;
 import org.chromium.chrome.browser.xsurface.FeedActionsHandler;
 import org.chromium.chrome.browser.xsurface.HybridListRenderer;
+import org.chromium.chrome.browser.xsurface.ImageFetchClient;
 import org.chromium.chrome.browser.xsurface.ProcessScope;
 import org.chromium.chrome.browser.xsurface.ProcessScopeDependencyProvider;
 import org.chromium.chrome.browser.xsurface.SurfaceActionsHandler;
@@ -201,7 +202,11 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
      */
     private static class FeedProcessScopeDependencyProvider
             implements ProcessScopeDependencyProvider {
-        FeedProcessScopeDependencyProvider() {}
+        private ImageFetchClient mImageFetchClient;
+
+        FeedProcessScopeDependencyProvider() {
+            mImageFetchClient = new FeedImageFetchClient();
+        }
 
         @Override
         public Context getContext() {
@@ -226,6 +231,21 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
         public String getClientInstanceId() {
             return FeedServiceBridge.getClientInstanceId();
         }
+
+        @Override
+        public ImageFetchClient getImageFetchClient() {
+            return mImageFetchClient;
+        }
+
+        @Override
+        public void logError(String tag, String format, Object... args) {
+            Log.e(tag, format, args);
+        }
+
+        @Override
+        public void logWarning(String tag, String format, Object... args) {
+            Log.w(tag, format, args);
+        }
     }
 
     /**

chrome/android/feed/feed_java_sources.gni

@@ -386,6 +386,7 @@ if (enable_feed_in_chrome) {
     "//chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/tooltip/BasicTooltipSupportedApi.java",
     "//chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/tooltip/FeedTooltipUtils.java",
     "//chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/CardMenuBottomSheetContent.java",
+    "//chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedImageFetchClient.java",
     "//chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedListContentManager.java",
     "//chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedServiceBridge.java",
     "//chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedSliceViewTracker.java",

chrome/browser/BUILD.gn

@@ -5186,6 +5186,7 @@ static_library("browser") {
       "android/feed/history/feed_history_helper.h",
       "android/feed/v2/background_refresh_task.cc",
       "android/feed/v2/background_refresh_task.h",
+      "android/feed/v2/feed_image_fetch_client.cc",
       "android/feed/v2/feed_service_bridge.cc",
       "android/feed/v2/feed_service_bridge.h",
       "android/feed/v2/feed_service_factory.cc",

chrome/browser/android/feed/v2/feed_image_fetch_client.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 <memory>
+#include "base/android/jni_android.h"
+#include "base/android/jni_array.h"
+#include "base/android/jni_string.h"
+#include "chrome/android/chrome_jni_headers/FeedImageFetchClient_jni.h"
+#include "chrome/browser/android/feed/v2/feed_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "components/feed/core/v2/public/feed_service.h"
+#include "components/feed/core/v2/public/types.h"
+
+using base::android::JavaParamRef;
+
+namespace feed {
+namespace {
+
+void OnFetchFinished(JNIEnv* env,
+                     base::android::ScopedJavaGlobalRef<jobject> callback,
+                     NetworkResponse response) {
+  Java_FeedImageFetchClient_onHttpResponse(
+      env, callback, response.status_code,
+      base::android::ToJavaByteArray(env, response.response_bytes));
+}
+
+}  // namespace
+
+void JNI_FeedImageFetchClient_SendRequest(
+    JNIEnv* env,
+    const JavaParamRef<jstring>& j_url,
+    const JavaParamRef<jobject>& j_response_callback) {
+  // Keep the callback as a ScopedJavaGlobalRef to enable binding it for use
+  // with OnFetchFinished.
+  base::android::ScopedJavaGlobalRef<jobject> callback(j_response_callback);
+
+  Profile* profile = ProfileManager::GetLastUsedProfile();
+  if (!profile)
+    return OnFetchFinished(env, std::move(callback), {});
+
+  FeedService* feed_service = FeedServiceFactory::GetForBrowserContext(profile);
+  if (!feed_service || !feed_service->GetStream())
+    return OnFetchFinished(env, std::move(callback), {});
+
+  feed_service->GetStream()->FetchImage(
+      GURL(base::android::ConvertJavaStringToUTF8(env, j_url)),
+      base::BindOnce(&OnFetchFinished, env, std::move(callback)));
+}
+
+}  // namespace feed

chrome/browser/xsurface/BUILD.gn

@@ -8,6 +8,7 @@ android_library("java") {
   sources = [
     "android/java/src/org/chromium/chrome/browser/xsurface/FeedActionsHandler.java",
     "android/java/src/org/chromium/chrome/browser/xsurface/HybridListRenderer.java",
+    "android/java/src/org/chromium/chrome/browser/xsurface/ImageFetchClient.java",
     "android/java/src/org/chromium/chrome/browser/xsurface/ListContentManager.java",
     "android/java/src/org/chromium/chrome/browser/xsurface/ListContentManagerObserver.java",
     "android/java/src/org/chromium/chrome/browser/xsurface/ProcessScope.java",

chrome/browser/xsurface/android/java/src/org/chromium/chrome/browser/xsurface/ImageFetchClient.java

+// 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.
+
+package org.chromium.chrome.browser.xsurface;
+
+/**
+ * An object that can send an HTTP GET request and receive bytes in response. This interface should
+ * only be used for fetching images.
+ */
+public interface ImageFetchClient {
+    /**
+     * HTTP response.
+     */
+    public interface HttpResponse {
+        /**
+         * HTTP status code if there was a response, or a net::Error if not.
+         */
+        default int status() {
+            return -2; // net::FAILED
+        }
+        default byte[] body() {
+            return new byte[0];
+        }
+    }
+
+    /**
+     * HTTP response callback interface.
+     */
+    public interface HttpResponseConsumer {
+        default void requestComplete(HttpResponse response) {}
+    }
+
+    /**
+     * Send a GET request. Upon completion, asynchronously calls the consumer with all body bytes
+     * from the response.
+     *
+     * @param url URL to request
+     * @param responseConsumer The callback to call with the response
+     */
+    default void sendRequest(String url, HttpResponseConsumer responseConsumer) {}
+}

chrome/browser/xsurface/android/java/src/org/chromium/chrome/browser/xsurface/ProcessScopeDependencyProvider.java

@@ -38,4 +38,12 @@ public interface ProcessScopeDependencyProvider {
 
     /** @see {Log.w} */
     default void logWarning(String tag, String messageTemplate, Object... args) {}
+
+    /**
+     * Returns an ImageFetchClient. ImageFetchClient should only be used for fetching images.
+     */
+    @Nullable
+    default ImageFetchClient getImageFetchClient() {
+        return null;
+    }
 }

components/feed/core/v2/feed_stream.cc

@@ -601,7 +601,7 @@ void FeedStream::FinishClearAll() {
 
 void FeedStream::FetchImage(
     const GURL& url,
-    base::OnceCallback<void(std::unique_ptr<std::string>)> callback) {
+    base::OnceCallback<void(NetworkResponse)> callback) {
   image_fetcher_->Fetch(url, std::move(callback));
 }
 

components/feed/core/v2/feed_stream.h

@@ -126,9 +126,8 @@ 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 FetchImage(const GURL& url,
+                  base::OnceCallback<void(NetworkResponse)> callback) override;
   void LoadMore(SurfaceId surface_id,
                 base::OnceCallback<void(bool)> callback) override;
   void ExecuteOperations(

components/feed/core/v2/feed_stream_unittest.cc

@@ -265,10 +265,9 @@ class TestImageFetcher : public ImageFetcher {
       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 {
+             base::OnceCallback<void(NetworkResponse)> callback) override {
     // Emulate a response.
-    auto response = std::make_unique<std::string>("dummyresponse");
+    NetworkResponse response = {"dummyresponse", 200};
     std::move(callback).Run(std::move(response));
   }
 };
@@ -876,10 +875,10 @@ TEST_F(FeedStreamTest, DetachSurface) {
 }
 
 TEST_F(FeedStreamTest, FetchImage) {
-  CallbackReceiver<std::unique_ptr<std::string>> receiver;
+  CallbackReceiver<NetworkResponse> receiver;
   stream_->FetchImage(GURL("https://example.com"), receiver.Bind());
 
-  EXPECT_EQ("dummyresponse", **receiver.GetResult());
+  EXPECT_EQ("dummyresponse", receiver.GetResult()->response_bytes);
 }
 
 TEST_F(FeedStreamTest, LoadFromNetwork) {

components/feed/core/v2/image_fetcher.cc

@@ -4,6 +4,7 @@
 
 #include "components/feed/core/v2/image_fetcher.h"
 
+#include "components/feed/core/v2/public/types.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"
@@ -56,7 +57,15 @@ 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));
+  NetworkResponse response{std::string(), simple_loader->NetError()};
+  if (simple_loader->ResponseInfo() && simple_loader->ResponseInfo()->headers) {
+    response.status_code =
+        simple_loader->ResponseInfo()->headers->response_code();
+  }
+
+  if (response_data)
+    response.response_bytes = std::move(*response_data);
+  std::move(callback).Run(std::move(response));
 }
 
-}  // namespace feed
+}  // namespace feed
\ No newline at end of file

components/feed/core/v2/image_fetcher.h

@@ -17,10 +17,12 @@ class SimpleURLLoader;
 
 namespace feed {
 
+struct NetworkResponse;
+
 // Fetcher object to retrieve an image resource from a URL.
 class ImageFetcher {
  public:
-  using ImageCallback = base::OnceCallback<void(std::unique_ptr<std::string>)>;
+  using ImageCallback = base::OnceCallback<void(NetworkResponse)>;
   explicit ImageFetcher(
       scoped_refptr<::network::SharedURLLoaderFactory> url_loader_factory);
   virtual ~ImageFetcher();
@@ -41,4 +43,4 @@ class ImageFetcher {
 
 }  // namespace feed
 
-#endif  // COMPONENTS_FEED_CORE_V2_IMAGE_FETCHER_H_
+#endif  // COMPONENTS_FEED_CORE_V2_IMAGE_FETCHER_H_
\ No newline at end of file

components/feed/core/v2/image_fetcher_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/strings/string_split.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/task_environment.h"
+#include "components/feed/core/v2/public/types.h"
 #include "components/feed/core/v2/test/callback_receiver.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
@@ -83,7 +84,7 @@ class ImageFetcherTest : public testing::Test {
 };
 
 TEST_F(ImageFetcherTest, SendRequestSendsValidRequest) {
-  CallbackReceiver<std::unique_ptr<std::string>> receiver;
+  CallbackReceiver<NetworkResponse> receiver;
   image_fetcher()->Fetch(GURL("https://example.com"), receiver.Bind());
   network::ResourceRequest resource_request =
       RespondToRequest("", net::HTTP_OK);
@@ -93,42 +94,48 @@ TEST_F(ImageFetcherTest, SendRequestSendsValidRequest) {
 }
 
 TEST_F(ImageFetcherTest, SendRequestValidResponse) {
-  CallbackReceiver<std::unique_ptr<std::string>> receiver;
+  CallbackReceiver<NetworkResponse> 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"));
+  EXPECT_THAT(receiver.GetResult()->response_bytes,
+              HasSubstr("example_response"));
+  EXPECT_EQ(net::HTTP_OK, receiver.GetResult()->status_code);
 }
 
 TEST_F(ImageFetcherTest, SendSequentialRequestsValidResponses) {
-  CallbackReceiver<std::unique_ptr<std::string>> receiver1;
+  CallbackReceiver<NetworkResponse> receiver1;
   image_fetcher()->Fetch(GURL("https://example1.com"), receiver1.Bind());
   RespondToRequest("example1_response", net::HTTP_OK);
 
-  CallbackReceiver<std::unique_ptr<std::string>> receiver2;
+  CallbackReceiver<NetworkResponse> 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"));
+  EXPECT_THAT(receiver1.GetResult()->response_bytes,
+              HasSubstr("example1_response"));
   ASSERT_TRUE(receiver2.GetResult());
-  EXPECT_THAT(**receiver2.GetResult(), HasSubstr("example2_response"));
+  EXPECT_THAT(receiver2.GetResult()->response_bytes,
+              HasSubstr("example2_response"));
 }
 
 TEST_F(ImageFetcherTest, SendParallelRequestsValidResponses) {
-  CallbackReceiver<std::unique_ptr<std::string>> receiver1;
+  CallbackReceiver<NetworkResponse> receiver1;
   image_fetcher()->Fetch(GURL("https://example1.com"), receiver1.Bind());
-  CallbackReceiver<std::unique_ptr<std::string>> receiver2;
+  CallbackReceiver<NetworkResponse> 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"));
+  EXPECT_THAT(receiver1.GetResult()->response_bytes,
+              HasSubstr("example1_response"));
   ASSERT_TRUE(receiver2.GetResult());
-  EXPECT_THAT(**receiver2.GetResult(), HasSubstr("example2_response"));
+  EXPECT_THAT(receiver2.GetResult()->response_bytes,
+              HasSubstr("example2_response"));
 }
 
 }  // namespace

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

@@ -76,7 +76,7 @@ class FeedStreamApi {
   // with the network response when complete.
   virtual void FetchImage(
       const GURL& url,
-      base::OnceCallback<void(std::unique_ptr<std::string>)> callback) = 0;
+      base::OnceCallback<void(NetworkResponse)> callback) = 0;
 
   // Apply |operations| to the stream model. Does nothing if the model is not
   // yet loaded.

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

@@ -49,6 +49,16 @@ struct NetworkResponseInfo {
   size_t response_body_bytes = 0;
 };
 
+struct NetworkResponse {
+  // HTTP response body.
+  std::string response_bytes;
+  // HTTP status code if available, or net::Error otherwise.
+  int status_code;
+
+  NetworkResponse(NetworkResponse&& other) = default;
+  NetworkResponse& operator=(NetworkResponse&& other) = default;
+};
+
 // For the snippets-internals page.
 struct DebugStreamData {
   static const int kVersion = 1;  // If a field changes, increment.