Dont set the actions upload enable bit when signed out in feed v1

Bug: 1137913
Change-Id: I6ba03b8e3a7dd41bfae0a2e3f5e2a6e20e0b1098
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2468239Reviewed-by: Dan H <harringtond@chromium.org>
Commit-Queue: Vincent Boisselle <vincb@google.com>
Cr-Commit-Position: refs/heads/master@{#818247}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/library/api/host/network/HttpResponse.java

@@ -8,10 +8,12 @@ package org.chromium.chrome.browser.feed.library.api.host.network;
 public final class HttpResponse {
     private final int mResponseCode;
     private final byte[] mResponseBody;
+    private final boolean mIsSignedIn;
 
-    public HttpResponse(int responseCode, byte[] responseBody) {
+    public HttpResponse(int responseCode, byte[] responseBody, boolean isSignedIn) {
         this.mResponseCode = responseCode;
         this.mResponseBody = responseBody;
+        this.mIsSignedIn = isSignedIn;
     }
 
     /**
@@ -28,4 +30,8 @@ public final class HttpResponse {
     public byte[] getResponseBody() {
         return mResponseBody;
     }
+
+    public boolean isSignedIn() {
+        return mIsSignedIn;
+    }
 }

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/library/feedrequestmanager/FeedRequestManagerImpl.java

@@ -265,12 +265,13 @@ public class FeedRequestManagerImpl implements FeedRequestManager {
                         "FeedRequestManagerImpl consumer", () -> consumer.accept(Result.failure()));
                 return;
             }
-            handleResponseBytes(input.getResponseBody(), consumer, isRefreshRequest);
+            handleResponseBytes(
+                    input.getResponseBody(), consumer, isRefreshRequest, input.isSignedIn());
         });
     }
 
     private void handleResponseBytes(final byte[] responseBytes,
-            final Consumer<Result<Model>> consumer, boolean isRefreshRequest) {
+            final Consumer<Result<Model>> consumer, boolean isRefreshRequest, boolean isSignedIn) {
         mTaskQueue.execute(Task.HANDLE_RESPONSE_BYTES, TaskType.IMMEDIATE, () -> {
             Response response;
             boolean isLengthPrefixed = mConfiguration.getValueOrDefault(
@@ -287,8 +288,14 @@ public class FeedRequestManagerImpl implements FeedRequestManager {
                 return;
             }
             logServerCapabilities(response, isRefreshRequest);
-            mMainThreadRunner.execute("FeedRequestManagerImpl consumer",
-                    () -> consumer.accept(mProtocolAdapter.createModel(response)));
+            mMainThreadRunner.execute("FeedRequestManagerImpl consumer", () -> {
+                // Update the signed in pref only when the request is a refresh. It shouldn't be
+                // done for other requests such as load more.
+                if (isRefreshRequest) {
+                    updateLastRefreshSignedPref(isSignedIn);
+                }
+                consumer.accept(mProtocolAdapter.createModel(response));
+            });
         });
     }
 
@@ -310,6 +317,11 @@ public class FeedRequestManagerImpl implements FeedRequestManager {
                 .setBoolean(Pref.LAST_FETCH_HAD_NOTICE_CARD, hasNoticeCard);
     }
 
+    private void updateLastRefreshSignedPref(boolean isSignedIn) {
+        UserPrefs.get(Profile.getLastUsedRegularProfile())
+                .setBoolean(Pref.LAST_REFRESH_WAS_SIGNED_IN, isSignedIn);
+    }
+
     private static final class RequestBuilder {
         private ByteString mToken;
         private ConsistencyToken mConsistencyToken;

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/library/mocknetworkclient/MockServerNetworkClient.java

@@ -56,7 +56,7 @@ public final class MockServerNetworkClient implements NetworkClient {
     public void send(HttpRequest httpRequest, Consumer<HttpResponse> responseConsumer) {
         try {
             if (isAirplaneModeOn()) {
-                delayedAccept(new HttpResponse(400, new byte[] {}), responseConsumer);
+                delayedAccept(new HttpResponse(400, new byte[] {}, false), responseConsumer);
                 return;
             }
             Request request = getRequest(httpRequest);
@@ -80,7 +80,7 @@ public final class MockServerNetworkClient implements NetworkClient {
         } catch (IOException e) {
             // TODO : handle errors here
             Logger.e(TAG, e.getMessage());
-            delayedAccept(new HttpResponse(400, new byte[] {}), responseConsumer);
+            delayedAccept(new HttpResponse(400, new byte[] {}, false), responseConsumer);
         }
     }
 
@@ -121,7 +121,7 @@ public final class MockServerNetworkClient implements NetworkClient {
 
     private HttpResponse createHttpResponse(Response response) {
         if (response == null) {
-            return new HttpResponse(500, new byte[] {});
+            return new HttpResponse(500, new byte[] {}, false);
         }
 
         try {
@@ -131,10 +131,10 @@ public final class MockServerNetworkClient implements NetworkClient {
             codedOutputStream.writeUInt32NoTag(rawResponse.length);
             codedOutputStream.writeRawBytes(rawResponse);
             codedOutputStream.flush();
-            return new HttpResponse(200, newResponse);
+            return new HttpResponse(200, newResponse, false);
         } catch (IOException e) {
             Logger.e(TAG, "Error creating response", e);
-            return new HttpResponse(500, new byte[] {});
+            return new HttpResponse(500, new byte[] {}, false);
         }
     }
 }

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v1/FeedLoggingBridge.java

@@ -95,12 +95,12 @@ public class FeedLoggingBridge implements BasicLoggingApi {
 
     @Override
     public void onContentViewed(ContentLoggingData data) {
-        onShownSlice(data.getPositionInStream());
-
         // Bridge could have been destroyed for policy when this is called.
         // See https://crbug.com/901414.
         if (mNativeFeedLoggingBridge == 0) return;
 
+        onShownSlice(data.getPositionInStream());
+
         FeedLoggingBridgeJni.get().onContentViewed(mNativeFeedLoggingBridge, FeedLoggingBridge.this,
                 data.getPositionInStream(),
                 TimeUnit.SECONDS.toMillis(data.getPublishedTimeSeconds()),
@@ -112,7 +112,9 @@ public class FeedLoggingBridge implements BasicLoggingApi {
         if (mHasReachedShownIndexesThreshold) {
             return;
         }
-
+        if (!lastRefreshWasSignedIn()) {
+            return;
+        }
         if (index + 1 >= SHOWN_INDEX_THRESHOLD) {
             mHasReachedShownIndexesThreshold = true;
             UserPrefs.get(Profile.getLastUsedRegularProfile())
@@ -120,6 +122,11 @@ public class FeedLoggingBridge implements BasicLoggingApi {
         }
     }
 
+    private boolean lastRefreshWasSignedIn() {
+        return UserPrefs.get(Profile.getLastUsedRegularProfile())
+                .getBoolean(Pref.LAST_REFRESH_WAS_SIGNED_IN);
+    }
+
     @Override
     public void onContentDismissed(ContentLoggingData data, boolean wasCommitted) {
         // Bridge could have been destroyed for policy when this is called.

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v1/FeedNetworkBridge.java

@@ -43,7 +43,7 @@ public class FeedNetworkBridge implements NetworkClient {
     @Override
     public void send(HttpRequest request, Consumer<HttpResponse> responseConsumer) {
         if (mNativeBridge == 0) {
-            responseConsumer.accept(createHttpResponse(500, new byte[0]));
+            responseConsumer.accept(createHttpResponse(500, new byte[0], false));
         } else {
             FeedNetworkBridgeJni.get().sendNetworkRequest(mNativeBridge, FeedNetworkBridge.this,
                     request.getUri().toString(), request.getMethod(), request.getBody(),
@@ -61,8 +61,8 @@ public class FeedNetworkBridge implements NetworkClient {
     }
 
     @CalledByNative
-    private static HttpResponse createHttpResponse(int code, byte[] body) {
-        return new HttpResponse(code, body);
+    private static HttpResponse createHttpResponse(int code, byte[] body, boolean isSignedIn) {
+        return new HttpResponse(code, body, isSignedIn);
     }
 
     @NativeMethods

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v1/TestNetworkClient.java

@@ -151,7 +151,7 @@ public class TestNetworkClient implements NetworkClient {
             codedOutputStream.writeUInt32NoTag(rawResponse.length);
             codedOutputStream.writeRawBytes(rawResponse);
             codedOutputStream.flush();
-            return new HttpResponse(200, newResponse);
+            return new HttpResponse(200, newResponse, /* isSignedIn= */ false);
         } catch (IOException e) {
             throw new RuntimeException(e);
         }

chrome/android/feed/core/javatests/src/org/chromium/chrome/browser/feed/library/feedrequestmanager/FeedActionUploadRequestManagerTest.java

@@ -454,7 +454,7 @@ public class FeedActionUploadRequestManagerTest {
         codedOutputStream.writeUInt32NoTag(rawResponse.length);
         codedOutputStream.writeRawBytes(rawResponse);
         codedOutputStream.flush();
-        return new HttpResponse(responseCode, buffer.array());
+        return new HttpResponse(responseCode, buffer.array(), false);
     }
 
     private ActionRequest getActionRequestFromHttpRequest(HttpRequest httpRequest)

chrome/android/feed/core/javatests/src/org/chromium/chrome/browser/feed/library/feedrequestmanager/FeedRequestManagerImplTest.java

@@ -265,14 +265,14 @@ public class FeedRequestManagerImplTest {
                                                 Capability.REPORT_FEED_USER_ACTIONS_NOTICE_CARD)
                                         .build())
                         .build();
-        mFakeNetworkClient.addResponse(new HttpResponse(200, response.toByteArray()));
+        mFakeNetworkClient.addResponse(new HttpResponse(200, response.toByteArray(), false));
         mRequestManager.triggerRefresh(RequestReason.HOST_REQUESTED, input -> {});
 
         verify(mPrefService, times(1)).setBoolean(Pref.LAST_FETCH_HAD_NOTICE_CARD, true);
     }
 
     @Test
-    public void testLoadMore_setNoticeCardPref() throws Exception {
+    public void testTriggerRefresh_setLastRefreshWasSignedInPref() throws Exception {
         // Skip the read of the int that determines the length of the encoded proto. This is to
         // avoid having to encode the length which is a feature we don't want to test here.
         Configuration configuration =
@@ -287,7 +287,29 @@ public class FeedRequestManagerImplTest {
                 mFakeTooltipSupportedApi);
 
         mFakeNetworkClient.addResponse(
-                new HttpResponse(200, Response.getDefaultInstance().toByteArray()));
+                new HttpResponse(200, Response.getDefaultInstance().toByteArray(), true));
+        mRequestManager.triggerRefresh(RequestReason.HOST_REQUESTED, input -> {});
+
+        verify(mPrefService, times(1)).setBoolean(Pref.LAST_REFRESH_WAS_SIGNED_IN, true);
+    }
+
+    @Test
+    public void testLoadMore_dontSetNoticeCardPref() throws Exception {
+        // Skip the read of the int that determines the length of the encoded proto. This is to
+        // avoid having to encode the length which is a feature we don't want to test here.
+        Configuration configuration =
+                new Configuration.Builder()
+                        .put(ConfigKey.FEED_SERVER_RESPONSE_LENGTH_PREFIXED, false)
+                        .build();
+
+        mRequestManager = new FeedRequestManagerImpl(configuration, mFakeNetworkClient,
+                mFakeProtocolAdapter, new FeedExtensionRegistry(ArrayList::new), mScheduler,
+                mFakeTaskQueue, mTimingUtils, mFakeThreadUtils, mFakeActionReader, mContext,
+                mApplicationInfo, mFakeMainThreadRunner, mFakeBasicLoggingApi,
+                mFakeTooltipSupportedApi);
+
+        mFakeNetworkClient.addResponse(
+                new HttpResponse(200, Response.getDefaultInstance().toByteArray(), false));
         StreamToken token =
                 StreamToken.newBuilder()
                         .setNextPageToken(ByteString.copyFrom("abc", Charset.defaultCharset()))
@@ -298,6 +320,33 @@ public class FeedRequestManagerImplTest {
         verify(mPrefService, never()).setBoolean(Pref.LAST_FETCH_HAD_NOTICE_CARD, true);
     }
 
+    @Test
+    public void testLoadMore_dontSetLastRefreshWasSignedInPref() throws Exception {
+        // Skip the read of the int that determines the length of the encoded proto. This is to
+        // avoid having to encode the length which is a feature we don't want to test here.
+        Configuration configuration =
+                new Configuration.Builder()
+                        .put(ConfigKey.FEED_SERVER_RESPONSE_LENGTH_PREFIXED, false)
+                        .build();
+
+        mRequestManager = new FeedRequestManagerImpl(configuration, mFakeNetworkClient,
+                mFakeProtocolAdapter, new FeedExtensionRegistry(ArrayList::new), mScheduler,
+                mFakeTaskQueue, mTimingUtils, mFakeThreadUtils, mFakeActionReader, mContext,
+                mApplicationInfo, mFakeMainThreadRunner, mFakeBasicLoggingApi,
+                mFakeTooltipSupportedApi);
+
+        mFakeNetworkClient.addResponse(
+                new HttpResponse(200, Response.getDefaultInstance().toByteArray(), false));
+        StreamToken token =
+                StreamToken.newBuilder()
+                        .setNextPageToken(ByteString.copyFrom("abc", Charset.defaultCharset()))
+                        .build();
+        mFakeThreadUtils.enforceMainThread(false);
+        mRequestManager.loadMore(token, ConsistencyToken.getDefaultInstance(), input -> {});
+
+        verify(mPrefService, never()).setBoolean(Pref.LAST_REFRESH_WAS_SIGNED_IN, true);
+    }
+
     @Test
     public void testTriggerRefresh_FeedUiCapabilityAddedWhenFlagIsOn() throws Exception {
         testCapabilityAdded(ConfigKey.FEED_UI_ENABLED, Capability.FEED_UI);
@@ -791,7 +840,7 @@ public class FeedRequestManagerImplTest {
     @Test
     public void testHandleResponse_missingLengthPrefixNotSupported() {
         mFakeNetworkClient.addResponse(new HttpResponse(
-                /* responseCode= */ 200, Response.getDefaultInstance().toByteArray()));
+                /* responseCode= */ 200, Response.getDefaultInstance().toByteArray(), false));
         mRequestManager.triggerRefresh(RequestReason.HOST_REQUESTED, mConsumer);
         assertThat(mConsumer.isCalled()).isTrue();
         assertThat(mConsumedResult.isSuccessful()).isFalse();
@@ -949,7 +998,7 @@ public class FeedRequestManagerImplTest {
         codedOutputStream.writeUInt32NoTag(rawResponse.length);
         codedOutputStream.writeRawBytes(rawResponse);
         codedOutputStream.flush();
-        return new HttpResponse(responseCode, buffer.array());
+        return new HttpResponse(responseCode, buffer.array(), false);
     }
 
     private static DismissActionWithSemanticProperties buildDismissAction(

chrome/android/junit/src/org/chromium/chrome/browser/feed/v1/FeedLoggingBridgeTest.java

@@ -4,6 +4,7 @@
 package org.chromium.chrome.browser.feed.v1;
 
 import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -76,11 +77,14 @@ public class FeedLoggingBridgeTest {
         MockitoAnnotations.initMocks(this);
         ShadowRecordHistogram.reset();
         mocker.mock(FeedLoggingBridgeJni.TEST_HOOKS, mFeedLoggingBridgeJniMock);
+        when(mFeedLoggingBridgeJniMock.init(any(), any())).thenReturn((long) 1);
 
         mocker.mock(UserPrefsJni.TEST_HOOKS, mUserPrefsJniMock);
         Profile.setLastUsedProfileForTesting(mProfile);
         when(mUserPrefsJniMock.get(mProfile)).thenReturn(mPrefService);
 
+        when(mPrefService.getBoolean(Pref.LAST_REFRESH_WAS_SIGNED_IN)).thenReturn(true);
+
         Profile profile = null;
         mFakeClock = new FakeClock();
         mFeedLoggingBridge = new FeedLoggingBridge(profile, mFakeClock);
@@ -220,6 +224,20 @@ public class FeedLoggingBridgeTest {
                 .setBoolean(Pref.HAS_REACHED_CLICK_AND_VIEW_ACTIONS_UPLOAD_CONDITIONS, true);
     }
 
+    @Test
+    @SmallTest
+    @Feature({"Feed"})
+    @Features.EnableFeatures(ChromeFeatureList.INTEREST_FEEDV1_CLICKS_AND_VIEWS_CONDITIONAL_UPLOAD)
+    public void onContentViewed_dontSetPref_whenLastRefreshWasNotSignedIn() throws Exception {
+        when(mPrefService.getBoolean(Pref.LAST_REFRESH_WAS_SIGNED_IN)).thenReturn(false);
+
+        ContentLoggingData data = makeContentData(2);
+        mFeedLoggingBridge.onContentViewed(data);
+
+        verify(mPrefService, never())
+                .setBoolean(Pref.HAS_REACHED_CLICK_AND_VIEW_ACTIONS_UPLOAD_CONDITIONS, true);
+    }
+
     private void verifyHistogram(int sample, int expectedCount) {
         assertEquals(expectedCount,
                 RecordHistogram.getHistogramValueCountForTesting(

chrome/browser/android/feed/feed_network_bridge.cc

@@ -74,7 +74,8 @@ void FeedNetworkBridge::CancelRequests(JNIEnv* env,
 
 void FeedNetworkBridge::OnResult(const ScopedJavaGlobalRef<jobject>& j_callback,
                                  int32_t http_code,
-                                 std::vector<uint8_t> response_bytes) {
+                                 std::vector<uint8_t> response_bytes,
+                                 bool isSignedIn) {
   // TODO(ssid): Remove after fixing https://crbug.com/916791.
   TRACE_EVENT0("browser", "FeedNetworkBridge::OnResult");
   JNIEnv* env = base::android::AttachCurrentThread();
@@ -82,7 +83,7 @@ void FeedNetworkBridge::OnResult(const ScopedJavaGlobalRef<jobject>& j_callback,
       base::android::ToJavaByteArray(env, response_bytes);
   ScopedJavaLocalRef<jobject> j_http_response =
       Java_FeedNetworkBridge_createHttpResponse(env, http_code,
-                                                j_response_bytes);
+                                                j_response_bytes, isSignedIn);
   TRACE_EVENT0("browser", "FeedNetworkBridge::OnResult_Callback");
   base::android::RunObjectCallbackAndroid(j_callback, j_http_response);
 }

chrome/browser/android/feed/feed_network_bridge.h

@@ -42,7 +42,8 @@ class FeedNetworkBridge {
  private:
   void OnResult(const base::android::ScopedJavaGlobalRef<jobject>& j_callback,
                 int32_t http_code,
-                std::vector<uint8_t> response_bytes);
+                std::vector<uint8_t> response_bytes,
+                bool is_signed_in);
 
   FeedNetworkingHost* networking_host_;
 

components/feed/core/common/pref_names.cc

@@ -36,6 +36,7 @@ const char kHostOverrideBlessNonce[] = "feed.host_override.bless_nonce";
 const char kHasReachedClickAndViewActionsUploadConditions[] =
     "feed.clicks_and_views_upload_conditions_reached";
 const char kLastFetchHadNoticeCard[] = "feed.last_fetch_had_notice_card";
+const char kLastRefreshWasSignedIn[] = "feed.last_refresh_was_signed_in";
 
 const char kThrottlerRequestCountListPrefName[] =
     "feedv2.request_throttler.request_counts";
@@ -68,6 +69,7 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(
       feed::prefs::kHasReachedClickAndViewActionsUploadConditions, false);
   registry->RegisterBooleanPref(feed::prefs::kLastFetchHadNoticeCard, true);
+  registry->RegisterBooleanPref(feed::prefs::kLastRefreshWasSignedIn, false);
   UserClassifier::RegisterProfilePrefs(registry);
 }
 

components/feed/core/common/pref_names.h

@@ -56,6 +56,10 @@ extern const char kHasReachedClickAndViewActionsUploadConditions[];
 // detected. Currently only used in V1.
 extern const char kLastFetchHadNoticeCard[];
 
+// The pref name for the bit that determines whether the last refresh request
+// was signed in.
+extern const char kLastRefreshWasSignedIn[];
+
 // The following prefs are used only by v2.
 
 // The pref name for the request throttler counts.

components/feed/core/feed_networking_host.cc

@@ -345,7 +345,8 @@ void NetworkFetch::OnSimpleLoaderComplete(
                             static_cast<int>(response_body.size() / 1024));
   }
 
-  std::move(done_callback_).Run(status_code, std::move(response_body));
+  std::move(done_callback_)
+      .Run(status_code, std::move(response_body), !access_token_.empty());
 }
 
 FeedNetworkingHost::FeedNetworkingHost(
@@ -388,12 +389,13 @@ void FeedNetworkingHost::NetworkFetchFinished(
     NetworkFetch* fetch,
     ResponseCallback callback,
     int32_t http_code,
-    std::vector<uint8_t> response_body) {
+    std::vector<uint8_t> response_body,
+    bool is_signed_in) {
   auto fetch_iterator = pending_requests_.find(fetch);
   CHECK(fetch_iterator != pending_requests_.end());
   pending_requests_.erase(fetch_iterator);
 
-  std::move(callback).Run(http_code, std::move(response_body));
+  std::move(callback).Run(http_code, std::move(response_body), is_signed_in);
 }
 
 }  // namespace feed

components/feed/core/feed_networking_host.h

@@ -45,7 +45,8 @@ class FeedNetworkingHost {
   // status code(if the request succeeded in reaching the server).
   using ResponseCallback =
       base::OnceCallback<void(int32_t status_code,
-                              std::vector<uint8_t> response_bytes)>;
+                              std::vector<uint8_t> response_bytes,
+                              bool is_signed_in)>;
 
   FeedNetworkingHost(
       signin::IdentityManager* identity_manager,
@@ -74,7 +75,8 @@ class FeedNetworkingHost {
   void NetworkFetchFinished(NetworkFetch* fetch,
                             ResponseCallback callback,
                             int32_t http_code,
-                            std::vector<uint8_t> response_body);
+                            std::vector<uint8_t> response_body,
+                            bool isSignedIn);
 
   base::flat_set<std::unique_ptr<NetworkFetch>, base::UniquePtrComparator>
       pending_requests_;

components/feed/core/feed_networking_host_unittest.cc

@@ -46,16 +46,20 @@ class MockResponseDoneCallback {
  public:
   MockResponseDoneCallback() : has_run(false), code(0) {}
 
-  void Done(int32_t http_code, std::vector<uint8_t> response) {
+  void Done(int32_t http_code,
+            std::vector<uint8_t> response,
+            bool is_signed_in) {
     EXPECT_FALSE(has_run);
     has_run = true;
     code = http_code;
     response_bytes = std::move(response);
+    is_signed_in_result = is_signed_in;
   }
 
   bool has_run;
   int32_t code;
   std::vector<uint8_t> response_bytes;
+  bool is_signed_in_result;
 };
 
 }  // namespace
@@ -266,6 +270,15 @@ TEST_F(FeedNetworkingHostTest, ShouldSetHeadersCorrectly) {
   EXPECT_EQ(authorization, "Bearer access_token");
 }
 
+TEST_F(FeedNetworkingHostTest, ProvideIsSignedInBitInResult) {
+  MockResponseDoneCallback done_callback;
+  SendRequestAndRespond("http://foobar.com/feed", "POST", "body", "",
+                        net::HTTP_OK, network::URLLoaderCompletionStatus(),
+                        &done_callback);
+
+  EXPECT_TRUE(done_callback.is_signed_in_result);
+}
+
 TEST_F(FeedNetworkingHostTest, ShouldNotSendContentEncodingForEmptyBody) {
   MockResponseDoneCallback done_callback;
   net::HttpRequestHeaders headers;