Android: Fix StrictMode violation on Smart Selection icon loading #2

The previous fix http://crrev/c/2562059 isn't very correct:
1) It introduced menu flickering because the icons could be set after
   the menu is already showing.
2) Even after a pre-warming, the second #loadDrawableAsync() call could
   still be slow, so the icons doesn't show sometimes.
3) An internal Android framework change made the situation worse that
   sometimes the menu item won't send the intent. Because the menu item
   can change its icon from null to an actual drawable, but the
   framework code made the assumption that the menu item won't change.

This fix moves the icon loading work to the same background task for
the smart text classification task, after we get the classification
result. In this way we could have a deterministic
icon loading result while not violating the strict mode rule.

Fixed: 1129211
Change-Id: I7c35d8221bf57bd8ed68295f797d5bd6871dbae7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626461Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Shimi Zhang <ctzsm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843100}

content/public/android/java/src/org/chromium/content/browser/selection/AdditionalMenuItemProvider.java

@@ -5,11 +5,14 @@
 package org.chromium.content.browser.selection;
 
 import android.content.Context;
+import android.graphics.drawable.Drawable;
 import android.view.Menu;
 import android.view.MenuItem;
 import android.view.View;
 import android.view.textclassifier.TextClassification;
 
+import java.util.List;
+
 /**
  * An interface for adding more menu items.
  */
@@ -20,7 +23,8 @@ public interface AdditionalMenuItemProvider {
      * @param menu Add menu items to this menu.
      * @param classification Providing info to generate menu items.
      */
-    void addMenuItems(Context context, Menu menu, TextClassification classification);
+    void addMenuItems(
+            Context context, Menu menu, TextClassification classification, List<Drawable> icons);
 
     /**
      * Call this to trigger internal cleanup.

content/public/android/java/src/org/chromium/content/browser/selection/AdditionalMenuItemProviderImpl.java

@@ -8,6 +8,7 @@ import android.annotation.TargetApi;
 import android.app.PendingIntent;
 import android.app.RemoteAction;
 import android.content.Context;
+import android.graphics.drawable.Drawable;
 import android.os.Build;
 import android.text.TextUtils;
 import android.view.Menu;
@@ -19,7 +20,9 @@ import android.view.textclassifier.TextClassification;
 import org.chromium.base.Log;
 import org.chromium.base.annotations.VerifiesOnP;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 // TODO(ctzsm): Add unit tests for this class once this is upstreamed.
@@ -42,18 +45,24 @@ public class AdditionalMenuItemProviderImpl implements AdditionalMenuItemProvide
     private final Map<MenuItem, OnClickListener> mAssistClickHandlers = new HashMap<>();
 
     @Override
-    public void addMenuItems(Context context, Menu menu, TextClassification classification) {
+    public void addMenuItems(
+            Context context, Menu menu, TextClassification classification, List<Drawable> icons) {
         if (menu == null || classification == null) return;
 
         final int count = classification.getActions().size();
 
+        assert icons == null
+                || icons.size()
+                        == count
+            : "icons list should be either null or have the same length with actions.";
+
         // Fallback to new API to set icon on P.
         if (count > 0) {
             RemoteAction primaryAction = classification.getActions().get(0);
 
             MenuItem item = menu.findItem(android.R.id.textAssist);
             if (primaryAction.shouldShowIcon()) {
-                item.setIcon(primaryAction.getIcon().loadDrawable(context));
+                item.setIcon(icons == null ? null : icons.get(0));
             } else {
                 item.setIcon(null);
             }
@@ -72,7 +81,7 @@ public class AdditionalMenuItemProviderImpl implements AdditionalMenuItemProvide
                     MENU_ITEM_ORDER_SECONDARY_ASSIST_ACTIONS_START + i, action.getTitle());
             item.setContentDescription(action.getContentDescription());
             if (action.shouldShowIcon()) {
-                item.setIcon(action.getIcon().loadDrawable(context));
+                item.setIcon(icons == null ? null : icons.get(i));
             }
             // Set this flag to SHOW_AS_ACTION_IF_ROOM to match text processing menu items. So
             // Android could put them to the same level and then consider their actual order.
@@ -93,6 +102,20 @@ public class AdditionalMenuItemProviderImpl implements AdditionalMenuItemProvide
         listener.onClick(view);
     }
 
+    // Because Icon#loadDrawable() should not be called on UI thread, we pre-load the icons on
+    // background thread right after we get the text classification result in
+    // SmartSelectionProvider. TextClassification#getActions() is only available on P and above, so
+    // make this method in a @VerifiesOnP class.
+    public static List<Drawable> loadIconDrawables(Context context, TextClassification tc) {
+        if (context == null || tc == null) return null;
+
+        ArrayList<Drawable> res = new ArrayList<>();
+        for (RemoteAction action : tc.getActions()) {
+            res.add(action.getIcon().loadDrawable(context));
+        }
+        return res;
+    }
+
     private static OnClickListener getSupportedOnClickListener(
             CharSequence title, PendingIntent pendingIntent) {
         if (TextUtils.isEmpty(title) || pendingIntent == null) return null;

content/public/android/java/src/org/chromium/content/browser/selection/SelectionPopupControllerImpl.java

@@ -796,8 +796,9 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
         Context windowContext = mWindowAndroid.getContext().get();
         if (mClassificationResult != null && mAdditionalMenuItemProvider != null
                 && windowContext != null) {
-            mAdditionalMenuItemProvider.addMenuItems(
-                    windowContext, menu, mClassificationResult.textClassification);
+            mAdditionalMenuItemProvider.addMenuItems(windowContext, menu,
+                    mClassificationResult.textClassification,
+                    mClassificationResult.additionalIcons);
         }
 
         if (!hasSelection() || isSelectionPassword()) return;

content/public/android/java/src/org/chromium/content/browser/selection/SmartSelectionProvider.java

@@ -130,7 +130,10 @@ public class SmartSelectionProvider {
             mClassificationTask = null;
         }
 
-        mClassificationTask = new ClassificationTask(classifier, requestType, text, start, end);
+        // We checked mWindowAndroid.getContext().get() is not null in getTextClassifier(), so pass
+        // the value directly here.
+        mClassificationTask = new ClassificationTask(
+                classifier, requestType, text, start, end, mWindowAndroid.getContext().get());
         mClassificationTask.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
     }
 
@@ -141,14 +144,16 @@ public class SmartSelectionProvider {
         private final CharSequence mText;
         private final int mOriginalStart;
         private final int mOriginalEnd;
+        private final Context mContext;
 
         ClassificationTask(TextClassifier classifier, @RequestType int requestType,
-                CharSequence text, int start, int end) {
+                CharSequence text, int start, int end, Context context) {
             mTextClassifier = classifier;
             mRequestType = requestType;
             mText = text;
             mOriginalStart = start;
             mOriginalEnd = end;
+            mContext = context;
         }
 
         @Override
@@ -184,6 +189,11 @@ public class SmartSelectionProvider {
             result.textSelection = ts;
             result.textClassification = tc;
 
+            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {
+                result.additionalIcons = AdditionalMenuItemProviderImpl.loadIconDrawables(
+                        mContext, result.textClassification);
+            }
+
             return result;
         }
 

content/public/android/java/src/org/chromium/content_public/browser/SelectionClient.java

@@ -14,6 +14,8 @@ import android.view.textclassifier.TextSelection;
 import org.chromium.content.browser.selection.SmartSelectionClient;
 import org.chromium.ui.touch_selection.SelectionEventType;
 
+import java.util.List;
+
 /**
  * Interface to a content layer client that can process and modify selection text.
  */
@@ -64,6 +66,11 @@ public interface SelectionClient {
          */
         public TextSelection textSelection;
 
+        /**
+         * Icons for additional menu items.
+         */
+        public List<Drawable> additionalIcons;
+
         /**
          * A helper method that returns true if the result has both visual info
          * and an action so that, for instance, one can make a new menu item.