Query Tiles: UI fixes

This CL fixes
1 - If we get redundant calls to the TileSuggestionProcessor, ignore them
and don't update the underlying widget. This will prevent the previous
suggestion from flashing up momementarily before new tile set is loaded.
2 - Default ItemAnimator is removed to prevent the jank
3 - Only reload the tile if NTP is reloaded via home button. For regular
page load, don't animate query tiles (fixes submit query flow jank)
4 - Fix for international languages - ASCII comparison is not enough
for non-english languages. string16 is used for comparison purposes,
which fixes the omnibox flow for query tiles (Bug 1086672)

Bug : 1086672, 1081557

Change-Id: I8536759fb3f7a9c9c00ddb56f5c12f40df26a114
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2211661
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772080}

chrome/android/BUILD.gn

@@ -3077,6 +3077,7 @@ native_java_unittests_tests = [
   "native_java_unittests/src/org/chromium/chrome/browser/omnibox/suggestions/answer/AnswerSuggestionProcessorUnitTest.java",
   "native_java_unittests/src/org/chromium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionUnitTest.java",
   "native_java_unittests/src/org/chromium/chrome/browser/omnibox/suggestions/entity/EntitySuggestionProcessorUnitTest.java",
+  "native_java_unittests/src/org/chromium/chrome/browser/omnibox/suggestions/tiles/TileSuggestionProcessorUnitTest.java",
   "native_java_unittests/src/org/chromium/chrome/browser/omnibox/suggestions/CachedZeroSuggestionsManagerUnitTest.java",
   "native_java_unittests/src/org/chromium/chrome/browser/partnercustomizations/PartnerBrowserCustomizationsUnitTest.java",
   "native_java_unittests/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderUnitTest.java",
@@ -3111,6 +3112,7 @@ android_library("native_java_unittests_java") {
     "//components/omnibox/browser:browser_java",
     "//components/payments/content/android:java",
     "//components/payments/mojom:mojom_java",
+    "//components/query_tiles:java",
     "//components/search_engines/android:java",
     "//components/security_state/content/android:java",
     "//components/security_state/core:security_state_enums_java",

chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java

@@ -346,7 +346,7 @@ public class NewTabPage implements NativePage, InvalidationAwareThumbnailProvide
 
             @Override
             public void onLoadUrl(Tab tab, LoadUrlParams params, int loadType) {
-                mNewTabPageLayout.onLoadUrl();
+                mNewTabPageLayout.onLoadUrl(isNTPUrl(tab.getUrl()));
             }
         };
         mTab.addObserver(mTabObserver);

chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java

@@ -605,8 +605,8 @@ public class NewTabPageLayout extends LinearLayout implements TileGroup.Observer
         if (mQueryTileSection != null) mQueryTileSection.onUrlFocusAnimationChanged(percent);
     }
 
-    void onLoadUrl() {
-        if (mQueryTileSection != null) mQueryTileSection.reloadTiles();
+    void onLoadUrl(boolean isNtpUrl) {
+        if (isNtpUrl && mQueryTileSection != null) mQueryTileSection.reloadTiles();
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/omnibox/suggestions/tiles/TileSuggestionProcessor.java

@@ -14,10 +14,12 @@ import org.chromium.chrome.browser.omnibox.suggestions.SuggestionProcessor;
 import org.chromium.components.query_tiles.QueryTile;
 import org.chromium.ui.modelutil.PropertyModel;
 
+import java.util.ArrayList;
 import java.util.List;
 
 /** A class that handles model and view creation for the query tile suggestions. */
 public class TileSuggestionProcessor implements SuggestionProcessor {
+    private List<QueryTile> mLastProcessedTiles;
     private final Callback<List<QueryTile>> mQueryTileSuggestionCallback;
     private final int mMinViewHeight;
 
@@ -49,7 +51,13 @@ public class TileSuggestionProcessor implements SuggestionProcessor {
     }
 
     @Override
-    public void onUrlFocusChange(boolean hasFocus) {}
+    public void onUrlFocusChange(boolean hasFocus) {
+        if (hasFocus) return;
+
+        // Clear out any old UI state for this processor.
+        mLastProcessedTiles = null;
+        mQueryTileSuggestionCallback.onResult(new ArrayList<>());
+    }
 
     @Override
     public void onNativeInitialized() {}
@@ -61,7 +69,11 @@ public class TileSuggestionProcessor implements SuggestionProcessor {
 
     @Override
     public void populateModel(OmniboxSuggestion suggestion, PropertyModel model, int position) {
-        mQueryTileSuggestionCallback.onResult(suggestion.getQueryTiles());
+        List<QueryTile> tiles = suggestion.getQueryTiles();
+
+        if (mLastProcessedTiles != null && mLastProcessedTiles.equals(tiles)) return;
+        mLastProcessedTiles = new ArrayList<>(tiles);
+        mQueryTileSuggestionCallback.onResult(tiles);
     }
 
     @Override

chrome/android/java/src/org/chromium/chrome/browser/query_tiles/QueryTileSection.java

@@ -200,6 +200,6 @@ public class QueryTileSection {
         return ChromeFeatureList.getFieldTrialParamByFeatureAsInt(ChromeFeatureList.QUERY_TILES,
                 isSmallScreen ? MOST_VISITED_MAX_ROWS_SMALL_SCREEN
                               : MOST_VISITED_MAX_ROWS_NORMAL_SCREEN,
-                2);
+                1);
     }
 }

chrome/android/native_java_unittests/src/org/chromium/chrome/browser/omnibox/suggestions/tiles/TileSuggestionProcessorUnitTest.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.omnibox.suggestions.tiles;
+
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import android.content.Context;
+import android.content.res.Resources;
+
+import org.junit.Assert;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import org.chromium.base.Callback;
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.CalledByNativeJavaTest;
+import org.chromium.chrome.browser.omnibox.suggestions.OmniboxSuggestion;
+import org.chromium.components.query_tiles.QueryTile;
+import org.chromium.ui.modelutil.PropertyModel;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * Unit tests for the "query tiles" omnibox suggestion.
+ */
+public final class TileSuggestionProcessorUnitTest {
+    private PropertyModel mModel;
+    private TileSuggestionProcessor mProcessor;
+
+    @Mock
+    Context mContext;
+
+    @Mock
+    Resources mResources;
+
+    @Mock
+    Callback<List<QueryTile>> mSuggestionCallback;
+
+    @Mock
+    OmniboxSuggestion mSuggestion;
+
+    @CalledByNative
+    private TileSuggestionProcessorUnitTest() {}
+
+    @CalledByNative
+    public void setUp() {
+        MockitoAnnotations.initMocks(this);
+        when(mContext.getResources()).thenReturn(mResources);
+
+        mModel = new PropertyModel();
+        mProcessor = new TileSuggestionProcessor(mContext, mSuggestionCallback);
+    }
+
+    @CalledByNativeJavaTest
+    public void testDuplicatesDoNotPropagateToUi() {
+        List<QueryTile> tiles = Arrays.asList(buildTile("1"), buildTile("2"));
+        List<QueryTile> tilesSameRef = new ArrayList<>(tiles);
+        List<QueryTile> tilesSameVal = Arrays.asList(buildTile("1"), buildTile("2"));
+        List<QueryTile> tilesDifferent = Arrays.asList(buildTile("3"));
+
+        when(mSuggestion.getQueryTiles())
+                .thenReturn(tiles, tilesSameRef, tilesSameVal, tilesDifferent, tilesDifferent);
+
+        mProcessor.populateModel(mSuggestion, mModel, 0);
+        mProcessor.populateModel(mSuggestion, mModel, 0);
+        mProcessor.populateModel(mSuggestion, mModel, 0);
+        mProcessor.populateModel(mSuggestion, mModel, 0);
+
+        mProcessor.onUrlFocusChange(false);
+        mProcessor.populateModel(mSuggestion, mModel, 0);
+
+        ArgumentCaptor<List<QueryTile>> captor = ArgumentCaptor.forClass(List.class);
+        verify(mSuggestionCallback, times(4)).onResult(captor.capture());
+
+        List<List<QueryTile>> results = captor.getAllValues();
+        Assert.assertEquals(tiles, results.get(0));
+        Assert.assertEquals(tilesDifferent, results.get(1));
+        Assert.assertEquals(new ArrayList<>(), results.get(2));
+        Assert.assertEquals(tilesDifferent, results.get(3));
+    }
+
+    private static QueryTile buildTile(String suffix) {
+        return new QueryTile("id" + suffix, "displayTitle" + suffix, "accessibilityText" + suffix,
+                "queryText" + suffix, new String[] {"url" + suffix}, new String[] {},
+                new ArrayList<>());
+    }
+}

chrome/browser/omnibox/suggestions/tiles/tile_suggestion_processor_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/android/jni_android.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/android/native_j_unittests_jni_headers/TileSuggestionProcessorUnitTest_jni.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::android::AttachCurrentThread;
+
+class TileSuggestionProcessorUnitTest : public ::testing::Test {
+ public:
+  TileSuggestionProcessorUnitTest()
+      : j_test_(Java_TileSuggestionProcessorUnitTest_Constructor(
+            AttachCurrentThread())) {}
+
+  void SetUp() override {
+    Java_TileSuggestionProcessorUnitTest_setUp(AttachCurrentThread(), j_test_);
+  }
+
+  const base::android::ScopedJavaGlobalRef<jobject>& j_test() {
+    return j_test_;
+  }
+
+ private:
+  base::android::ScopedJavaGlobalRef<jobject> j_test_;
+};
+
+JAVA_TESTS(TileSuggestionProcessorUnitTest, j_test())

chrome/test/BUILD.gn

@@ -3859,6 +3859,7 @@ test("unit_tests") {
       "../browser/omnibox/suggestions/clipboard/clipboard_suggestion_processor_unittest.cc",
       "../browser/omnibox/suggestions/editurl/edit_url_suggestions_unittest.cc",
       "../browser/omnibox/suggestions/entity/entity_suggestions_processor_unittest.cc",
+      "../browser/omnibox/suggestions/tiles/tile_suggestion_processor_unittest.cc",
       "../browser/optimization_guide/android/optimization_guide_bridge_unittest.cc",
       "../browser/page_load_metrics/observers/android_page_load_metrics_observer_unittest.cc",
       "../browser/partnercustomizations/partner_browser_customizations_unittest.cc",

components/browser_ui/widget/android/java/src/org/chromium/components/browser_ui/widget/image_tiles/ImageTile.java

@@ -4,6 +4,10 @@
 
 package org.chromium.components.browser_ui.widget.image_tiles;
 
+import android.text.TextUtils;
+
+import androidx.annotation.Nullable;
+
 /**
  * Class encapsulating data needed to render a image tile. An {@link ImageTile} is a tile meant to
  * show an image with some text.
@@ -24,4 +28,12 @@ public class ImageTile {
         this.displayTitle = displayTitle;
         this.accessibilityText = accessibilityText;
     }
+
+    @Override
+    public boolean equals(@Nullable Object obj) {
+        if (!(obj instanceof ImageTile)) return false;
+        ImageTile other = (ImageTile) obj;
+        return TextUtils.equals(id, other.id) && TextUtils.equals(displayTitle, other.displayTitle)
+                && TextUtils.equals(accessibilityText, other.accessibilityText);
+    }
 }

components/browser_ui/widget/android/java/src/org/chromium/components/browser_ui/widget/image_tiles/TileListView.java

@@ -53,6 +53,7 @@ class TileListView {
         mLayoutManager = new LinearLayoutManager(context, LinearLayoutManager.HORIZONTAL, false);
         mView.setLayoutManager(mLayoutManager);
         mView.addItemDecoration(new ItemDecorationImpl(context));
+        mView.setItemAnimator(null);
         mView.setLayoutAnimation(
                 AnimationUtils.loadLayoutAnimation(context, R.anim.image_grid_enter));
 

components/omnibox/browser/query_tile_provider.cc

@@ -23,10 +23,10 @@ bool TextMatchesQueryTile(base::string16 input_text,
                           base::Optional<query_tiles::Tile> tile) {
   auto trimmed_input =
       base::TrimWhitespace(input_text, base::TrimPositions::TRIM_TRAILING);
-  auto tile_text = tile.has_value() ? tile->query_text : "";
+  auto tile_text = base::UTF8ToUTF16(tile.has_value() ? tile->query_text : "");
   auto trimmed_tile_text =
-      base::TrimWhitespaceASCII(tile_text, base::TrimPositions::TRIM_TRAILING);
-  return base::EqualsASCII(trimmed_input, trimmed_tile_text);
+      base::TrimWhitespace(tile_text, base::TrimPositions::TRIM_TRAILING);
+  return trimmed_input == trimmed_tile_text;
 }
 
 // Helper function to determine if we are currently showing an URL and the

components/query_tiles/android/java/src/org/chromium/components/query_tiles/QueryTile.java

@@ -4,6 +4,10 @@
 
 package org.chromium.components.query_tiles;
 
+import android.text.TextUtils;
+
+import androidx.annotation.Nullable;
+
 import org.chromium.components.browser_ui.widget.image_tiles.ImageTile;
 
 import java.util.ArrayList;
@@ -38,4 +42,22 @@ public class QueryTile extends ImageTile {
         this.children =
                 Collections.unmodifiableList(children == null ? new ArrayList<>() : children);
     }
+
+    @Override
+    public boolean equals(@Nullable Object obj) {
+        if (!super.equals(obj)) return false;
+        if (!(obj instanceof QueryTile)) return false;
+
+        QueryTile other = (QueryTile) obj;
+
+        if (!TextUtils.equals(queryText, other.queryText)) return false;
+
+        if (children != null && !children.equals(other.children)) return false;
+        if (children == null && other.children != null) return false;
+
+        if (urls != null && !urls.equals(other.urls)) return false;
+        if (urls == null && other.urls != null) return false;
+
+        return true;
+    }
 }