[Omnibox] Fix for Ripple effect race condition on action buttons

Theme leaking fix introduced a race condition manifesting in background
drawable not always being set on suggestion action buttons.

This patch updates the method responsible for adding/binding action
buttons to also include setting the background.

Second modification of the code prefer copying of drawable over
retrieval, which in local testing is significantly better than
getting a resource, especially in incognito, where additional theme
leaking code causes long wait on the first retrieved resource.

In normal case copy is about 2 times faster.
In incognito case it is up to 10 times faster than subsequent
retrievals.

Bug: 1113542
Change-Id: Ic2d09dc0bec4696cec78601b93a30d0fd1b8fd5d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2341403Reviewed-by: Tomasz Wiszkowski <ender@google.com>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Filip Gorski <fgorski@chromium.org>
Commit-Queue: Tomasz Wiszkowski <ender@google.com>
Cr-Commit-Position: refs/heads/master@{#795703}

chrome/android/java/src/org/chromium/chrome/browser/omnibox/suggestions/base/BaseSuggestionViewBinder.java

@@ -72,6 +72,9 @@ public final class BaseSuggestionViewBinder<T extends View>
         final int actionCount = actions != null ? actions.size() : 0;
         view.setActionButtonsCount(actionCount);
 
+        // Drawable retrieved once here (expensive) and will be copied multiple times (cheap).
+        Drawable backgroundDrawable = getSelectableBackgroundDrawable(view, model);
+
         final List<ImageView> actionViews = view.getActionButtons();
         for (int index = 0; index < actionCount; index++) {
             final ImageView actionView = actionViews.get(index);
@@ -79,6 +82,7 @@ public final class BaseSuggestionViewBinder<T extends View>
             actionView.setOnClickListener(v -> action.callback.run());
             actionView.setContentDescription(
                     view.getContext().getResources().getString(action.accessibilityDescription));
+            actionView.setBackground(copyDrawable(backgroundDrawable));
             updateIcon(actionView, action.icon,
                     ChromeColors.getPrimaryIconTintRes(!useDarkColors(model)));
         }
@@ -88,10 +92,8 @@ public final class BaseSuggestionViewBinder<T extends View>
     private static <T extends View> void updateColorScheme(
             PropertyModel model, BaseSuggestionView<T> view) {
         updateSuggestionIcon(model, view);
-        Drawable selectableBackgroundDrawable = OmniboxResourceProvider.resolveAttributeToDrawable(
-                view.getContext(), model.get(SuggestionCommonProperties.OMNIBOX_THEME),
-                R.attr.selectableItemBackground);
-        view.getDecoratedSuggestionView().setBackground(selectableBackgroundDrawable);
+        Drawable backgroundDrawable = getSelectableBackgroundDrawable(view, model);
+        view.getDecoratedSuggestionView().setBackground(backgroundDrawable);
 
         final List<Action> actions = model.get(BaseSuggestionViewProperties.ACTIONS);
         // Setting ACTIONS and updating actionViews can happen later. Appropriate color scheme will
@@ -101,7 +103,7 @@ public final class BaseSuggestionViewBinder<T extends View>
         final List<ImageView> actionViews = view.getActionButtons();
         for (int index = 0; index < actionViews.size(); index++) {
             ImageView actionView = actionViews.get(index);
-            actionView.setBackground(selectableBackgroundDrawable.getConstantState().newDrawable());
+            actionView.setBackground(copyDrawable(backgroundDrawable));
             updateIcon(actionView, actions.get(index).icon,
                     ChromeColors.getPrimaryIconTintRes(!useDarkColors(model)));
         }
@@ -191,6 +193,33 @@ public final class BaseSuggestionViewBinder<T extends View>
         view.getContentView().setMinimumHeight(minimumHeight);
     }
 
+    /**
+     * Retrieves selecatable background drawable from resources. If possible prefer
+     * {@link #copyDrawable(Drawable)} over this operation, as it offers an order of magnitude
+     * better performance in incognito.
+     * The drawable should be used only once, all other uses should make a copy.
+     *
+     * @param view A view that provides context.
+     * @param model A property model to look up relevant properties.
+     * @return A selectable background drawable.
+     */
+    private static Drawable getSelectableBackgroundDrawable(View view, PropertyModel model) {
+        return OmniboxResourceProvider.resolveAttributeToDrawable(view.getContext(),
+                model.get(SuggestionCommonProperties.OMNIBOX_THEME),
+                R.attr.selectableItemBackground);
+    }
+
+    /**
+     * Creates a copy of the drawable. The drawable should be used only once, all other uses should
+     * make a copy.
+     *
+     * @param original Original drawable to be copied.
+     * @return Copied drawable.
+     */
+    private static Drawable copyDrawable(Drawable original) {
+        return original.getConstantState().newDrawable();
+    }
+
     /** Update image view using supplied drawable state object. */
     private static void updateIcon(
             ImageView view, SuggestionDrawableState sds, @ColorRes int tintRes) {

chrome/android/junit/src/org/chromium/chrome/browser/omnibox/suggestions/base/BaseSuggestionViewBinderUnitTest.java

@@ -141,6 +141,7 @@ public class BaseSuggestionViewBinderUnitTest {
         Assert.assertEquals(1, actionButtons.size());
         Assert.assertEquals(View.VISIBLE, actionButtons.get(0).getVisibility());
         Assert.assertEquals(list.get(0).icon.drawable, actionButtons.get(0).getDrawable());
+        Assert.assertNotNull(actionButtons.get(0).getBackground());
         verify(mBaseView, times(1)).addView(actionButtons.get(0));
 
         Assert.assertTrue(actionButtons.get(0).performClick());