[iOS] Enabled autofill::ExecuteJavaScriptFunction to handle various

JS return types.

Previously only strings could be returned which led to double
conversions.

Bug: 1075444
Change-Id: Ibf9cee2e03143654c21c8c48d9219b20e52a7cb8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2340969
Commit-Queue: Maria Kazinova <kazinova@google.com>
Reviewed-by: Vadym Doroshenko  <dvadym@chromium.org>
Reviewed-by: Javier Ernesto Flores Robles <javierrobles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796226}

components/autofill/ios/browser/autofill_util.h

@@ -66,13 +66,22 @@ bool ExtractFormData(const base::Value& form,
 bool ExtractFormFieldData(const base::DictionaryValue& field,
                           FormFieldData* field_data);
 
+typedef base::OnceCallback<void(const base::Value*)> JavaScriptResultCallback;
+
+// Creates a callback for a string JS function return type.
+JavaScriptResultCallback CreateStringCallback(
+    void (^completionHandler)(NSString*));
+
+// Creates a callback for a bool JS function return type.
+JavaScriptResultCallback CreateBoolCallback(void (^completionHandler)(BOOL));
+
 // Executes the JavaScript function with the given name and argument.
 // If |callback| is not null, it will be called when the result of the
 // command is received, or immediately if the command cannot be executed.
 void ExecuteJavaScriptFunction(const std::string& name,
                                const std::vector<base::Value>& parameters,
                                web::WebFrame* frame,
-                               base::OnceCallback<void(NSString*)> callback);
+                               JavaScriptResultCallback callback);
 
 }  // namespace autofill
 

components/autofill/ios/browser/autofill_util.mm

@@ -227,11 +227,32 @@ bool ExtractFormFieldData(const base::DictionaryValue& field,
   return field_data->option_values.size() == field_data->option_contents.size();
 }
 
+JavaScriptResultCallback CreateStringCallback(
+    void (^completionHandler)(NSString*)) {
+  return base::BindOnce(^(const base::Value* res) {
+    NSString* result = nil;
+    if (res && res->is_string()) {
+      result = base::SysUTF8ToNSString(res->GetString());
+    }
+    completionHandler(result);
+  });
+}
+
+JavaScriptResultCallback CreateBoolCallback(void (^completionHandler)(BOOL)) {
+  return base::BindOnce(^(const base::Value* res) {
+    BOOL result = NO;
+    if (res && res->is_bool()) {
+      result = res->GetBool();
+    }
+    completionHandler(result);
+  });
+}
+
 void ExecuteJavaScriptFunction(const std::string& name,
                                const std::vector<base::Value>& parameters,
                                web::WebFrame* frame,
-                               base::OnceCallback<void(NSString*)> callback) {
-  __block base::OnceCallback<void(NSString*)> cb = std::move(callback);
+                               JavaScriptResultCallback callback) {
+  __block JavaScriptResultCallback cb = std::move(callback);
 
   if (!frame) {
     if (!cb.is_null()) {
@@ -243,11 +264,7 @@ void ExecuteJavaScriptFunction(const std::string& name,
   if (!cb.is_null()) {
     bool called = frame->CallJavaScriptFunction(
         name, parameters, base::BindOnce(^(const base::Value* res) {
-          NSString* result = nil;
-          if (res && res->is_string()) {
-            result = base::SysUTF8ToNSString(res->GetString());
-          }
-          std::move(cb).Run(result);
+          std::move(cb).Run(res);
         }),
         base::TimeDelta::FromSeconds(kJavaScriptExecutionTimeoutInSeconds));
     if (!called) {

components/autofill/ios/browser/js_autofill_manager.mm

@@ -37,9 +37,9 @@
     if (base::StringToInt(delayString, &commandLineDelay)) {
       std::vector<base::Value> parameters;
       parameters.push_back(base::Value(commandLineDelay));
-      autofill::ExecuteJavaScriptFunction(
-          "autofill.setDelay", parameters, frame,
-          base::OnceCallback<void(NSString*)>());
+      autofill::ExecuteJavaScriptFunction("autofill.setDelay", parameters,
+                                          frame,
+                                          autofill::JavaScriptResultCallback());
     }
   }
 }
@@ -55,8 +55,9 @@
   std::vector<base::Value> parameters;
   parameters.push_back(base::Value(static_cast<int>(requiredFieldsCount)));
   parameters.push_back(base::Value(restrictUnownedFieldsToFormlessCheckout));
-  autofill::ExecuteJavaScriptFunction("autofill.extractForms", parameters,
-                                      frame, base::BindOnce(completionHandler));
+  autofill::ExecuteJavaScriptFunction(
+      "autofill.extractForms", parameters, frame,
+      autofill::CreateStringCallback(completionHandler));
 }
 
 #pragma mark -
@@ -70,7 +71,7 @@
   parameters.push_back(std::move(*data));
   autofill::ExecuteJavaScriptFunction("autofill.fillActiveFormField",
                                       parameters, frame,
-                                      base::BindOnce(^(NSString*) {
+                                      base::BindOnce(^(const base::Value*) {
                                         completionHandler();
                                       }));
 }
@@ -80,7 +81,7 @@
   parameters.push_back(base::Value(state ? 200 : 0));
   autofill::ExecuteJavaScriptFunction("formHandlers.trackFormMutations",
                                       parameters, frame,
-                                      base::OnceCallback<void(NSString*)>());
+                                      autofill::JavaScriptResultCallback());
 }
 
 - (void)toggleTrackingUserEditedFields:(BOOL)state
@@ -89,7 +90,7 @@
   parameters.push_back(base::Value(static_cast<bool>(state)));
   autofill::ExecuteJavaScriptFunction(
       "formHandlers.toggleTrackingUserEditedFields", parameters, frame,
-      base::OnceCallback<void(NSString*)>());
+      autofill::JavaScriptResultCallback());
 }
 
 - (void)fillForm:(std::unique_ptr<base::Value>)data
@@ -106,7 +107,7 @@
   parameters.push_back(std::move(*data));
   parameters.push_back(base::Value(fieldIdentifier));
   autofill::ExecuteJavaScriptFunction("autofill.fillForm", parameters, frame,
-                                      base::BindOnce(^(NSString*) {
+                                      base::BindOnce(^(const base::Value*) {
                                         completionHandler();
                                       }));
 }
@@ -121,7 +122,7 @@
   parameters.push_back(base::Value(base::SysNSStringToUTF8(fieldIdentifier)));
   autofill::ExecuteJavaScriptFunction("autofill.clearAutofilledFields",
                                       parameters, frame,
-                                      base::BindOnce(^(NSString*) {
+                                      base::BindOnce(^(const base::Value*) {
                                         completionHandler();
                                       }));
 }
@@ -133,7 +134,7 @@
   parameters.push_back(std::move(*data));
   autofill::ExecuteJavaScriptFunction("autofill.fillPredictionData", parameters,
                                       frame,
-                                      base::OnceCallback<void(NSString*)>());
+                                      autofill::JavaScriptResultCallback());
 }
 
 @end

components/autofill/ios/browser/js_suggestion_manager.mm

@@ -56,7 +56,7 @@
   parameters.push_back(base::Value(base::SysNSStringToUTF8(fieldName)));
   autofill::ExecuteJavaScriptFunction(
       "suggestion.selectNextElement", parameters,
-      [self frameWithFrameID:frameID], base::OnceCallback<void(NSString*)>());
+      [self frameWithFrameID:frameID], autofill::JavaScriptResultCallback());
 }
 
 - (void)selectPreviousElementInFrameWithID:(NSString*)frameID {
@@ -71,7 +71,7 @@
   parameters.push_back(base::Value(base::SysNSStringToUTF8(fieldName)));
   autofill::ExecuteJavaScriptFunction(
       "suggestion.selectPreviousElement", parameters,
-      [self frameWithFrameID:frameID], base::OnceCallback<void(NSString*)>());
+      [self frameWithFrameID:frameID], autofill::JavaScriptResultCallback());
 }
 
 - (void)fetchPreviousAndNextElementsPresenceInFrameWithID:(NSString*)frameID
@@ -96,7 +96,8 @@
   parameters.push_back(base::Value(base::SysNSStringToUTF8(fieldName)));
   autofill::ExecuteJavaScriptFunction(
       "suggestion.hasPreviousNextElements", parameters,
-      [self frameWithFrameID:frameID], base::BindOnce(^(NSString* result) {
+      [self frameWithFrameID:frameID],
+      autofill::CreateStringCallback(^(NSString* result) {
         // The result maybe an empty string here due to 2 reasons:
         // 1) When there is an exception running the JS
         // 2) There is a race when the page is changing due to which
@@ -124,7 +125,7 @@
   std::vector<base::Value> parameters;
   autofill::ExecuteJavaScriptFunction(
       "suggestion.blurActiveElement", parameters,
-      [self frameWithFrameID:frameID], base::OnceCallback<void(NSString*)>());
+      [self frameWithFrameID:frameID], autofill::JavaScriptResultCallback());
 }
 
 - (web::WebFrame*)frameWithFrameID:(NSString*)frameID {

components/password_manager/ios/js_password_manager.h

@@ -65,7 +65,7 @@ std::unique_ptr<base::Value> SerializePasswordFormFillData(
                  inFrame:(web::WebFrame*)frame
             withUsername:(std::string)username
                 password:(std::string)password
-       completionHandler:(void (^)(NSString*))completionHandler;
+       completionHandler:(void (^)(BOOL))completionHandler;
 
 // Fills new password field for (optional) |newPasswordIdentifier| and for
 // (optional) confirm password field |confirmPasswordIdentifier| in the form
@@ -77,7 +77,7 @@ std::unique_ptr<base::Value> SerializePasswordFormFillData(
     confirmPasswordIdentifier:
         (autofill::FieldRendererId)confirmPasswordIdentifier
             generatedPassword:(NSString*)generatedPassword
-            completionHandler:(void (^)(NSString*))completionHandler;
+            completionHandler:(void (^)(BOOL))completionHandler;
 
 // Sets up the next available unique ID value in a document.
 - (void)setUpForUniqueIDsWithInitialState:(uint32_t)nextAvailableID

components/password_manager/ios/js_password_manager.mm

@@ -16,6 +16,8 @@
 #error "This file requires ARC support."
 #endif
 
+using autofill::CreateBoolCallback;
+using autofill::CreateStringCallback;
 using autofill::FormRendererId;
 using autofill::FieldRendererId;
 using base::SysNSStringToUTF8;
@@ -78,7 +80,8 @@ std::unique_ptr<base::Value> SerializeFillData(
   DCHECK(completionHandler);
   std::vector<base::Value> parameters;
   autofill::ExecuteJavaScriptFunction("passwords.findPasswordForms", parameters,
-                                      frame, base::BindOnce(completionHandler));
+                                      frame,
+                                      CreateStringCallback(completionHandler));
 }
 
 - (void)extractForm:(FormRendererId)formIdentifier
@@ -89,21 +92,22 @@ std::unique_ptr<base::Value> SerializeFillData(
   parameters.emplace_back(static_cast<int>(formIdentifier.value()));
   autofill::ExecuteJavaScriptFunction("passwords.getPasswordFormDataAsString",
                                       parameters, frame,
-                                      base::BindOnce(completionHandler));
+                                      CreateStringCallback(completionHandler));
 }
 
 - (void)fillPasswordForm:(std::unique_ptr<base::Value>)form
                  inFrame:(web::WebFrame*)frame
             withUsername:(std::string)username
                 password:(std::string)password
-       completionHandler:(void (^)(NSString*))completionHandler {
+       completionHandler:(void (^)(BOOL))completionHandler {
   DCHECK(completionHandler);
   std::vector<base::Value> parameters;
   parameters.push_back(std::move(*form));
   parameters.emplace_back(std::move(username));
   parameters.emplace_back(std::move(password));
   autofill::ExecuteJavaScriptFunction("passwords.fillPasswordForm", parameters,
-                                      frame, base::BindOnce(completionHandler));
+                                      frame,
+                                      CreateBoolCallback(completionHandler));
 }
 
 - (void)fillPasswordForm:(FormRendererId)formIdentifier
@@ -111,7 +115,7 @@ std::unique_ptr<base::Value> SerializeFillData(
         newPasswordIdentifier:(FieldRendererId)newPasswordIdentifier
     confirmPasswordIdentifier:(FieldRendererId)confirmPasswordIdentifier
             generatedPassword:(NSString*)generatedPassword
-            completionHandler:(void (^)(NSString*))completionHandler {
+            completionHandler:(void (^)(BOOL))completionHandler {
   DCHECK(completionHandler);
   std::vector<base::Value> parameters;
   parameters.emplace_back(static_cast<int>(formIdentifier.value()));
@@ -120,7 +124,7 @@ std::unique_ptr<base::Value> SerializeFillData(
   parameters.push_back(base::Value(SysNSStringToUTF8(generatedPassword)));
   autofill::ExecuteJavaScriptFunction(
       "passwords.fillPasswordFormWithGeneratedPassword", parameters, frame,
-      base::BindOnce(completionHandler));
+      CreateBoolCallback(completionHandler));
 }
 
 - (void)setUpForUniqueIDsWithInitialState:(uint32_t)nextAvailableID
@@ -129,7 +133,7 @@ std::unique_ptr<base::Value> SerializeFillData(
   parameters.emplace_back(static_cast<int>(nextAvailableID));
   autofill::ExecuteJavaScriptFunction("fill.setUpForUniqueIDs", parameters,
                                       frame,
-                                      base::OnceCallback<void(NSString*)>());
+                                      autofill::JavaScriptResultCallback());
 }
 
 @end

components/password_manager/ios/password_form_helper.mm

@@ -322,8 +322,7 @@ constexpr char kCommandPrefix[] = "passwordForm";
                 inFrame:GetMainFrame(_webState)
            withUsername:UTF16ToUTF8(usernameValue)
                password:UTF16ToUTF8(passwordValue)
-      completionHandler:^(NSString* result) {
-        BOOL success = [result isEqual:@"true"];
+      completionHandler:^(BOOL success) {
         if (success) {
           weakSelf.fieldDataManager->UpdateFieldDataWithAutofilledValue(
               usernameID, usernameValue,
@@ -351,8 +350,7 @@ constexpr char kCommandPrefix[] = "passwordForm";
           newPasswordIdentifier:newPasswordIdentifier
       confirmPasswordIdentifier:confirmPasswordIdentifier
               generatedPassword:generatedPassword
-              completionHandler:^(NSString* result) {
-                BOOL success = [result isEqual:@"true"];
+              completionHandler:^(BOOL success) {
                 if (success) {
                   weakSelf.fieldDataManager->UpdateFieldDataWithAutofilledValue(
                       newPasswordIdentifier,
@@ -384,8 +382,7 @@ constexpr char kCommandPrefix[] = "passwordForm";
                 inFrame:GetMainFrame(_webState)
            withUsername:UTF16ToUTF8(usernameValue)
                password:UTF16ToUTF8(passwordValue)
-      completionHandler:^(NSString* result) {
-        BOOL success = [result isEqual:@"true"];
+      completionHandler:^(BOOL success) {
         if (success) {
           weakSelf.fieldDataManager->UpdateFieldDataWithAutofilledValue(
               usernameID, usernameValue,

components/password_manager/ios/resources/password_controller.js

@@ -198,10 +198,7 @@ __gCrWeb.passwords['getPasswordFormDataAsString'] = function(identifier) {
  * @param {AutofillFormData} formData Form data.
  * @param {string} username The username to fill.
  * @param {string} password The password to fill.
- * @return {string} Whether a form field has been filled.
- *
- * TODO(crbug.com/1075444): Rewrite callback handler to accept various
- * return types and return boolean.
+ * @return {boolean} Whether a form field has been filled.
  */
 __gCrWeb.passwords['fillPasswordForm'] = function(
     formData, username, password) {
@@ -209,10 +206,9 @@ __gCrWeb.passwords['fillPasswordForm'] = function(
       __gCrWeb.common.removeQueryAndReferenceFromURL(window.location.href);
   const origin = /** @type {string} */ (formData['origin']);
   if (!__gCrWeb.common.isSameOrigin(origin, normalizedOrigin)) {
-    return 'false';
+    return false;
   }
-  return fillPasswordFormWithData(formData, username, password, window)
-      .toString();
+  return fillPasswordFormWithData(formData, username, password, window);
 };
 
 /**
@@ -224,23 +220,20 @@ __gCrWeb.passwords['fillPasswordForm'] = function(
  * @param {number} confirmPasswordIdentifier The id of confirm password element
  *   to fill.
  * @param {string} password The password to fill.
- * @return {string} Whether new password field has been filled.
- *
- * TODO(crbug.com/1075444): Rewrite callback handler to accept various
- * return types and return boolean.
+ * @return {boolean} Whether new password field has been filled.
  */
 __gCrWeb.passwords['fillPasswordFormWithGeneratedPassword'] = function(
     formIdentifier, newPasswordIdentifier, confirmPasswordIdentifier,
     password) {
   const form = __gCrWeb.form.getFormElementFromUniqueFormId(formIdentifier);
   if (!form) {
-    return 'false';
+    return false;
   }
   const inputs = getFormInputElements(form);
   const newPasswordField =
       findInputByUniqueFieldId(inputs, newPasswordIdentifier);
   if (!newPasswordField) {
-    return 'false';
+    return false;
   }
   // Avoid resetting if same value, as it moves cursor to the end.
   if (newPasswordField.value !== password) {
@@ -251,7 +244,7 @@ __gCrWeb.passwords['fillPasswordFormWithGeneratedPassword'] = function(
   if (confirmPasswordField && confirmPasswordField.value !== password) {
     __gCrWeb.fill.setInputElementValue(password, confirmPasswordField);
   }
-  return 'true';
+  return true;
 };
 
 /**

ios/chrome/browser/passwords/password_controller_js_unittest.mm

@@ -93,8 +93,7 @@ TEST_F(PasswordControllerJsTest,
   LoadHtmlAndInject(GAIASignInForm(formOrigin, username, YES), GURL(origin));
   SetUpUniqueIDs();
   EXPECT_NSEQ(
-      @"true",
-      ExecuteJavaScriptWithFormat(
+      @YES, ExecuteJavaScriptWithFormat(
                 @"__gCrWeb.passwords.fillPasswordForm(%@, '%@', '%@')",
                 GAIASignInFormData(formOrigin, formName), username, password));
   // Verifies that the sign-in form has been filled with username/password.
@@ -117,8 +116,7 @@ TEST_F(PasswordControllerJsTest,
   LoadHtmlAndInject(GAIASignInForm(formOrigin, username1, YES), GURL(origin));
   SetUpUniqueIDs();
   EXPECT_NSEQ(
-      @"false",
-      ExecuteJavaScriptWithFormat(
+      @NO, ExecuteJavaScriptWithFormat(
                @"__gCrWeb.passwords.fillPasswordForm(%@, '%@', '%@')",
                GAIASignInFormData(formOrigin, formName), username2, password));
   // Verifies that the sign-in form has not been filled.
@@ -141,8 +139,7 @@ TEST_F(PasswordControllerJsTest,
   LoadHtmlAndInject(GAIASignInForm(formOrigin, username1, NO), GURL(origin));
   SetUpUniqueIDs();
   EXPECT_NSEQ(
-      @"true",
-      ExecuteJavaScriptWithFormat(
+      @YES, ExecuteJavaScriptWithFormat(
                 @"__gCrWeb.passwords.fillPasswordForm(%@, '%@', '%@')",
                 GAIASignInFormData(formOrigin, formName), username2, password));
   // Verifies that the sign-in form has been filled with the new username
@@ -452,8 +449,7 @@ TEST_F(PasswordControllerJsTest, OriginsAreDifferentInPathes) {
                         "  ]"
                         "}",
                        page_origin.c_str(), form_fill_data_origin.c_str()];
-  EXPECT_NSEQ(@"true",
-              ExecuteJavaScriptWithFormat(
+  EXPECT_NSEQ(@YES, ExecuteJavaScriptWithFormat(
                         @"__gCrWeb.passwords.fillPasswordForm(%@, '%@', '%@')",
                         form_fill_data, username, password));
   // Verifies that the sign-in form has been filled with username/password.
@@ -478,7 +474,7 @@ TEST_F(PasswordControllerJsTest,
   uint32_t formIdentifier = 404;
   NSString* const password = @"abc";
   uint32_t newPasswordIdentifier = 1;
-  EXPECT_NSEQ(@"false",
+  EXPECT_NSEQ(@NO,
               ExecuteJavaScriptWithFormat(
                   @"__gCrWeb.passwords."
                   @"fillPasswordFormWithGeneratedPassword(%d, %d, %d, '%@')",
@@ -502,12 +498,12 @@ TEST_F(PasswordControllerJsTest,
   NSString* const password = @"abc";
   uint32_t const newPasswordIdentifier = 2;
   uint32_t const confirmPasswordIdentifier = 3;
-  EXPECT_NSEQ(@"false",
-              ExecuteJavaScriptWithFormat(
+  EXPECT_NSEQ(
+      @NO, ExecuteJavaScriptWithFormat(
                @"__gCrWeb.passwords."
                @"fillPasswordFormWithGeneratedPassword(%d, %d, %d, '%@')",
-                  formIdentifier, newPasswordIdentifier,
-                  confirmPasswordIdentifier, password));
+               formIdentifier, newPasswordIdentifier, confirmPasswordIdentifier,
+               password));
 }
 
 // Check that a matching and complete password form is successfully filled
@@ -530,7 +526,7 @@ TEST_F(PasswordControllerJsTest,
   NSString* const password = @"abc";
   uint32_t const newPasswordIdentifier = 2;
   uint32_t const confirmPasswordIdentifier = 3;
-  EXPECT_NSEQ(@"true",
+  EXPECT_NSEQ(@YES,
               ExecuteJavaScriptWithFormat(
                   @"__gCrWeb.passwords."
                   @"fillPasswordFormWithGeneratedPassword(%u, %u, %u, '%@')",
@@ -567,7 +563,7 @@ TEST_F(
   uint32_t formIdentifier = 0;
   NSString* const password = @"abc";
   uint32_t const newPasswordIdentifier = 2;
-  EXPECT_NSEQ(@"true",
+  EXPECT_NSEQ(@YES,
               ExecuteJavaScriptWithFormat(
                   @"__gCrWeb.passwords."
                   @"fillPasswordFormWithGeneratedPassword(%u, %u, %u, '%@')",
@@ -602,7 +598,7 @@ TEST_F(
   uint32_t formIdentifier = 0;
   NSString* const password = @"abc";
   uint32_t const confirmPasswordIdentifier = 3;
-  EXPECT_NSEQ(@"false",
+  EXPECT_NSEQ(@NO,
               ExecuteJavaScriptWithFormat(
                   @"__gCrWeb.passwords."
                   @"fillPasswordFormWithGeneratedPassword(%u, %u, %u, '%@')",
@@ -634,8 +630,7 @@ TEST_F(
   uint32_t formIdentifier = 0;
   NSString* const password = @"abc";
   EXPECT_NSEQ(
-      @"false",
-      ExecuteJavaScriptWithFormat(
+      @NO, ExecuteJavaScriptWithFormat(
                @"__gCrWeb.passwords."
                @"fillPasswordFormWithGeneratedPassword(%u, '%@', null, '%@')",
                formIdentifier, @"hello", password));
@@ -676,7 +671,7 @@ TEST_F(PasswordControllerJsTest, FillOnlyPasswordField) {
            "  ]"
            "}",
           page_origin.c_str(), form_fill_data_origin.c_str()];
-  EXPECT_NSEQ(@"true", ExecuteJavaScriptWithFormat(
+  EXPECT_NSEQ(@YES, ExecuteJavaScriptWithFormat(
                         @"__gCrWeb.passwords.fillPasswordForm(%@, '', '%@')",
                         form_fill_data, password));
   // Verifies that the sign-in form has been filled with |password|.