[EoC] Use canonical url for fetching suggestions

This will use the canonical URL specified in the <link rel="canonical">
element when it exists instead of the actual URL. Most notably this gets the
canonical URL for AMP pages. Since this means there are potentially two separate
URLs per page, we add a canonical URL to the readiness state object.

Bug: 844123
Change-Id: Ib5e99fdd284006a7402136d78ea89cca31181f32
Reviewed-on: https://chromium-review.googlesource.com/1067679
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561991}

chrome/android/java/src/org/chromium/chrome/browser/contextual_suggestions/FetchHelper.java

@@ -7,8 +7,10 @@ package org.chromium.chrome.browser.contextual_suggestions;
 import android.os.SystemClock;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
+import android.text.TextUtils;
 import android.webkit.URLUtil;
 
+import org.chromium.base.Callback;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.ChromeFeatureList;
@@ -57,6 +59,7 @@ class FetchHelper {
         private long mFetchTimeBaselineMillis;
         private String mUrl;
         private boolean mSuggestionsDismissed;
+        private String mCanonicalUrl;
 
         TabFetchReadinessState(String url) {
             updateUrl(url);
@@ -69,6 +72,7 @@ class FetchHelper {
          */
         void updateUrl(String url) {
             mUrl = URLUtil.isNetworkUrl(url) ? url : null;
+            mCanonicalUrl = "";
             mFetchTimeBaselineMillis = 0;
             setSuggestionsDismissed(false);
         }
@@ -78,6 +82,12 @@ class FetchHelper {
             return mUrl;
         }
 
+        /** Set the canonical url, which can differ from the actual URL. If the canonical url is
+         * set, the readiness state is considered to be tracking both urls. */
+        void setCanonicalUrl(String canonicalUrl) {
+            mCanonicalUrl = canonicalUrl;
+        }
+
         /**
          * @return Whether the tab state is tracking a tab with valid page loaded and valid status
          *         for fetching.
@@ -121,7 +131,8 @@ class FetchHelper {
          * @return Whether the URLs can be considered the same.
          */
         boolean isContextTheSame(String url) {
-            return UrlUtilities.urlsMatchIgnoringFragments(url, mUrl);
+            return UrlUtilities.urlsMatchIgnoringFragments(url, mUrl)
+                    || UrlUtilities.urlsMatchIgnoringFragments(url, mCanonicalUrl);
         }
     }
 
@@ -280,14 +291,40 @@ class FetchHelper {
         return false;
     }
 
-    private void maybeStartFetch(Tab tab) {
+    private void maybeStartFetch(final Tab tab) {
         if (tab == null || tab != mCurrentTab) return;
 
-        assert !tab.isIncognito();
-
         // Skip additional requests for current tab, until clearState is called.
         if (mFetchRequestedForCurrentTab) return;
 
+        if (shouldFetchCanonicalUrl(tab)) {
+            mFetchRequestedForCurrentTab = true;
+            tab.getWebContents().getMainFrame().getCanonicalUrlForSharing(new Callback<String>() {
+                @Override
+                public void onResult(String result) {
+                    // Reset mFetchRequestedForCurrentTab so that it remains false if
+                    // maybeStartFetchWithCanonicalURLResolved doesn't end up requesting
+                    // suggestions. If it does end up requesting suggestions,
+                    // mFetchRequestedForCurrentTab will be set back to true in the same synchronous
+                    // flow of execution, so there shouldn't be a race condition.
+                    mFetchRequestedForCurrentTab = false;
+                    TabFetchReadinessState tabFetchReadinessState = getTabFetchReadinessState(tab);
+                    tabFetchReadinessState.setCanonicalUrl(result);
+                    maybeStartFetchWithCanonicalURLResolved(
+                            tab, getUrlToFetchFor(tab.getUrl(), result));
+                }
+            });
+            return;
+        } else {
+            maybeStartFetchWithCanonicalURLResolved(tab, tab.getUrl());
+        }
+    }
+
+    private void maybeStartFetchWithCanonicalURLResolved(Tab tab, String resolvedUrl) {
+        if (tab == null || tab != mCurrentTab) return;
+
+        assert !tab.isIncognito();
+
         TabFetchReadinessState tabFetchReadinessState = getTabFetchReadinessState(mCurrentTab);
 
         // If we are not tracking a valid page, we can bail.
@@ -306,17 +343,16 @@ class FetchHelper {
             return;
         }
 
-        String url = tabFetchReadinessState.getUrl();
         long remainingFetchDelayMillis =
                 SystemClock.uptimeMillis() - tabFetchReadinessState.getFetchTimeBaselineMillis();
         if (!sDisableDelayForTesting && remainingFetchDelayMillis < MINIMUM_FETCH_DELAY_MILLIS) {
-            postDelayedFetch(
-                    url, mCurrentTab, MINIMUM_FETCH_DELAY_MILLIS - remainingFetchDelayMillis);
+            postDelayedFetch(resolvedUrl, mCurrentTab,
+                    MINIMUM_FETCH_DELAY_MILLIS - remainingFetchDelayMillis);
             return;
         }
 
         mFetchRequestedForCurrentTab = true;
-        mDelegate.requestSuggestions(url);
+        mDelegate.requestSuggestions(resolvedUrl);
     }
 
     private void postDelayedFetch(final String url, final Tab tab, long delayMillis) {
@@ -475,6 +511,23 @@ class FetchHelper {
         return false;
     }
 
+    static boolean shouldFetchCanonicalUrl(final Tab currentTab) {
+        WebContents webContents = currentTab.getWebContents();
+        if (webContents == null) return false;
+        if (webContents.getMainFrame() == null) return false;
+        String url = currentTab.getUrl();
+        if (TextUtils.isEmpty(url)) return false;
+        if (currentTab.isShowingErrorPage() || currentTab.isShowingInterstitialPage()
+                || currentTab.isShowingSadTab()) {
+            return false;
+        }
+        return true;
+    }
+
+    static String getUrlToFetchFor(String visibleUrl, String canonicalUrl) {
+        return TextUtils.isEmpty(canonicalUrl) ? visibleUrl : canonicalUrl;
+    }
+
     @VisibleForTesting
     static void setDisableDelayForTesting(boolean disable) {
         sDisableDelayForTesting = disable;

chrome/android/javatests/src/org/chromium/chrome/browser/contextual_suggestions/ContextualSuggestionsTest.java

@@ -110,6 +110,7 @@ public class ContextualSuggestionsTest {
 
     @Rule
     public MethodRule mMethodParamAnnotationProcessor = new MethodParamAnnotationRule();
+    /** Parameter provider for the Slim Peek UI */
     public static class SlimPeekUIParams implements ParameterProvider {
         @Override
         public Iterable<ParameterSet> getParameters() {
@@ -158,6 +159,7 @@ public class ContextualSuggestionsTest {
 
         mTestServer = EmbeddedTestServer.createAndStartServer(InstrumentationRegistry.getContext());
         mActivityTestRule.startMainActivityWithURL(mTestServer.getURL(TEST_PAGE));
+        final CallbackHelper waitForSuggestionsHelper = new CallbackHelper();
 
         ThreadUtils.runOnUiThreadBlocking(() -> {
             mCoordinator =
@@ -171,9 +173,13 @@ public class ContextualSuggestionsTest {
                         && mModel.getToolbarShadowVisibility()) {
                     mToolbarShadowVisibleCallback.notifyCalled();
                 }
+                if (propertyKey == PropertyKey.TITLE && mModel.getTitle() != null) {
+                    waitForSuggestionsHelper.notifyCalled();
+                }
             });
         });
 
+        waitForSuggestionsHelper.waitForCallback(0);
         mBottomSheet = mActivityTestRule.getActivity().getBottomSheet();
     }
 

chrome/android/junit/src/org/chromium/chrome/browser/contextual_suggestions/FetchHelperTest.java

@@ -9,6 +9,7 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -20,9 +21,12 @@ import org.mockito.ArgumentCaptor;
 import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
 import org.robolectric.annotation.Config;
 import org.robolectric.shadows.ShadowLooper;
 
+import org.chromium.base.Callback;
 import org.chromium.base.test.BaseRobolectricTestRunner;
 import org.chromium.chrome.browser.contextual_suggestions.FetchHelper.Delegate;
 import org.chromium.chrome.browser.tab.Tab;
@@ -33,6 +37,7 @@ import org.chromium.chrome.browser.tabmodel.TabModel.TabSelectionType;
 import org.chromium.chrome.browser.tabmodel.TabModelObserver;
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
 import org.chromium.chrome.browser.util.test.ShadowUrlUtilities;
+import org.chromium.content_public.browser.RenderFrameHost;
 import org.chromium.content_public.browser.WebContents;
 
 import java.util.ArrayList;
@@ -43,8 +48,8 @@ import java.util.function.Consumer;
 @RunWith(BaseRobolectricTestRunner.class)
 @Config(manifest = Config.NONE, shadows = {ShadowUrlUtilities.class})
 public final class FetchHelperTest {
-    private static final String STARTING_URL = "http://starting.url";
-    private static final String DIFFERENT_URL = "http://different.url";
+    private static final String STARTING_URL = "https://starting.url";
+    private static final String DIFFERENT_URL = "https://different.url";
 
     private class TestFetchHelper extends FetchHelper {
         public TestFetchHelper(Delegate delegate, TabModelSelector tabModelSelector) {
@@ -71,6 +76,8 @@ public final class FetchHelperTest {
     @Mock
     private WebContents mWebContents;
     @Mock
+    private RenderFrameHost mFrameHost;
+    @Mock
     private Tab mTab2;
     @Mock
     private WebContents mWebContents2;
@@ -387,6 +394,30 @@ public final class FetchHelperTest {
         verify(mDelegate, times(1)).requestSuggestions(eq(STARTING_URL));
     }
 
+    @Test
+    public void canonicalUrl_isResolved() {
+        doReturn(false).when(mTab).isLoading();
+        doReturn(mFrameHost).when(mWebContents).getMainFrame();
+        doAnswer(new Answer<Void>() {
+            @Override
+            public Void answer(InvocationOnMock invocation) {
+                if (invocation.getArguments()[0] instanceof Callback) {
+                    @SuppressWarnings("unchecked")
+                    Callback<String> callback = (Callback<String>) invocation.getArguments()[0];
+                    callback.onResult(DIFFERENT_URL);
+                }
+                return null;
+            }
+        })
+                .when(mFrameHost)
+                .getCanonicalUrlForSharing(any());
+        FetchHelper helper = createFetchHelper();
+
+        getTabObserver().onPageLoadFinished(mTab);
+        runUntilFetchPossible();
+        verify(mDelegate, times(1)).requestSuggestions(DIFFERENT_URL);
+    }
+
     private void addTab(Tab tab) {
         getTabModelObserver().didAddTab(tab, TabLaunchType.FROM_LINK);
     }