[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}diff --git a/chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedLoggingBridge.java b/chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedLoggingBridge.java
index 76d304f..97cbe7c 100644
--- a/chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedLoggingBridge.java
+++ b/chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedLoggingBridge.java
@@ -109,27 +109,14 @@
* 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 @@
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);
}
diff --git a/chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/action/FeedActionHandler.java b/chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/action/FeedActionHandler.java
index 1776ee8..9621112 100644
--- a/chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/action/FeedActionHandler.java
+++ b/chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/action/FeedActionHandler.java
@@ -140,26 +140,38 @@
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) -> {
- 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);
- });
+ 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());
+ 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();
}
}
diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/feed/action/FeedActionHandlerTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/feed/action/FeedActionHandlerTest.java
index d28b9c0..d5823a20 100644
--- a/chrome/android/junit/src/org/chromium/chrome/browser/feed/action/FeedActionHandlerTest.java
+++ b/chrome/android/junit/src/org/chromium/chrome/browser/feed/action/FeedActionHandlerTest.java
@@ -6,10 +6,13 @@
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.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.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 @@
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 @@
@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 @@
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 @@
@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 @@
@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);
- }
-
- @Test
- @SmallTest
- public void testOpenUrlInNewWindowOnline() {
- when(mOfflineIndicator.getOfflineIdIfPageIsOfflined(TEST_URL)).thenReturn(OFFLINE_ID);
- mActionHandler.openUrlInNewWindow(TEST_URL);
- verifyOpenedOffline(WindowOpenDisposition.NEW_WINDOW);
+ verify(mLoggingBridge, times(1)).onContentTargetVisited(anyLong(), /*isOffline*/ eq(false));
}
@Test
@SmallTest
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 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 @@
// 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());
+ }
}
diff --git a/chrome/browser/android/feed/feed_logging_bridge.cc b/chrome/browser/android/feed/feed_logging_bridge.cc
index 22f8430..33934e6 100644
--- a/chrome/browser/android/feed/feed_logging_bridge.cc
+++ b/chrome/browser/android/feed/feed_logging_bridge.cc
@@ -123,17 +123,15 @@
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) {
- feed_logging_metrics_->OnSuggestionOfflinePageVisited(
- base::TimeDelta::FromMilliseconds(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
diff --git a/chrome/browser/android/feed/feed_logging_bridge.h b/chrome/browser/android/feed/feed_logging_bridge.h
index f0ee72f4..a67e794e 100644
--- a/chrome/browser/android/feed/feed_logging_bridge.h
+++ b/chrome/browser/android/feed/feed_logging_bridge.h
@@ -73,11 +73,8 @@
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_;
