Ntp: use AMP urls for content suggestions when available.

Controlled by the NTPPreferAmpUrls feature, enabled by default.

BUG=668095
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2593573003
Cr-Commit-Position: refs/heads/master@{#439838}

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java

@@ -27,12 +27,9 @@ public class SnippetArticle {
     /** The snippet preview text. */
     public final String mPreviewText;
 
-    /** The URL of this article. */
+    /** The URL of this article. This may be an AMP url. */
     public final String mUrl;
 
-    /** the AMP url for this article (possible for this to be empty). */
-    public final String mAmpUrl;
-
     /** The time when this article was published. */
     public final long mPublishTimestampMilliseconds;
 
@@ -51,7 +48,7 @@ public class SnippetArticle {
     /** Stores whether impression of this article has been tracked already. */
     private boolean mImpressionTracked;
 
-    /** To be run when the offline status of the article or AMP article changes. */
+    /** To be run when the offline status of the article changes. */
     private Runnable mOfflineStatusChangeRunnable;
 
     /** Whether the linked article represents an asset download. */
@@ -73,15 +70,13 @@ public class SnippetArticle {
      * Creates a SnippetArticleListItem object that will hold the data.
      */
     public SnippetArticle(int category, String idWithinCategory, String title, String publisher,
-            String previewText, String url, String ampUrl, long timestamp, float score,
-            int position) {
+            String previewText, String url, long timestamp, float score, int position) {
         mCategory = category;
         mIdWithinCategory = idWithinCategory;
         mTitle = title;
         mPublisher = publisher;
         mPreviewText = previewText;
         mUrl = url;
-        mAmpUrl = ampUrl;
         mPublishTimestampMilliseconds = timestamp;
         mScore = score;
         mPosition = position;

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java

@@ -206,10 +206,10 @@ public class SnippetsBridge implements SuggestionsSource {
     @CalledByNative
     private static SnippetArticle addSuggestion(List<SnippetArticle> suggestions, int category,
             String id, String title, String publisher, String previewText, String url,
-            String ampUrl, long timestamp, float score) {
+            long timestamp, float score) {
         int position = suggestions.size();
-        suggestions.add(new SnippetArticle(category, id, title, publisher, previewText, url, ampUrl,
-                timestamp, score, position));
+        suggestions.add(new SnippetArticle(
+                category, id, title, publisher, previewText, url, timestamp, score, position));
         return suggestions.get(position);
     }
 

chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java

@@ -110,7 +110,7 @@ public class NewTabPageRecyclerViewTest extends ChromeTabbedActivityTestBase {
     @MediumTest
     @Feature({"NewTabPage"})
     @CommandLineFlags.Add("enable-features=NTPSnippets")
-    public void testClickSuggestion() throws InterruptedException, TimeoutException {
+    public void testClickSuggestion() throws InterruptedException {
         setSuggestionsAndWaitForUpdate(10);
         List<SnippetArticle> suggestions =
                 mSource.getSuggestionsForCategory(KnownCategories.ARTICLES);
@@ -268,8 +268,7 @@ public class NewTabPageRecyclerViewTest extends ChromeTabbedActivityTestBase {
         });
     }
 
-    private void setSuggestionsAndWaitForUpdate(final int suggestionsCount)
-            throws InterruptedException, TimeoutException {
+    private void setSuggestionsAndWaitForUpdate(final int suggestionsCount) {
         final FakeSuggestionsSource source = mSource;
 
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
@@ -288,7 +287,7 @@ public class NewTabPageRecyclerViewTest extends ChromeTabbedActivityTestBase {
         for (int i = 0; i < suggestionsCount; i++) {
             String url = mTestServer.getURL(TEST_PAGE) + "#" + i;
             suggestions.add(new SnippetArticle(KnownCategories.ARTICLES, "id" + i, "title" + i,
-                    "publisher" + i, "previewText" + i, url, url, 1466614774 + i, 10f, 0));
+                    "publisher" + i, "previewText" + i, url, 1466614774 + i, 10f, 0));
         }
         return suggestions;
     }

chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java

@@ -135,7 +135,6 @@ public class ArticleSnippetsTest extends ChromeActivityTestCaseBase<ChromeActivi
                 "Publisher",
                 "Preview Text",
                 "www.google.com",
-                "",  // AMP URL
                 1466614774,  // Timestamp
                 10f,  // Score
                 0);  // Position
@@ -149,7 +148,6 @@ public class ArticleSnippetsTest extends ChromeActivityTestCaseBase<ChromeActivi
                 new String(new char[20]).replace("\0", "Publisher "),
                 new String(new char[80]).replace("\0", "Preview Text "),
                 "www.google.com",
-                "",  // AMP URL
                 1466614074,  // Timestamp
                 20f,  // Score
                 1);  // Position
@@ -161,7 +159,6 @@ public class ArticleSnippetsTest extends ChromeActivityTestCaseBase<ChromeActivi
                 "Publisher",
                 "This should not be displayed",
                 "www.google.com",
-                "",  // AMP URL
                 1466614774,  // Timestamp
                 10f,  // Score
                 0);  // Position
@@ -173,7 +170,6 @@ public class ArticleSnippetsTest extends ChromeActivityTestCaseBase<ChromeActivi
                 "Publisher",
                 "This should not be displayed",
                 "www.google.com",
-                "",  // AMP URL
                 1466614774,  // Timestamp
                 10f,  // Score
                 0);  // Position

chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java

@@ -24,7 +24,7 @@ public final class ContentSuggestionsTestUtils {
         for (int index = 0; index < count; index++) {
             suggestions.add(new SnippetArticle(KnownCategories.BOOKMARKS,
                     "https://site.com/url" + index, "title" + index, "pub" + index, "txt" + index,
-                    "https://site.com/url" + index, "https://amp.site.com/url" + index, 0, 0, 0));
+                    "https://site.com/url" + index, 0, 0, 0));
         }
         return suggestions;
     }

chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java

@@ -211,8 +211,8 @@ public class NewTabPageAdapterTest {
 
         // The adapter should ignore any new incoming data.
         mSource.setSuggestionsForCategory(KnownCategories.ARTICLES,
-                Arrays.asList(new SnippetArticle[] {new SnippetArticle(0, "foo", "title1", "pub1",
-                        "txt1", "foo", "bar", 0, 0, 0)}));
+                Arrays.asList(new SnippetArticle[] {
+                        new SnippetArticle(0, "foo", "title1", "pub1", "txt1", "url", 0, 0, 0)}));
 
         assertItemsFor(section(numSuggestions));
     }
@@ -239,8 +239,8 @@ public class NewTabPageAdapterTest {
 
         // The adapter should ignore any new incoming data.
         mSource.setSuggestionsForCategory(KnownCategories.ARTICLES,
-                Arrays.asList(new SnippetArticle[] {new SnippetArticle(0, "foo", "title1", "pub1",
-                        "txt1", "foo", "bar", 0, 0, 0)}));
+                Arrays.asList(new SnippetArticle[] {
+                        new SnippetArticle(0, "foo", "title1", "pub1", "txt1", "url", 0, 0, 0)}));
         assertItemsFor(section(numSuggestions));
     }
 
@@ -288,7 +288,7 @@ public class NewTabPageAdapterTest {
         // If we have snippets, we should not load the new list (i.e. the extra item does *not*
         // appear).
         suggestions.add(new SnippetArticle(0, "https://site.com/url1", "title1", "pub1", "txt1",
-                "https://site.com/url1", "https://amp.site.com/url1", 0, 0, 0));
+                "https://site.com/url1", 0, 0, 0));
         mSource.setSuggestionsForCategory(KnownCategories.ARTICLES, suggestions);
         assertItemsFor(section(3));
 

chrome/browser/android/ntp/ntp_snippets_bridge.cc

@@ -64,7 +64,6 @@ ScopedJavaLocalRef<jobject> ToJavaSuggestionList(
             ConvertUTF16ToJavaString(env, suggestion.publisher_name()),
             ConvertUTF16ToJavaString(env, suggestion.snippet_text()),
             ConvertUTF8ToJavaString(env, suggestion.url().spec()),
-            ConvertUTF8ToJavaString(env, suggestion.amp_url().spec()),
             suggestion.publish_date().ToJavaTime(), suggestion.score());
     if (suggestion.id().category().IsKnownCategory(
             KnownCategories::DOWNLOADS) &&

chrome/browser/resources/snippets_internals.html

@@ -140,11 +140,6 @@ found in the LICENSE file.
                 <tr>
                   <td>URL
                   <td><a class="url" jsvalues="href:url" jscontent="url"></a>
-                <tr>
-                  <td>AMP URL
-                  <td>
-                    <a class="amp-url" jsvalues="href:ampUrl"
-                        jscontent="ampUrl"></a>
                 <tr>
                   <td>Snippet text
                   <td jscontent="snippetText">
@@ -179,11 +174,6 @@ found in the LICENSE file.
                 <tr>
                   <td>URL
                   <td><a class="url" jsvalues="href:url" jscontent="url"></a>
-                <tr>
-                  <td>AMP URL
-                  <td>
-                    <a class="amp-url" jsvalues="href:ampUrl"
-                        jscontent="ampUrl"></a>
                 <tr>
                   <td>Snippet text
                   <td jscontent="snippetText">

chrome/browser/ui/webui/snippets_internals_message_handler.cc

@@ -48,7 +48,6 @@ std::unique_ptr<base::DictionaryValue> PrepareSuggestion(
   auto entry = base::MakeUnique<base::DictionaryValue>();
   entry->SetString("idWithinCategory", suggestion.id().id_within_category());
   entry->SetString("url", suggestion.url().spec());
-  entry->SetString("ampUrl", suggestion.amp_url().spec());
   entry->SetString("title", suggestion.title());
   entry->SetString("snippetText", suggestion.snippet_text());
   entry->SetString("publishDate",

components/ntp_snippets/content_suggestion.h

@@ -82,15 +82,10 @@ class ContentSuggestion {
   // An ID for identifying the suggestion. The ID is unique application-wide.
   const ID& id() const { return id_; }
 
-  // The normal content URL where the content referenced by the suggestion can
-  // be accessed.
+  // The URL where the content referenced by the suggestion can be accessed.
+  // This may be an AMP URL.
   const GURL& url() const { return url_; }
 
-  // If available, this contains an URL to an AMP version of the same content.
-  // Otherwise, this is an empty GURL().
-  const GURL& amp_url() const { return amp_url_; }
-  void set_amp_url(const GURL& amp_url) { amp_url_ = amp_url; }
-
   // Title of the suggestion.
   const base::string16& title() const { return title_; }
   void set_title(const base::string16& title) { title_ = title; }
@@ -141,7 +136,6 @@ class ContentSuggestion {
  private:
   ID id_;
   GURL url_;
-  GURL amp_url_;
   base::string16 title_;
   base::string16 snippet_text_;
   base::Time publish_date_;

components/ntp_snippets/features.cc

@@ -41,4 +41,7 @@ const base::Feature kForeignSessionsSuggestionsFeature{
 const base::Feature kFetchMoreFeature{"NTPSuggestionsFetchMore",
                                       base::FEATURE_ENABLED_BY_DEFAULT};
 
+const base::Feature kPreferAmpUrlsFeature{"NTPPreferAmpUrls",
+                                          base::FEATURE_ENABLED_BY_DEFAULT};
+
 }  // namespace ntp_snippets

components/ntp_snippets/features.h

@@ -38,6 +38,9 @@ extern const base::Feature kIncreasedVisibility;
 // Feature to enable the Fetch More action
 extern const base::Feature kFetchMoreFeature;
 
+// Feature to prefer AMP URLs over regular URLs when available.
+extern const base::Feature kPreferAmpUrlsFeature;
+
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_FEATURES_H_

components/ntp_snippets/remote/ntp_snippet.h

@@ -128,7 +128,10 @@ class NTPSnippet {
   std::string title_;
   GURL url_;
   std::string publisher_name_;
+
+  // TODO(mvanouwerkerk): Remove this field and its uses, just use |url_|.
   GURL amp_url_;
+
   GURL salient_image_url_;
   std::string snippet_;
   base::Time publish_date_;

components/ntp_snippets/remote/remote_suggestions_provider.cc

@@ -200,8 +200,12 @@ std::vector<ContentSuggestion> ConvertToContentSuggestions(
     if (!snippet->is_complete()) {
       continue;
     }
-    ContentSuggestion suggestion(category, snippet->id(), snippet->url());
-    suggestion.set_amp_url(snippet->amp_url());
+    GURL url = snippet->url();
+    if (base::FeatureList::IsEnabled(kPreferAmpUrlsFeature) &&
+        !snippet->amp_url().is_empty()) {
+      url = snippet->amp_url();
+    }
+    ContentSuggestion suggestion(category, snippet->id(), url);
     suggestion.set_title(base::UTF8ToUTF16(snippet->title()));
     suggestion.set_snippet_text(base::UTF8ToUTF16(snippet->snippet()));
     suggestion.set_publish_date(snippet->publish_date());

components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc

@@ -734,7 +734,6 @@ TEST_F(RemoteSuggestionsProviderTest, Full) {
   EXPECT_EQ(GetDefaultCreationTime(), suggestion.publish_date());
   EXPECT_EQ(kSnippetPublisherName,
             base::UTF16ToUTF8(suggestion.publisher_name()));
-  EXPECT_EQ(GURL(kSnippetAmpUrl), suggestion.amp_url());
 }
 
 TEST_F(RemoteSuggestionsProviderTest, CategoryTitle) {
@@ -804,7 +803,6 @@ TEST_F(RemoteSuggestionsProviderTest, MultipleCategories) {
     EXPECT_EQ(GetDefaultCreationTime(), suggestion.publish_date());
     EXPECT_EQ(kSnippetPublisherName,
               base::UTF16ToUTF8(suggestion.publisher_name()));
-    EXPECT_EQ(GURL(kSnippetAmpUrl), suggestion.amp_url());
   }
 
   {
@@ -816,7 +814,6 @@ TEST_F(RemoteSuggestionsProviderTest, MultipleCategories) {
     EXPECT_EQ(GetDefaultCreationTime(), suggestion.publish_date());
     EXPECT_EQ(kSnippetPublisherName,
               base::UTF16ToUTF8(suggestion.publisher_name()));
-    EXPECT_EQ(GURL(kSnippetAmpUrl), suggestion.amp_url());
   }
 }