Norge Vizcay (Gerrit)

unread,

Jun 12, 2026, 5:34:47 AM (4 days ago) Jun 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 voted and added 2 comments

Votes added by Norge Vizcay

Code-Review

+1

2 comments

File components/autofill/core/common/test_utils/autofill_form_test_utils.cc

Line 22, Patchset 10 (Latest): add_member_checker("host_frame", &FormFieldData::host_frame,

Norge Vizcay . unresolved

I think we are missing:

placeholder_attribute
pattern
css_classes

Can you double check?

Line 74, Patchset 10 (Latest):

    add_member_checker(
        "check_status", &FormFieldData::check_status,
        std::optional{
            *expected.checked
                ? FormFieldData::CheckStatus::kChecked
                : FormFieldData::CheckStatus::kCheckableButUnchecked});

Norge Vizcay . unresolved

Opt:

Should this be
```
matchers.push_back(testing::Property(
          "check_status", &FormFieldData::check_status,
          *expected.checked ? FormFieldData::CheckStatus::kChecked
                            : FormFieldData::CheckStatus::kCheckableButUnchecked));
```

Converting to optional here a non-null value doesn't bring a lot of value.

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

Norge Vizcay (Gerrit)

unread,

Jun 12, 2026, 5:36:02 AM (4 days ago) Jun 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 1 comment

Patchset-level comments

File-level comment, Patchset 8:

Tamino Bauknecht . resolved

@viz...@google.com This one introduces the matcher used by https://crrev.com/c/7912949.

Norge Vizcay

Acknowledged

satisfied_requirement

unsatisfied_requirement

open

diffy

Tamino Bauknecht (Gerrit)

unread,

Jun 12, 2026, 6:21:06 AM (4 days ago) Jun 12

to Norge Vizcay, 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 Jihad Hanna and Norge Vizcay

Tamino Bauknecht added 2 comments

File components/autofill/core/common/test_utils/autofill_form_test_utils.cc

Line 22, Patchset 10: add_member_checker("host_frame", &FormFieldData::host_frame,

Norge Vizcay . unresolved

I think we are missing:
placeholder_attribute
pattern
css_classes

Can you double check?

Tamino Bauknecht

Done, good catch - they were added in a rebase and I forgot to update this here :) I also reordered the statements to match the order in the struct definition which should make these mistakes easier to see.

I added `LINT.IfChange` markers like in `FormDataEq()` to avoid this issue in the future. I've never used them before, can you check if I used them correctly?

Line 74, Patchset 10: add_member_checker(


        "check_status", &FormFieldData::check_status,
        std::optional{
            *expected.checked
                ? FormFieldData::CheckStatus::kChecked
                : FormFieldData::CheckStatus::kCheckableButUnchecked});

Norge Vizcay . unresolved

Opt:

Should this be
```
matchers.push_back(testing::Property(
          "check_status", &FormFieldData::check_status,
          *expected.checked ? FormFieldData::CheckStatus::kChecked
                            : FormFieldData::CheckStatus::kCheckableButUnchecked));
```

Converting to optional here a non-null value doesn't bring a lot of value.

Tamino Bauknecht

Sorry, I agree that my hack to reuse the function wasn't great. What do you think of the new version using `and_then`? If you think this is also unnecessarily hard to read, I will simply use your snippet :)

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 12, 2026, 6:35:01 AM (4 days ago) Jun 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 Jihad Hanna and Tamino Bauknecht

Norge Vizcay added 4 comments

File components/autofill/core/common/test_utils/autofill_form_test_utils.h

Line 100, Patchset 11 (Latest): // LINT.ThenChange(autofill_form_test_utils.cc:FormFieldDescriptionEq)

Norge Vizcay . unresolved

We're generally required to use the full path.

File components/autofill/core/common/test_utils/autofill_form_test_utils.cc

Line 22, Patchset 10: add_member_checker("host_frame", &FormFieldData::host_frame,

Norge Vizcay . resolved

I think we are missing:
placeholder_attribute
pattern
css_classes

Can you double check?

Tamino Bauknecht

Done, good catch - they were added in a rebase and I forgot to update this here :) I also reordered the statements to match the order in the struct definition which should make these mistakes easier to see.
I added `LINT.IfChange` markers like in `FormDataEq()` to avoid this issue in the future. I've never used them before, can you check if I used them correctly?

Norge Vizcay

+1 on using a LINT rule. Left a couple of comments. Thanks for putting this together!

Line 74, Patchset 10:

    add_member_checker(
        "check_status", &FormFieldData::check_status,
        std::optional{
            *expected.checked
                ? FormFieldData::CheckStatus::kChecked
                : FormFieldData::CheckStatus::kCheckableButUnchecked});

Norge Vizcay . resolved

Opt:

Should this be
```
matchers.push_back(testing::Property(
          "check_status", &FormFieldData::check_status,
          *expected.checked ? FormFieldData::CheckStatus::kChecked
                            : FormFieldData::CheckStatus::kCheckableButUnchecked));
```

Converting to optional here a non-null value doesn't bring a lot of value.

Tamino Bauknecht

Sorry, I agree that my hack to reuse the function wasn't great. What do you think of the new version using `and_then`? If you think this is also unnecessarily hard to read, I will simply use your snippet :)

Norge Vizcay

Yours works for me :)

Line 88, Patchset 11 (Latest): // LINT.ThenChange(autofill_form_test_utils.h:FieldDescriptionDataMembers)

Norge Vizcay . unresolved

We're generally required to use the full path.

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 12, 2026, 9:39:16 AM (4 days ago) Jun 12

to Norge Vizcay, 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 Jihad Hanna and Norge Vizcay

Tamino Bauknecht added 2 comments

File components/autofill/core/common/test_utils/autofill_form_test_utils.h

Line 100, Patchset 11: // LINT.ThenChange(autofill_form_test_utils.cc:FormFieldDescriptionEq)

Norge Vizcay . resolved

We're generally required to use the full path.

Tamino Bauknecht

Done, good to know. Thanks :)

File components/autofill/core/common/test_utils/autofill_form_test_utils.cc

Line 88, Patchset 11: // LINT.ThenChange(autofill_form_test_utils.h:FieldDescriptionDataMembers)

Norge Vizcay . resolved

We're generally required to use the full path.

Tamino Bauknecht

Done

Open in Gerrit

Related details

Attention is currently required from:

Jihad Hanna
Norge Vizcay

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 12, 2026, 7:16:23 PM (4 days ago) Jun 12

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 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

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 14, 2026, 8:38:00 AM (2 days ago) Jun 14

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 Tamino Bauknecht

Norge Vizcay 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:56:38 AM (yesterday) Jun 15

to Tamino Bauknecht, Norge Vizcay, Jihad Hanna, 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: Add test::FormFieldDescriptionEq for partial matching

This change allows users of the matcher to perform partial comparisons
to an expected `FormFieldData`. In many cases a test case only wants to
verify that a couple of members of `FormFieldData` is set accordingly.
At the moment, this requires a long call to `test::GetFormFieldData()`
to fully set up the expected object and then `test::FormFieldDataEq` is
used to compare the actual and expected objects. While
`test::WithoutUnserializedData` already enables partial comparisons, it
is very restrictive and only cancels out a fixed set of members.

Concretely, `renderer_id`, `bounds`, `should_autocomplete` and some
other fields can differ in a regular test case from the default of
`test::GetFormFieldData()` which would cause multiple lines of
additional code for each expectation. At the same time, the test might
not even have the intent to check these, but needs to add the code
anyway.

It utilizes that all fields in `FieldDescription` are optional and only
compares the explicitly set fields with a `FormFieldData`.

Bug: 41483772

Change-Id: Ia93f827bf2fcd06b78a97a67d7ca6a54242d54b6

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

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

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

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

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

Files:

M components/autofill/core/common/BUILD.gn
A components/autofill/core/common/test_utils/autofill_form_test_utils.cc
M components/autofill/core/common/test_utils/autofill_form_test_utils.h

Change size: M

Delta: 3 files changed, 101 insertions(+), 0 deletions(-)

Branch: refs/heads/main

Submit Requirements:

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

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: Add test::FormFieldDescriptionEq for partial matching [chromium/src : main]

Norge Vizcay (Gerrit)

Norge Vizcay voted and added 2 comments

Votes added by Norge Vizcay

2 comments

Related details

Norge Vizcay (Gerrit)

Norge Vizcay added 1 comment

Tamino Bauknecht (Gerrit)

Tamino Bauknecht added 2 comments

Related details

Norge Vizcay (Gerrit)

Norge Vizcay added 4 comments

Related details

Tamino Bauknecht (Gerrit)

Tamino Bauknecht added 2 comments

Related details

Jihad Hanna (Gerrit)

Jihad Hanna voted Code-Review+1

Related details

Norge Vizcay (Gerrit)

Norge Vizcay voted Code-Review+1

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information