Norge Vizcay (Gerrit)

unread,

Jun 12, 2026, 6:16:39 AMJun 12

to Tamino Bauknecht, Daniel Cheng, Jihad Hanna, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jshin...@chromium.org, browser-comp...@chromium.org

Attention needed from Tamino Bauknecht

Norge Vizcay added 3 comments

File chrome/renderer/autofill/form_autofill_browsertest.cc

Line 668, Patchset 12 (Latest):

    EXPECT_THAT(fields[0],
                test::FormFieldDescriptionEq({.label = u"John",
                                              .name = u"firstname",
                                              .id_attribute = u"firstname",
                                              .value = u"John"}));

    EXPECT_THAT(fields[1],
                test::FormFieldDescriptionEq({.label = u"Smith",
                                              .name = u"lastname",
                                              .id_attribute = u"lastname",
                                              .value = u"Smith"}));

    EXPECT_THAT(fields[2], test::FormFieldDescriptionEq(
                               {.label = u"jo...@example.com",
                                .name = u"email",
                                .id_attribute = u"email",
                                .value = u"jo...@example.com",
                                .autocomplete_attribute = "off"}));

    EXPECT_THAT(fields[3],
                test::FormFieldDescriptionEq({.label = u"1.800.555.1234",
                                              .name = u"phone",
                                              .id_attribute = u"phone",
                                              .value = u"1.800.555.1234",
                                              .autocomplete_attribute = {}}));

Norge Vizcay . unresolved

You can do:

EXPECT_THAT(form.fields(), testing::ElementsAre(
     test::FormFieldDescriptionEq({...}),
     test::FormFieldDescriptionEq({...}),
     test::FormFieldDescriptionEq({...})
));

(Same below)

Line 1083, Patchset 12 (Latest):

    EXPECT_THAT(
        fields[1],
        test::FormFieldDescriptionEq(
            {.label = initial_lastname
                          ? ASCIIToUTF16(initial_lastname)
                          : (placeholder_lastname ? std::optional(ASCIIToUTF16(
                                                        placeholder_lastname))
                                                  : std::nullopt),
             .name = u"lastname",
             .id_attribute = u"lastname",
             .value = initial_lastname
                          ? std::optional(ASCIIToUTF16(initial_lastname))
                          : (placeholder_lastname ? std::optional(ASCIIToUTF16(
                                                        placeholder_lastname))
                                                  : std::nullopt),
             .placeholder =
                 !initial_lastname && placeholder_lastname
                     ? std::optional(ASCIIToUTF16(placeholder_lastname))
                     : std::nullopt,
             .is_autofilled_according_to_renderer = false}));

    EXPECT_THAT(
        fields[2],
        test::FormFieldDescriptionEq(
            {.label = initial_email
                          ? ASCIIToUTF16(initial_email)
                          : (placeholder_email ? std::optional(ASCIIToUTF16(
                                                     placeholder_email))
                                               : std::nullopt),
             .name = u"email",
             .id_attribute = u"email",
             .value = initial_email
                          ? ASCIIToUTF16(initial_email)
                          : (placeholder_email ? std::optional(ASCIIToUTF16(
                                                     placeholder_email))
                                               : std::nullopt),
             .placeholder = !initial_email && placeholder_email
                                ? std::optional(ASCIIToUTF16(placeholder_email))
                                : std::nullopt,
             .is_autofilled_according_to_renderer = false}));

Norge Vizcay . unresolved

Opt: Consider some of the labels and values into local variables to make things more readable.

Line 1485, Patchset 12 (Latest):

  EXPECT_EQ(result.selected_option_text(), u"California");
  ASSERT_EQ(2U, result.options().size());

Norge Vizcay . unresolved

Isn't this checked above?

Open in Gerrit

Related details

Attention is currently required from:

Tamino Bauknecht

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tamino Bauknecht (Gerrit)

unread,

Jun 15, 2026, 3:40:00 AMJun 15

to Daniel Cheng, Norge Vizcay, Jihad Hanna, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jshin...@chromium.org, browser-comp...@chromium.org

Attention needed from Norge Vizcay

Tamino Bauknecht added 4 comments

Patchset-level comments

File-level comment, Patchset 10:

Tamino Bauknecht . resolved

Hey Norge, do you have time to take a look at these CLs for the second approval? It's probably best to start here to see how the newly introduced function of the other CL is used :)

Tamino Bauknecht

Mark as resolved.

File chrome/renderer/autofill/form_autofill_browsertest.cc

Line 668, Patchset 12: EXPECT_THAT(fields[0],


                test::FormFieldDescriptionEq({.label = u"John",
                                              .name = u"firstname",
                                              .id_attribute = u"firstname",
                                              .value = u"John"}));

    EXPECT_THAT(fields[1],
                test::FormFieldDescriptionEq({.label = u"Smith",
                                              .name = u"lastname",
                                              .id_attribute = u"lastname",
                                              .value = u"Smith"}));

    EXPECT_THAT(fields[2], test::FormFieldDescriptionEq(
                               {.label = u"jo...@example.com",
                                .name = u"email",
                                .id_attribute = u"email",
                                .value = u"jo...@example.com",
                                .autocomplete_attribute = "off"}));

    EXPECT_THAT(fields[3],
                test::FormFieldDescriptionEq({.label = u"1.800.555.1234",
                                              .name = u"phone",
                                              .id_attribute = u"phone",
                                              .value = u"1.800.555.1234",
                                              .autocomplete_attribute = {}}));

Norge Vizcay . resolved

You can do:

EXPECT_THAT(form.fields(), testing::ElementsAre(
     test::FormFieldDescriptionEq({...}),
     test::FormFieldDescriptionEq({...}),
     test::FormFieldDescriptionEq({...})
));

(Same below)

Tamino Bauknecht

Done, good idea - thanks :) Sorry that it caused a pretty huge diff again :|

Line 1083, Patchset 12: EXPECT_THAT(

Norge Vizcay . resolved

Opt: Consider some of the labels and values into local variables to make things more readable.

Tamino Bauknecht

Done. I will also refactor these test functions in a future follow-up - this test file is simply too large to refactor everything in one go :) (reviewing it is probably already horrible as-is, sorry about that :| )

Line 1485, Patchset 12:

  EXPECT_EQ(result.selected_option_text(), u"California");
  ASSERT_EQ(2U, result.options().size());

Norge Vizcay . resolved

Isn't this checked above?

Tamino Bauknecht

Done - you are right, it should be safe to assume that `value()` matches the corresponding `selected_option_text()` here.

Open in Gerrit

Related details

Attention is currently required from:

Norge Vizcay

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jihad Hanna (Gerrit)

unread,

Jun 15, 2026, 3:45:29 AMJun 15

to Tamino Bauknecht, Daniel Cheng, Norge Vizcay, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jshin...@chromium.org, browser-comp...@chromium.org

Attention needed from Norge Vizcay and Tamino Bauknecht

Jihad Hanna voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Norge Vizcay
Tamino Bauknecht

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Norge Vizcay (Gerrit)

unread,

Jun 15, 2026, 4:02:15 AMJun 15

to Tamino Bauknecht, Jihad Hanna, Daniel Cheng, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jshin...@chromium.org, browser-comp...@chromium.org

Attention needed from Jihad Hanna and Tamino Bauknecht

Norge Vizcay voted and added 2 comments

Votes added by Norge Vizcay

Code-Review

+1

2 comments

File chrome/renderer/autofill/form_autofill_browsertest.cc

Line 723, Patchset 19 (Latest): {.label = {},

Norge Vizcay . unresolved

Do we need to manually define empty values?

Line 2760, Patchset 19 (Latest):

  std::vector<test::CommonFieldDescription> fields;

  fields.push_back({.label = u"* First Name",


                    .name = u"firstname",

.name_attribute = u"",


                    .id_attribute = u"firstname",

                    .value = u"John",
                    .form_control_type = FormControlType::kInputText});

  fields.push_back({.label = u"* Middle Name",
                    .name = u"middlename",
                    .name_attribute = u"",
                    .id_attribute = u"middlename",
                    .value = u"Joe",
                    .form_control_type = FormControlType::kInputText});

  fields.push_back({.label = u"* Last Name",


                    .name = u"lastname",

.name_attribute = u"",


                    .id_attribute = u"lastname",

                    .value = u"Smith",
                    .form_control_type = FormControlType::kInputText});

  fields.push_back({.label = u"* Country",
                    .name = u"country",
                    .name_attribute = u"",
                    .id_attribute = u"country",
                    .value = u"US",
                    .max_length = 0,
                    .form_control_type = FormControlType::kSelectOne});

  fields.push_back({.label = u"* Email",


                    .name = u"email",

.name_attribute = u"",


                    .id_attribute = u"email",
                    .value = u"jo...@example.com",

.form_control_type = FormControlType::kInputText});

Norge Vizcay . unresolved

Can we use an initializer list here? e.g.

```
const std::vector<test::CommonFieldDescription> fields = {
     {.label = u"* First Name",


      .name = u"firstname",

      .name_attribute = u"",


      .id_attribute = u"firstname",

      .value = u"John",
      .form_control_type = FormControlType::kInputText},
     {.label = u"* Middle Name",
      .name = u"middlename",
      // ...
      .form_control_type = FormControlType::kInputText},
     // ...
 };
```
?

Open in Gerrit

Related details

Attention is currently required from:

Jihad Hanna
Tamino Bauknecht

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tamino Bauknecht (Gerrit)

unread,

Jun 15, 2026, 4:17:55 AMJun 15

to Norge Vizcay, Jihad Hanna, Daniel Cheng, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jshin...@chromium.org, browser-comp...@chromium.org

Attention needed from Jihad Hanna and Norge Vizcay

Tamino Bauknecht added 6 comments

File chrome/renderer/autofill/form_autofill_browsertest.cc

Line 690, Patchset 3 (Parent):

    FormFieldData expected;
    expected.set_form_control_type(FormControlType::kInputText);
    expected.set_max_length(FormFieldData::kDefaultMaxLength);

Tamino Bauknecht . resolved

These were dropped in the expectations since I don't think they need to be checked here. This was done for most of the `form_control_type`s and `max_length`s.

Jihad Hanna

Acknowledged

Tamino Bauknecht

Marked as resolved.

Line 723, Patchset 19: {.label = {},

Norge Vizcay . unresolved

Do we need to manually define empty values?

Tamino Bauknecht

Great that you caught these, thanks - yes, I think we do want to set the fields here to an empty value because they won't be checked otherwise, but you are completely right that we want to set it to an empty string and not `std::nullopt`. I (hopefully) replaced all of these mistakes.

Line 1074, Patchset 3: : std::nullopt,

Tamino Bauknecht . resolved

This is a change in logic since it will no longer check that the label is empty (same below). However, I think this is acceptable for these test cases considering the HTML used when calling this function.
In a follow-up, I would also like to refactor all of these functions anyway or at least replace all of these `const char*` by e.g. `std::string_view`.

Jihad Hanna

Acknowledged

Tamino Bauknecht

Marked as resolved.

Line 1566, Patchset 1 (Parent): expected.set_should_autocomplete(false);

Tamino Bauknecht . resolved

This line was intentionally not transferred to the new implementation since it should be `true` and the test only passed because `EXPECT_FORM_FIELD_DATA_EQUALS` did not check `should_autocomplete`.

Jihad Hanna

Acknowledged

Tamino Bauknecht

Marked as resolved.

Line 2760, Patchset 19: std::vector<test::CommonFieldDescription> fields;

Norge Vizcay . resolved

Can we use an initializer list here? e.g.

```
const std::vector<test::CommonFieldDescription> fields = {
     {.label = u"* First Name",
      .name = u"firstname",
      .name_attribute = u"",
      .id_attribute = u"firstname",
      .value = u"John",
      .form_control_type = FormControlType::kInputText},
     {.label = u"* Middle Name",
      .name = u"middlename",
      // ...
      .form_control_type = FormControlType::kInputText},
     // ...
 };
```
?

Tamino Bauknecht

Done, good point :)

Line 4503, Patchset 3 (Parent):

  expected.set_form_control_type(FormControlType::kInputText);
  expected.set_max_length(FormFieldData::kDefaultMaxLength);

Tamino Bauknecht . resolved

Although these are explicitly defined for each expectation, I don't think the test actually wants to check for the type of input field for the first two fields. Thus, I think it is fine to omit them in the refactoring (in other places I transferred them to the new `EXPECT`s if it was explicit).

Jihad Hanna

Acknowledged

Tamino Bauknecht

Marked as resolved.

Open in Gerrit

Related details

Attention is currently required from:

Jihad Hanna
Norge Vizcay

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Norge Vizcay (Gerrit)

unread,

Jun 15, 2026, 4:19:40 AMJun 15

to Tamino Bauknecht, Jihad Hanna, Daniel Cheng, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jshin...@chromium.org, browser-comp...@chromium.org

Attention needed from Jihad Hanna and Tamino Bauknecht

Norge Vizcay voted and added 2 comments

Votes added by Norge Vizcay

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 20 (Latest):

Norge Vizcay . resolved

lgtm!

File chrome/renderer/autofill/form_autofill_browsertest.cc

Line 723, Patchset 19: {.label = {},

Norge Vizcay . resolved

Do we need to manually define empty values?

Tamino Bauknecht

Great that you caught these, thanks - yes, I think we do want to set the fields here to an empty value because they won't be checked otherwise, but you are completely right that we want to set it to an empty string and not `std::nullopt`. I (hopefully) replaced all of these mistakes.

Norge Vizcay

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:

Jihad Hanna
Tamino Bauknecht

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jihad Hanna (Gerrit)

unread,

Jun 15, 2026, 4:20:48 AMJun 15

to Tamino Bauknecht, Norge Vizcay, Daniel Cheng, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jshin...@chromium.org, browser-comp...@chromium.org

Attention needed from Tamino Bauknecht

Jihad Hanna voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Tamino Bauknecht

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 15, 2026, 5:59:21 AMJun 15

to Tamino Bauknecht, Jihad Hanna, Norge Vizcay, Daniel Cheng, chromium...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jshin...@chromium.org, browser-comp...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

autofill: Refactor EXPECT_FORM_FIELD_DATA_EQUALS

This refactors the C-style macro `EXPECT_FORM_FIELD_DATA_EQUALS` in
`form_autofill_browsertest.cc` to use `EXPECT_THAT` instead and the
matcher `test::FormFieldDescriptionEq`.

Unfortunately, `test::FormFieldDataEq` cannot be used because even with
`test::WithoutUnserializedData`, too many fields are compared which are
not equal, namely `renderer_id`, `host_form_id`, `should_complete`,
`text_direction` and `bounds` are not equal for multiple fields. Since
manually setting these expectations would add a lot of additional code,
the introduction of `test::FormFieldDescriptionEq` is proposed,
implemented in a separate CL.

Bug: 41483772

Change-Id: I814f64ff6f8bfc959ccde1b22ee88a37b35802d1

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7912949

Reviewed-by: Norge Vizcay <viz...@google.com>

Commit-Queue: Tamino Bauknecht <tam...@chromium.org>

Reviewed-by: Jihad Hanna <jihad...@google.com>

Cr-Commit-Position: refs/heads/main@{#1646677}

Files:

M chrome/renderer/autofill/form_autofill_browsertest.cc

Change size: XL

Delta: 1 file changed, 661 insertions(+), 954 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Norge Vizcay, +1 by Jihad Hanna

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

autofill: Refactor EXPECT_FORM_FIELD_DATA_EQUALS [chromium/src : main]

Norge Vizcay (Gerrit)

Norge Vizcay added 3 comments

Related details

Tamino Bauknecht (Gerrit)

Tamino Bauknecht added 4 comments

Related details

Jihad Hanna (Gerrit)

Jihad Hanna voted Code-Review+1

Related details

Norge Vizcay (Gerrit)

Norge Vizcay voted and added 2 comments

Votes added by Norge Vizcay

2 comments

Related details

Tamino Bauknecht (Gerrit)

Tamino Bauknecht added 6 comments

Related details

Norge Vizcay (Gerrit)

Norge Vizcay voted and added 2 comments

Votes added by Norge Vizcay

2 comments

Related details

Jihad Hanna (Gerrit)

Jihad Hanna voted Code-Review+1

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information