Action In Suggest: pass to Java only what is needed.

The change drops the protobuf serialization in favor of what is
actually required java-side to execute an Action in Suggest.

Bug: 1418077
Change-Id: I14ba6e5ca304fd61a1adf58425ed173448048576
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4544908
Commit-Queue: Fred Mello <fredmello@chromium.org>
Auto-Submit: Tomasz Wiszkowski <ender@google.com>
Reviewed-by: Fred Mello <fredmello@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1146672}

chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxActionsTest.java

@@ -160,8 +160,8 @@ private AutocompleteMatch createDummyHistoryClustersAction(String name) {
     private AutocompleteMatch createDummyActionInSuggest(ActionInfo.ActionType... types) {
         var actions = new ArrayList<OmniboxAction>();
         for (var type : types) {
-            actions.add(new OmniboxActionInSuggest(
-                    "hint", ActionInfo.newBuilder().setActionType(type).build()));
+            actions.add(
+                    new OmniboxActionInSuggest("hint", type.getNumber(), "https://www.google.com"));
         }
 
         return createDummySuggestion(actions);

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

@@ -112,9 +112,7 @@ public void populateModel(AutocompleteMatch suggestion, PropertyModel model, int
                     break;
 
                 case OmniboxActionType.ACTION_IN_SUGGEST:
-                    var actionType = OmniboxActionInSuggest.from(chip)
-                                             .actionInfo.getActionType()
-                                             .getNumber();
+                    var actionType = OmniboxActionInSuggest.from(chip).actionType;
                     mActionInSuggestShownOrUsed.put(actionType, false);
                     break;
             }
@@ -143,7 +141,7 @@ private boolean actionSupported(@NonNull OmniboxAction action) {
                 return true;
 
             case OmniboxActionType.ACTION_IN_SUGGEST:
-                return OmniboxActionInSuggest.from(action).actionInfo.getActionType().getNumber()
+                return OmniboxActionInSuggest.from(action).actionType
                         != EntityInfoProto.ActionInfo.ActionType.CALL_VALUE
                         || mDialerAvailable;
         }
@@ -162,8 +160,7 @@ private void executeAction(@NonNull OmniboxAction action, int position) {
                 break;
 
             case OmniboxActionType.ACTION_IN_SUGGEST:
-                var actionType =
-                        OmniboxActionInSuggest.from(action).actionInfo.getActionType().getNumber();
+                var actionType = OmniboxActionInSuggest.from(action).actionType;
                 mActionInSuggestShownOrUsed.put(actionType, true);
                 break;
         }

components/omnibox/browser/actions/omnibox_action_factory_android.cc

@@ -51,11 +51,12 @@ base::android::ScopedJavaGlobalRef<jobject> BuildHistoryClustersAction(
 base::android::ScopedJavaGlobalRef<jobject> BuildOmniboxActionInSuggest(
     JNIEnv* env,
     const std::u16string& hint,
-    const std::string& serialized_action) {
+    int action_type,
+    const std::string& action_uri) {
   return base::android::ScopedJavaGlobalRef(
       Java_OmniboxActionFactory_buildActionInSuggest(
-          env, base::android::ConvertUTF16ToJavaString(env, hint),
-          base::android::ToJavaByteArray(env, serialized_action)));
+          env, base::android::ConvertUTF16ToJavaString(env, hint), action_type,
+          base::android::ConvertUTF8ToJavaString(env, action_uri)));
 }
 
 // Convert a vector of OmniboxActions to Java counterpart.

components/omnibox/browser/actions/omnibox_action_factory_android.h

@@ -25,7 +25,8 @@ base::android::ScopedJavaGlobalRef<jobject> BuildHistoryClustersAction(
 base::android::ScopedJavaGlobalRef<jobject> BuildOmniboxActionInSuggest(
     JNIEnv* env,
     const std::u16string& hint,
-    const std::string& serialized_action);
+    int action_type,
+    const std::string& action_uri);
 
 base::android::ScopedJavaLocalRef<jobjectArray> ToJavaOmniboxActionsList(
     JNIEnv* env,

components/omnibox/browser/actions/omnibox_action_in_suggest.cc

@@ -93,12 +93,9 @@ OmniboxActionInSuggest::~OmniboxActionInSuggest() = default;
 base::android::ScopedJavaLocalRef<jobject>
 OmniboxActionInSuggest::GetOrCreateJavaObject(JNIEnv* env) const {
   if (!j_omnibox_action_) {
-    std::string serialized_action;
-    if (!action_info_.SerializeToString(&serialized_action)) {
-      serialized_action.clear();
-    }
-    j_omnibox_action_.Reset(
-        BuildOmniboxActionInSuggest(env, strings_.hint, serialized_action));
+    j_omnibox_action_.Reset(BuildOmniboxActionInSuggest(
+        env, strings_.hint, action_info_.action_type(),
+        action_info_.action_uri()));
   }
   return base::android::ScopedJavaLocalRef<jobject>(j_omnibox_action_);
 }

components/omnibox/browser/android/java/src/org/chromium/components/omnibox/action/OmniboxActionFactory.java

@@ -5,36 +5,27 @@
 package org.chromium.components.omnibox.action;
 
 import androidx.annotation.NonNull;
-import androidx.annotation.Nullable;
-
-import com.google.protobuf.InvalidProtocolBufferException;
 
 import org.chromium.base.annotations.CalledByNative;
-import org.chromium.components.omnibox.EntityInfoProto;
 
 /**
  * An interface for handling interactions for Omnibox Action Chips.
  */
 public class OmniboxActionFactory {
     @CalledByNative
-    public static @Nullable OmniboxAction buildOmniboxPedal(
+    public static @NonNull OmniboxAction buildOmniboxPedal(
             @NonNull String hint, @OmniboxPedalType int pedalId) {
         return new OmniboxPedal(hint, pedalId);
     }
 
     @CalledByNative
-    public static @Nullable OmniboxActionInSuggest buildActionInSuggest(
-            @NonNull String hint, @NonNull byte[] serializedActionInfo) {
-        try {
-            return new OmniboxActionInSuggest(
-                    hint, EntityInfoProto.ActionInfo.parseFrom(serializedActionInfo));
-        } catch (InvalidProtocolBufferException e) {
-        }
-        return null;
+    public static @NonNull OmniboxActionInSuggest buildActionInSuggest(@NonNull String hint,
+            /* EntityInfoProto.ActionInfo.ActionType */ int actionType, @NonNull String actionUri) {
+        return new OmniboxActionInSuggest(hint, actionType, actionUri);
     }
 
     @CalledByNative
-    public static @Nullable OmniboxAction buildHistoryClustersAction(
+    public static @NonNull OmniboxAction buildHistoryClustersAction(
             @NonNull String hint, @NonNull String query) {
         return new HistoryClustersAction(hint, query);
     }

components/omnibox/browser/android/java/src/org/chromium/components/omnibox/action/OmniboxActionInSuggest.java

@@ -22,13 +22,14 @@ public class OmniboxActionInSuggest extends OmniboxAction {
     /** Map of {@link EntityInfoProto.ActionInfo.ActionType} to {@link ChipIcon}. */
     private static final SparseArray<ChipIcon> ICON_MAP = createIconMap();
     /** The details about the underlying action. */
-    public final @NonNull EntityInfoProto.ActionInfo actionInfo;
-
-    public OmniboxActionInSuggest(
-            @NonNull String hint, @NonNull EntityInfoProto.ActionInfo actionInfo) {
-        super(OmniboxActionType.ACTION_IN_SUGGEST, hint,
-                ICON_MAP.get(actionInfo.getActionType().getNumber(), null));
-        this.actionInfo = actionInfo;
+    public final /* EntityInfoProto.ActionInfo.ActionType */ int actionType;
+    private final @NonNull String mActionUri;
+
+    public OmniboxActionInSuggest(@NonNull String hint,
+            /* EntityInfoProto.ActionInfo.ActionType */ int actionType, @NonNull String actionUri) {
+        super(OmniboxActionType.ACTION_IN_SUGGEST, hint, ICON_MAP.get(actionType, null));
+        this.actionType = actionType;
+        mActionUri = actionUri;
     }
 
     /**
@@ -65,24 +66,23 @@ private static SparseArray<ChipIcon> createIconMap() {
      */
     @Override
     public void execute(OmniboxActionDelegate delegate) {
-        var actionType = actionInfo.getActionType();
         boolean actionStarted = false;
         boolean isIncognito = delegate.isIncognito();
         Intent intent = null;
 
         try {
-            intent = Intent.parseUri(actionInfo.getActionUri(), Intent.URI_INTENT_SCHEME);
+            intent = Intent.parseUri(mActionUri, Intent.URI_INTENT_SCHEME);
         } catch (URISyntaxException e) {
             // Never happens. http://b/279756377.
         }
 
         switch (actionType) {
-            case REVIEWS:
+            case EntityInfoProto.ActionInfo.ActionType.REVIEWS_VALUE:
                 delegate.loadPageInCurrentTab(intent.getDataString());
                 actionStarted = true;
                 break;
 
-            case CALL:
+            case EntityInfoProto.ActionInfo.ActionType.CALL_VALUE:
                 // Don't call directly. Use `DIAL` instead to let the user decide.
                 // Note also that ACTION_CALL requires a dedicated permission.
                 intent.setAction(Intent.ACTION_DIAL);
@@ -92,7 +92,7 @@ public void execute(OmniboxActionDelegate delegate) {
                 actionStarted = delegate.startActivity(intent);
                 break;
 
-            case DIRECTIONS:
+            case EntityInfoProto.ActionInfo.ActionType.DIRECTIONS_VALUE:
                 // Open directions in maps only if maps are installed and the incognito mode is
                 // not engaged. In all other cases, redirect the action to Browser.
                 if (!isIncognito) {
@@ -116,15 +116,8 @@ public void execute(OmniboxActionDelegate delegate) {
                         OmniboxMetrics.ActionInSuggestIntentResult.ACTIVITY_NOT_FOUND);
             }
 
-            switch (actionType) {
-                case DIRECTIONS:
-                    delegate.loadPageInCurrentTab(intent.getDataString());
-                    break;
-
-                case CALL:
-                case REVIEWS:
-                    // Give up. Don't add the `default` clause though, capture missed variants.
-                    break;
+            if (actionType == EntityInfoProto.ActionInfo.ActionType.DIRECTIONS_VALUE) {
+                delegate.loadPageInCurrentTab(intent.getDataString());
             }
         }
     }

components/omnibox/browser/android/java/src/org/chromium/components/omnibox/action/OmniboxActionInSuggestUnitTest.java

@@ -7,7 +7,6 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThrows;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.doReturn;
@@ -57,12 +56,7 @@ public class OmniboxActionInSuggestUnitTest {
     @Test
     public void creation_usesCustomIconForKnownActionTypes() {
         for (var kesemActionType : sKnownActionTypes) {
-            var proto = EntityInfoProto.ActionInfo.newBuilder()
-                                .setActionType(EntityInfoProto.ActionInfo.ActionType.forNumber(
-                                        kesemActionType))
-                                .build();
-
-            var action = new OmniboxActionInSuggest("hint", proto);
+            var action = new OmniboxActionInSuggest("hint", kesemActionType, "");
             assertNotEquals(OmniboxAction.DEFAULT_ICON, action.icon);
         }
     }
@@ -71,37 +65,25 @@ public void creation_usesCustomIconForKnownActionTypes() {
     public void creation_usesFallbackIconForUnknownActionTypes() {
         for (var kesemActionType : EntityInfoProto.ActionInfo.ActionType.values()) {
             if (sKnownActionTypes.contains(kesemActionType.getNumber())) continue;
-
-            var proto =
-                    EntityInfoProto.ActionInfo.newBuilder().setActionType(kesemActionType).build();
-
-            var action = new OmniboxActionInSuggest("hint", proto);
+            var action = new OmniboxActionInSuggest("hint", kesemActionType.getNumber(), "");
             assertEquals(OmniboxAction.DEFAULT_ICON, action.icon);
         }
     }
 
-    @Test
-    public void creation_creationFailsWithInvalidSerializedProto() {
-        assertNull(OmniboxActionFactory.buildActionInSuggest("hint", new byte[] {1, 2, 3}));
-    }
-
-    @Test
-    public void creation_creationSucceedsWithValidSerializedProto() {
-        var proto = EntityInfoProto.ActionInfo.newBuilder().setDisplayedText("text").build();
-        var action = new OmniboxActionInSuggest("hint", proto);
-
-        assertNotNull(action);
-        assertEquals(action.actionInfo.getDisplayedText(), "text");
-    }
-
     @Test
     public void creation_failsWithNullHint() {
-        assertThrows(AssertionError.class, () -> new OmniboxActionInSuggest(null, EMPTY_INFO));
+        assertThrows(AssertionError.class,
+                ()
+                        -> new OmniboxActionInSuggest(
+                                null, EntityInfoProto.ActionInfo.ActionType.CALL_VALUE, ""));
     }
 
     @Test
     public void creation_failsWithEmptyHint() {
-        assertThrows(AssertionError.class, () -> new OmniboxActionInSuggest("", EMPTY_INFO));
+        assertThrows(AssertionError.class,
+                ()
+                        -> new OmniboxActionInSuggest(
+                                "", EntityInfoProto.ActionInfo.ActionType.CALL_VALUE, ""));
     }
 
     @Test
@@ -121,9 +103,9 @@ public void execute(OmniboxActionDelegate d) {}
     }
 
     @Test
-    public void safeCasting_successWithHistoryClusters() {
-        OmniboxActionInSuggest.from(
-                OmniboxActionFactory.buildActionInSuggest("hint", EMPTY_INFO.toByteArray()));
+    public void safeCasting_successWithFactoryBuiltAction() {
+        OmniboxActionInSuggest.from(OmniboxActionFactory.buildActionInSuggest(
+                "hint", EntityInfoProto.ActionInfo.ActionType.CALL_VALUE, ""));
     }
 
     /**
@@ -132,12 +114,7 @@ public void safeCasting_successWithHistoryClusters() {
     private OmniboxAction buildActionInSuggest(
             EntityInfoProto.ActionInfo.ActionType type, Intent intent) {
         var uri = intent.toUri(Intent.URI_INTENT_SCHEME);
-        var action = EntityInfoProto.ActionInfo.newBuilder()
-                             .setActionType(type)
-                             .setActionUri(uri)
-                             .build();
-
-        return new OmniboxActionInSuggest("wink", action);
+        return new OmniboxActionInSuggest("wink", type.getNumber(), uri);
     }
 
     @Test