Norge Vizcay (Gerrit)

unread,

Jun 12, 2026, 5:59:10 AM (12 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 1 comment

Votes added by Norge Vizcay

Code-Review

+1

1 comment

File chrome/renderer/autofill/form_autofill_browsertest.cc

Line 3953, Patchset 10 (Latest):


  // Find the newly-filled form that contains the input element.
  FormData form2 = FindForm(input_element);
  EXPECT_EQ(u"TestForm", form2.name());
  EXPECT_EQ(GURL("http://abc.com"), form2.action());

  const std::vector<FormFieldData>& fields2 = form2.fields();
  ASSERT_EQ(6U, fields2.size());

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

  expected.set_id_attribute(u"firstname");
  expected.set_name(expected.id_attribute());
  expected.set_value(u"Wyatt");
  expected.set_label(u"First Name");
  expected.set_placeholder(u"First Name");
  expected.set_is_autofilled_according_to_renderer(true);
  EXPECT_FORM_FIELD_DATA_EQUALS(expected, fields2[0]);

  // The last name field is not filled, because there is a value in it.
  expected.set_id_attribute(u"lastname");
  expected.set_name(expected.id_attribute());
  expected.set_value(u"Earp");
  expected.set_label(u"Last Name");
  expected.set_placeholder(u"Last Name");
  expected.set_is_autofilled_according_to_renderer(false);
  EXPECT_FORM_FIELD_DATA_EQUALS(expected, fields2[1]);

  expected.set_id_attribute(u"phone");
  expected.set_name(expected.id_attribute());
  expected.set_value(u"888-123-4567");
  expected.set_label(u"Phone");
  expected.set_placeholder(u"Phone");
  expected.set_is_autofilled_according_to_renderer(true);
  EXPECT_FORM_FIELD_DATA_EQUALS(expected, fields2[2]);

  expected.set_id_attribute(u"cc");
  expected.set_name(expected.id_attribute());
  expected.set_value(u"1111-2222-3333-4444");
  expected.set_label(u"Credit Card Number");
  expected.set_placeholder(u"Credit Card Number");
  expected.set_is_autofilled_according_to_renderer(true);
  EXPECT_FORM_FIELD_DATA_EQUALS(expected, fields2[3]);

  expected.set_id_attribute(u"city");
  expected.set_name(expected.id_attribute());
  expected.set_value(u"Montreal");
  expected.set_label(u"City");
  expected.set_placeholder(u"City");
  expected.set_is_autofilled_according_to_renderer(true);
  EXPECT_FORM_FIELD_DATA_EQUALS(expected, fields2[4]);

  expected.set_form_control_type(FormControlType::kSelectOne);
  expected.set_id_attribute(u"state");
  expected.set_name_attribute(u"state");
  expected.set_name(expected.name_attribute());
  expected.set_value(u"AA");
  expected.set_label(u"State");
  expected.set_placeholder(u"State");
  expected.set_is_autofilled_according_to_renderer(true);
  expected.set_max_length(0);
  EXPECT_FORM_FIELD_DATA_EQUALS(expected, fields2[5]);

Norge Vizcay . unresolved

I agree that inlining makes sense, but i think we should follow-up and refactor these tests a bit. There is a lot of repeated logic on these expectations. We could introduce a new util in the test file. Something like:

```
auto verify_field = [](const FormFieldData& actual, 
                       const std::u16string& id, 
                       const std::u16string& val, 
                       const std::u16string& label, 
                       bool is_autofilled) {
...
}                       
```

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, 6:00:56 AM (12 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

File chrome/renderer/autofill/form_autofill_browsertest.cc

Norge Vizcay . resolved

I agree that inlining makes sense, but i think we should follow-up and refactor these tests a bit. There is a lot of repeated logic on these expectations. We could introduce a new util in the test file. Something like:

```
auto verify_field = [](const FormFieldData& actual, 
                       const std::u16string& id, 
                       const std::u16string& val, 
                       const std::u16string& label, 
                       bool is_autofilled) {
...
}                       
```

Norge Vizcay

Nvm - i see this is address in a follow-up 😊

satisfied_requirement

unsatisfied_requirement

open

diffy

Tamino Bauknecht (Gerrit)

unread,

Jun 12, 2026, 7:20:54 AM (12 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

Tamino Bauknecht added 1 comment

Patchset-level comments

File-level comment, Patchset 8:

Tamino Bauknecht . resolved

Hey Norge, this change should be independent of the other changes in this CL chain, so you can also review it last

Tamino Bauknecht

Mark as resolved

Open in Gerrit

Related details

Attention set is empty

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:57:58 AM (9 days ago) Jun 15

to Tamino Bauknecht, Norge Vizcay, Daniel Cheng, Jihad Hanna, 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

Unreviewed changes

10 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.

Change information

Commit message:

autofill: Inline one-use functions in form_autofill_browsertest.cc

This inlines the following functions because they are only used once
anyway:

* `TestFillFormAndModifyValues`
* `TestFillFormAndModifyInitiatingValue`
* `TestFillFormJSModifiesUserInputValue`

Bug: 41483772

Change-Id: Id99916acb290ff51c4c9300659660e320d75dcca

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

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

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

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

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

Files:

M chrome/renderer/autofill/form_autofill_browsertest.cc

Change size: L

Delta: 1 file changed, 316 insertions(+), 416 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: Inline one-use functions in form_autofill_browsertest.cc [chromium/src : main]

Norge Vizcay (Gerrit)

Norge Vizcay voted and added 1 comment

Votes added by Norge Vizcay

1 comment

Related details

Norge Vizcay (Gerrit)

Norge Vizcay added 1 comment

Tamino Bauknecht (Gerrit)

Tamino Bauknecht added 1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Unreviewed changes

Change information