[Autofill Assistant] Moved overlay color parsing logic to native.

There are two benefits: First, we want to move all logic from Java to native anyway. Second, we are about to add more colors to the overlay (see bug), which exacerbates the problem.

This is a refactoring and does not contain any user-facing changes.

Bug: b/143452916
Change-Id: Ic418ce6993c1cbfd60746fa6df7f9ec493afdba3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1881560
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710319}

chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/overlay/AssistantOverlayModel.java

@@ -87,41 +87,60 @@ public class AssistantOverlayModel extends PropertyModel {
     }
 
     @CalledByNative
-    private boolean setBackgroundColor(String colorString) {
-        return setColor(BACKGROUND_COLOR, colorString);
+    private void setBackgroundColor(@ColorInt int color) {
+        set(BACKGROUND_COLOR, color);
     }
 
     @CalledByNative
-    private boolean setHighlightBorderColor(String colorString) {
-        return setColor(HIGHLIGHT_BORDER_COLOR, colorString);
+    private void clearBackgroundColor() {
+        set(BACKGROUND_COLOR, null);
     }
 
     @CalledByNative
-    private void setTapTracking(int count, long durationMs) {
-        set(TAP_TRACKING_COUNT, count);
-        set(TAP_TRACKING_DURATION_MS, durationMs);
+    private void setHighlightBorderColor(@ColorInt int color) {
+        set(HIGHLIGHT_BORDER_COLOR, color);
+    }
+
+    @CalledByNative
+    private void clearHighlightBorderColor() {
+        set(HIGHLIGHT_BORDER_COLOR, null);
     }
 
     /**
-     * Sets the given color property.
-     *
-     * @param property property to set
-     * @param colorString color value as a property. The empty string means use the default color
-     * @return true if the color string was parsed and set properly
+     * Parses {@code colorString} and returns the corresponding color integer. This is only safe to
+     * call for valid strings, which should be checked with {@code isValidColorString} before
+     * calling this method!
+     * @return the 32-bit integer representation of {@code colorString} or an unspecified fallback
+     * value if {@code colorString} is not a valid color string.
      */
-    private boolean setColor(WritableObjectPropertyKey<Integer> property, String colorString) {
+    @CalledByNative
+    private static @ColorInt int parseColorString(String colorString) {
+        if (!isValidColorString(colorString)) {
+            return Color.BLACK;
+        }
+        return Color.parseColor(colorString);
+    }
+
+    /**
+     * Returns whether {@code colorString} is a valid string representation of a color. Supported
+     * color formats are #RRGGBB and #AARRGGBB.
+     */
+    @CalledByNative
+    private static boolean isValidColorString(String colorString) {
         if (colorString.isEmpty()) {
-            set(property, null);
-            return true;
+            return false;
         }
-        @ColorInt
-        int colorInt;
         try {
-            set(property, Color.parseColor(colorString));
+            Color.parseColor(colorString);
             return true;
         } catch (IllegalArgumentException e) {
-            set(property, null);
             return false;
         }
     }
+
+    @CalledByNative
+    private void setTapTracking(int count, long durationMs) {
+        set(TAP_TRACKING_COUNT, count);
+        set(TAP_TRACKING_DURATION_MS, durationMs);
+    }
 }

chrome/browser/android/autofill_assistant/ui_controller_android.cc

@@ -14,6 +14,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/metrics/field_trial_params.h"
+#include "base/optional.h"
 #include "base/task/post_task.h"
 #include "base/time/time.h"
 #include "chrome/android/features/autofill_assistant/jni_headers/AssistantCollectUserDataModel_jni.h"
@@ -78,6 +79,22 @@ base::android::ScopedJavaLocalRef<jobject> CreateJavaDateTime(
       proto.time().hour(), proto.time().minute(), proto.time().second());
 }
 
+// Returns a 32-bit integer representing |color_string| in Java. Uses
+// base::Optional to distinguish between valid and invalid colors.
+base::Optional<int> CreateJavaColor(JNIEnv* env,
+                                    const std::string& color_string) {
+  if (!Java_AssistantOverlayModel_isValidColorString(
+          env, base::android::ConvertUTF8ToJavaString(env, color_string))) {
+    if (!color_string.empty()) {
+      DVLOG(1) << "Encountered invalid color string: " << color_string;
+    }
+    return base::Optional<int>();
+  }
+
+  return Java_AssistantOverlayModel_parseColorString(
+      env, base::android::ConvertUTF8ToJavaString(env, color_string));
+}
+
 // Creates the java equivalent to the text inputs specified in |section|.
 base::android::ScopedJavaLocalRef<jobject> CreateJavaTextInputsForSection(
     JNIEnv* env,
@@ -381,18 +398,21 @@ void UiControllerAndroid::OnOverlayColorsChanged(
     const UiDelegate::OverlayColors& colors) {
   JNIEnv* env = AttachCurrentThread();
   auto overlay_model = GetOverlayModel();
-  if (!Java_AssistantOverlayModel_setBackgroundColor(
-          env, overlay_model,
-          base::android::ConvertUTF8ToJavaString(env, colors.background))) {
-    DVLOG(1) << __func__
-             << ": Ignoring invalid overlay color: " << colors.background;
+
+  auto background_color = CreateJavaColor(env, colors.background);
+  if (background_color.has_value()) {
+    Java_AssistantOverlayModel_setBackgroundColor(env, overlay_model,
+                                                  *background_color);
+  } else {
+    Java_AssistantOverlayModel_clearBackgroundColor(env, overlay_model);
   }
-  if (!Java_AssistantOverlayModel_setHighlightBorderColor(
-          env, overlay_model,
-          base::android::ConvertUTF8ToJavaString(env,
-                                                 colors.highlight_border))) {
-    DVLOG(1) << __func__ << ": Ignoring invalid highlight border color: "
-             << colors.highlight_border;
+
+  auto highlight_border_color = CreateJavaColor(env, colors.highlight_border);
+  if (highlight_border_color.has_value()) {
+    Java_AssistantOverlayModel_setHighlightBorderColor(env, overlay_model,
+                                                       *highlight_border_color);
+  } else {
+    Java_AssistantOverlayModel_clearHighlightBorderColor(env, overlay_model);
   }
 }