[Feed] Check loadUrlParams for null, fallback to opening online.

Bug: 901179
Change-Id: Ieb3a421a7930ddc0014f7ef57b6a8f6d89dc3615
Reviewed-on: https://chromium-review.googlesource.com/c/1315358
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606886}

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

@@ -109,27 +109,14 @@ public class FeedLoggingBridge implements BasicLoggingApi {
      * Reports how long a user spends on the page.
      *
      * @param visitTimeMs Time spent reading the page.
+     * @param isOffline If the page is viewed in offline mode or not.
      */
-    public void onContentTargetVisited(long visitTimeMs) {
-        // We cannot assume that the |mNativeFeedLoggingBridge| is always available like other
+    public void onContentTargetVisited(long visitTimeMs, boolean isOffline) {
+        // We cannot assume that the|mNativeFeedLoggingBridge| is always available like other
         // methods. This method is called by objects not controlled by Feed lifetimes, and destroy()
         // may have already been called if Feed is disabled by policy.
         if (mNativeFeedLoggingBridge != 0) {
-            nativeOnContentTargetVisited(mNativeFeedLoggingBridge, visitTimeMs);
-        }
-    }
-
-    /**
-     * Reports how long a user spends on the offline page.
-     *
-     * @param visitTimeMs Time spent reading the page.
-     */
-    public void onOfflinePageVisited(long visitTimeMs) {
-        // We cannot assume that the |mNativeFeedLoggingBridge| is always available like other
-        // methods. This method is called by objects not controlled by Feed lifetimes, and destroy()
-        // may have already been called if Feed is disabled by policy.
-        if (mNativeFeedLoggingBridge != 0) {
-            nativeOnOfflinePageVisited(mNativeFeedLoggingBridge, visitTimeMs);
+            nativeOnContentTargetVisited(mNativeFeedLoggingBridge, visitTimeMs, isOffline);
         }
     }
 
@@ -171,6 +158,5 @@ public class FeedLoggingBridge implements BasicLoggingApi {
     private native void nativeOnOpenedWithNoImmediateContent(long nativeFeedLoggingBridge);
     private native void nativeOnOpenedWithNoContent(long nativeFeedLoggingBridge);
     private native void nativeOnContentTargetVisited(
-            long nativeFeedLoggingBridge, long visitTimeMs);
-    private native void nativeOnOfflinePageVisited(long nativeFeedLoggingBridge, long visitTimeMs);
+            long nativeFeedLoggingBridge, long visitTimeMs, boolean isOffline);
 }

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/action/FeedActionHandler.java

@@ -140,26 +140,38 @@ public class FeedActionHandler implements ActionApi {
     private void openOfflineIfPossible(int disposition, String url) {
         Long maybeOfflineId = mOfflineIndicator.getOfflineIdIfPageIsOfflined(url);
         if (maybeOfflineId == null) {
-            Tab loadingTab = mDelegate.openUrl(disposition, createLoadUrlParams(url));
-            if (loadingTab != null) {
-                // Records how long the user spending on the suggested page.
-                NavigationRecorder.record(loadingTab, visitData -> {
-                    mLoggingBridge.onContentTargetVisited(visitData.duration);
-                });
-            }
+            openAndRecord(disposition, createLoadUrlParams(url), /*isOffline*/ false);
         } else {
             mOfflinePageBridge.getLoadUrlParamsByOfflineId(
                     maybeOfflineId, LaunchLocation.SUGGESTION, (loadUrlParams) -> {
+                        if (loadUrlParams == null) {
+                            // Fall back to opening online if the lookup failed.
+                            openAndRecord(
+                                    disposition, createLoadUrlParams(url), /*isOffline*/ false);
+                        } else {
+                            // Offline headers need to be moved to be read correctly.
                             loadUrlParams.setVerbatimHeaders(loadUrlParams.getExtraHeadersString());
-                        Tab loadingTab = mDelegate.openUrl(disposition, loadUrlParams);
-                        if (loadingTab != null) {
-                            // Records how long the user spending on the offline page.
-                            NavigationRecorder.record(loadingTab, visitData -> {
-                                mLoggingBridge.onOfflinePageVisited(visitData.duration);
-                            });
+                            openAndRecord(disposition, loadUrlParams, /*isOffline*/ true);
                         }
                     });
         }
+    }
+    /**
+     * Opens the given resource, specified by params, and records how much time the user spends on
+     * the suggested page.
+     *
+     * @param disposition How to open the article.
+     * @param loadUrlParams Parameters specifying the URL to load and other navigation details.
+     * @param isOffline If the page should open in offline mode or not, for metrics reporting.
+     */
+    private void openAndRecord(int disposition, LoadUrlParams loadUrlParams, boolean isOffline) {
+        Tab loadingTab = mDelegate.openUrl(disposition, loadUrlParams);
+        if (loadingTab != null) {
+            // Records how long the user spending on the suggested page.
+            NavigationRecorder.record(loadingTab,
+                    visitData
+                    -> mLoggingBridge.onContentTargetVisited(visitData.duration, isOffline));
+        }
         mSuggestionConsumedObserver.run();
     }
 }

chrome/android/junit/src/org/chromium/chrome/browser/feed/action/FeedActionHandlerTest.java

@@ -6,10 +6,13 @@ package org.chromium.chrome.browser.feed.action;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -27,6 +30,7 @@ import org.mockito.ArgumentCaptor;
 import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
+import org.mockito.invocation.InvocationOnMock;
 import org.robolectric.annotation.Config;
 
 import org.chromium.base.Callback;
@@ -36,7 +40,11 @@ import org.chromium.chrome.browser.feed.FeedLoggingBridge;
 import org.chromium.chrome.browser.feed.FeedOfflineIndicator;
 import org.chromium.chrome.browser.offlinepages.OfflinePageBridge;
 import org.chromium.chrome.browser.suggestions.SuggestionsNavigationDelegate;
+import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.content_public.browser.LoadUrlParams;
+import org.chromium.content_public.browser.NavigationController;
+import org.chromium.content_public.browser.WebContents;
+import org.chromium.content_public.browser.WebContentsObserver;
 import org.chromium.ui.mojom.WindowOpenDisposition;
 
 import java.util.Collections;
@@ -61,6 +69,12 @@ public class FeedActionHandlerTest {
     private OfflinePageBridge mOfflinePageBridge;
     @Mock
     private FeedLoggingBridge mLoggingBridge;
+    @Mock
+    private Tab mTab;
+    @Mock
+    private WebContents mWebContents;
+    @Mock
+    private NavigationController mNavigationController;
 
     @Captor
     private ArgumentCaptor<Integer> mDispositionCapture;
@@ -69,8 +83,11 @@ public class FeedActionHandlerTest {
     @Captor
     private ArgumentCaptor<Long> mOfflineIdCapture;
     @Captor
-    private ArgumentCaptor<Callback<LoadUrlParams>> mLoadUrlParamsCallbackCapture;
+    private ArgumentCaptor<Callback<LoadUrlParams>> mLoadUrlParamsCallbackCaptor;
+    @Captor
+    ArgumentCaptor<WebContentsObserver> mWebContentsObserverCaptor;
 
+    int mLastCommittedEntryIndex = 0;
     private FeedActionHandler mActionHandler;
 
     private void verifyOpenedOffline(int expectedDisposition) {
@@ -97,22 +114,43 @@ public class FeedActionHandlerTest {
         mActionHandler = new FeedActionHandler(mDelegate, mSuggestionConsumedObserver,
                 mOfflineIndicator, mOfflinePageBridge, mLoggingBridge);
 
-        doAnswer(invocation -> {
-            LoadUrlParams params = new LoadUrlParams("");
-            params.setExtraHeaders(Collections.singletonMap("", OFFLINE_ID.toString()));
-            mLoadUrlParamsCallbackCapture.getValue().onResult(params);
+        // Setup mocks such that when NavigationRecorder#record is called, it immediately invokes
+        // the passed callback.
+        when(mDelegate.openUrl(anyInt(), any(LoadUrlParams.class))).thenReturn(mTab);
+        when(mTab.getWebContents()).thenReturn(mWebContents);
+        when(mWebContents.getNavigationController()).thenReturn(mNavigationController);
+        when(mNavigationController.getLastCommittedEntryIndex())
+                .thenReturn(mLastCommittedEntryIndex++);
+        doAnswer((InvocationOnMock invocation) -> {
+            mWebContentsObserverCaptor.getValue().navigationEntryCommitted();
             return null;
         })
-                .when(mOfflinePageBridge)
-                .getLoadUrlParamsByOfflineId(mOfflineIdCapture.capture(), anyInt(),
-                        mLoadUrlParamsCallbackCapture.capture());
-        doNothing().when(mLoggingBridge).onContentTargetVisited(anyLong());
+                .when(mWebContents)
+                .addObserver(mWebContentsObserverCaptor.capture());
 
         Map<String, Boolean> featureMap = new ArrayMap<>();
         featureMap.put(ChromeFeatureList.INTEREST_FEED_CONTENT_SUGGESTIONS, true);
         ChromeFeatureList.setTestFeatures(featureMap);
     }
 
+    private void answerWithGoodParams() {
+        LoadUrlParams params = new LoadUrlParams("");
+        params.setExtraHeaders(Collections.singletonMap("", OFFLINE_ID.toString()));
+        answerWithGivenParams(params);
+    }
+
+    // Configures mOfflinePageBridge to run the passed callback when getLoadUrlParamsByOfflineId is
+    // called. If this isn't setup, the callback will never be invoked.
+    private void answerWithGivenParams(LoadUrlParams params) {
+        doAnswer(invocation -> {
+            mLoadUrlParamsCallbackCaptor.getValue().onResult(params);
+            return null;
+        })
+                .when(mOfflinePageBridge)
+                .getLoadUrlParamsByOfflineId(mOfflineIdCapture.capture(), anyInt(),
+                        mLoadUrlParamsCallbackCaptor.capture());
+    }
+
     @After
     public void tearDown() {
         ChromeFeatureList.setTestFeatures(null);
@@ -120,18 +158,32 @@ public class FeedActionHandlerTest {
 
     @Test
     @SmallTest
-    public void testOpenUrlOnline() {
+    public void testOpenUrlOffline() {
         when(mOfflineIndicator.getOfflineIdIfPageIsOfflined(TEST_URL)).thenReturn(OFFLINE_ID);
+        answerWithGoodParams();
         mActionHandler.openUrl(TEST_URL);
         verifyOpenedOffline(WindowOpenDisposition.CURRENT_TAB);
+        verify(mLoggingBridge, times(1)).onContentTargetVisited(anyLong(), /*isOffline*/ eq(true));
     }
 
     @Test
     @SmallTest
-    public void testOpenUrlOffline() {
+    public void testOpenUrlOnline() {
         when(mOfflineIndicator.getOfflineIdIfPageIsOfflined(TEST_URL)).thenReturn(null);
         mActionHandler.openUrl(TEST_URL);
         verifyOpenedOnline(WindowOpenDisposition.CURRENT_TAB);
+        verify(mLoggingBridge, times(1)).onContentTargetVisited(anyLong(), /*isOffline*/ eq(false));
+    }
+
+    @Test
+    @SmallTest
+    public void testOpenUrlOfflineWithNullParams() {
+        // The indicator will give an offline id, but the model returns a null LoadUrlParams.
+        when(mOfflineIndicator.getOfflineIdIfPageIsOfflined(TEST_URL)).thenReturn(OFFLINE_ID);
+        answerWithGivenParams(null);
+        mActionHandler.openUrl(TEST_URL);
+        verifyOpenedOnline(WindowOpenDisposition.CURRENT_TAB);
+        verify(mLoggingBridge, times(1)).onContentTargetVisited(anyLong(), /*isOffline*/ eq(false));
     }
 
     @Test
@@ -147,34 +199,40 @@ public class FeedActionHandlerTest {
 
     @Test
     @SmallTest
-    public void testOpenUrlInNewTabOnline() {
+    public void testOpenUrlInNewTabOffline() {
         when(mOfflineIndicator.getOfflineIdIfPageIsOfflined(TEST_URL)).thenReturn(OFFLINE_ID);
+        answerWithGoodParams();
         mActionHandler.openUrlInNewTab(TEST_URL);
         verifyOpenedOffline(WindowOpenDisposition.NEW_BACKGROUND_TAB);
+        verify(mLoggingBridge, times(1)).onContentTargetVisited(anyLong(), /*isOffline*/ eq(true));
     }
 
     @Test
     @SmallTest
-    public void testOpenUrlInNewTabOffline() {
+    public void testOpenUrlInNewTabOnline() {
         when(mOfflineIndicator.getOfflineIdIfPageIsOfflined(TEST_URL)).thenReturn(null);
         mActionHandler.openUrlInNewTab(TEST_URL);
         verifyOpenedOnline(WindowOpenDisposition.NEW_BACKGROUND_TAB);
+        verify(mLoggingBridge, times(1)).onContentTargetVisited(anyLong(), /*isOffline*/ eq(false));
     }
 
     @Test
     @SmallTest
-    public void testOpenUrlInNewWindowOnline() {
+    public void testOpenUrlInNewWindowOffline() {
         when(mOfflineIndicator.getOfflineIdIfPageIsOfflined(TEST_URL)).thenReturn(OFFLINE_ID);
+        answerWithGoodParams();
         mActionHandler.openUrlInNewWindow(TEST_URL);
         verifyOpenedOffline(WindowOpenDisposition.NEW_WINDOW);
+        verify(mLoggingBridge, times(1)).onContentTargetVisited(anyLong(), /*isOffline*/ eq(true));
     }
 
     @Test
     @SmallTest
-    public void testOpenUrlInNewWindowOffline() {
+    public void testOpenUrlInNewWindowOnline() {
         when(mOfflineIndicator.getOfflineIdIfPageIsOfflined(TEST_URL)).thenReturn(null);
         mActionHandler.openUrlInNewWindow(TEST_URL);
         verifyOpenedOnline(WindowOpenDisposition.NEW_WINDOW);
+        verify(mLoggingBridge, times(1)).onContentTargetVisited(anyLong(), /*isOffline*/ eq(false));
     }
 
     @Test
@@ -188,4 +246,17 @@ public class FeedActionHandlerTest {
         // and as such the load params should not be updated with the offline id.
         verifyOpenedOnline(WindowOpenDisposition.SAVE_TO_DISK);
     }
+
+    @Test
+    @SmallTest
+    public void testOpenUrlNullTab() {
+        // If openUrl returns null, then the {@link NavigationRecorder} logic should be skipped.
+        reset(mDelegate);
+        when(mDelegate.openUrl(anyInt(), any(LoadUrlParams.class))).thenReturn(null);
+
+        when(mOfflineIndicator.getOfflineIdIfPageIsOfflined(TEST_URL)).thenReturn(null);
+        mActionHandler.openUrl(TEST_URL);
+        verifyOpenedOnline(WindowOpenDisposition.CURRENT_TAB);
+        verify(mLoggingBridge, times(0)).onContentTargetVisited(anyLong(), anyBoolean());
+    }
 }

chrome/browser/android/feed/feed_logging_bridge.cc

@@ -123,17 +123,15 @@ void FeedLoggingBridge::OnOpenedWithNoContent(
 void FeedLoggingBridge::OnContentTargetVisited(
     JNIEnv* j_env,
     const base::android::JavaRef<jobject>& j_this,
-    const jlong visit_time_ms) {
-  feed_logging_metrics_->OnSuggestionArticleVisited(
-      base::TimeDelta::FromMilliseconds(visit_time_ms));
-}
-
-void FeedLoggingBridge::OnOfflinePageVisited(
-    JNIEnv* j_env,
-    const base::android::JavaRef<jobject>& j_this,
-    const jlong visit_time_ms) {
+    const jlong visit_time_ms,
+    const jboolean is_offline) {
+  if (is_offline) {
     feed_logging_metrics_->OnSuggestionOfflinePageVisited(
         base::TimeDelta::FromMilliseconds(visit_time_ms));
+  } else {
+    feed_logging_metrics_->OnSuggestionArticleVisited(
+        base::TimeDelta::FromMilliseconds(visit_time_ms));
+  }
 }
 
 }  // namespace feed

chrome/browser/android/feed/feed_logging_bridge.h

@@ -73,11 +73,8 @@ class FeedLoggingBridge {
 
   void OnContentTargetVisited(JNIEnv* j_env,
                               const base::android::JavaRef<jobject>& j_this,
-                              const jlong visit_time_ms);
-
-  void OnOfflinePageVisited(JNIEnv* j_env,
-                            const base::android::JavaRef<jobject>& j_this,
-                            const jlong visit_time_ms);
+                              const jlong visit_time_ms,
+                              const jboolean is_offline);
 
  private:
   FeedLoggingMetrics* feed_logging_metrics_;