Autofill event - web exposed parts [chromium/src : main]
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
Hey Christoph!
Took a stab at the Blink side of things for the autofill event, and it seems like it's working! :) I'd appreciate your review on this, to make sure there's nothing I missed.
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 9 comments![]( "Open in Gerrit")
form_util::DispatchAutofillEvent(document, fields, *fill_id,
supports_refill);IIRC we don't want to fire the event for Undo Autofill. If so, this needs to be wrapped in `if (action_type == mojom::FormActionType::kFill)`.
if (auto* autofill_driver = unsafe_autofill_driver()) {
autofill_driver->RequestRefill(FillId(fill_id));
}Should this call `AutofillAgent::RequestRefill(FillId(fill_id), some_callback)`? Don't we need the callback in JavaScript to implement `await e.refill()`?
Imagine JavaScript code like
```
document.addEventListener('autofill', function(e) {
e.refill().then(foo).reject(bar);
};
```
would need to translate to C++ code like
```
autofill_agent.TriggerReparse(
fill_id, base::BindOnce([](bool success) {
if (success) {
foo();
} else {
bar();
}
});
```
const base::UnguessableToken& fill_id,nit: `FillId`?
void WebDocument::TriggerAutofillEvent(nit: `Dispatch`?
for (auto& pair : field_data) {
AutofillFieldData* data = AutofillFieldData::Create();
data->setField(pair.first.Get());
data->setValue(std::move(pair.second));
field_data_.push_back(data);
}opt: Should we use the [projection-based initialization](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/heap/collection_support/heap_vector.h;l=79-80;drc=26177a9b97540a3b036577b9489b70022c4c8a0c)?
return nullptr;For my own education: In JavaScript, that translate to `e.refill === null`, right?
RefillFunction* function =
MakeGarbageCollected<RefillFunction>(const_cast<AutofillEvent*>(this));
v8::Local<v8::Function> v8_function = function->ToV8Function(script_state);
refill_function_ = V8VoidFunction::Create(v8_function);Related to my other comment in `AutofillAgent::RequestRefill()`: Should `refill_function_` return a `Promise<void>`? (I don't know what that looks like in C++.)
RuntimeEnabled=AutofillEventDoy ou have thoughts on whether we should also introduce a `base::Feature` for `AutofillEvent` in `runtime_enabled_features.json5`? Would the rollout work only by origin trial or also a Finch study?
I assume OT is the way to go but I don't have first-hand experience with OTs.
If we go with OT, I'm wondering if we need an additional `base::Feature` for extra safety?
RuntimeEnabled=AutofillEventFor my education: Is that a base::Feature that must be enabled for `AutofillEvent` to be exposed?
Related details
- Yoav Weiss (@Shopify)
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 8 comments![]( "Open in Gerrit")
form_util::DispatchAutofillEvent(document, fields, *fill_id,
supports_refill);IIRC we don't want to fire the event for Undo Autofill. If so, this needs to be wrapped in `if (action_type == mojom::FormActionType::kFill)`.
Done
if (auto* autofill_driver = unsafe_autofill_driver()) {
autofill_driver->RequestRefill(FillId(fill_id));
}Should this call `AutofillAgent::RequestRefill(FillId(fill_id), some_callback)`? Don't we need the callback in JavaScript to implement `await e.refill()`?
Imagine JavaScript code like
```
document.addEventListener('autofill', function(e) {
e.refill().then(foo).reject(bar);
};
```
would need to translate to C++ code like
```
autofill_agent.TriggerReparse(
fill_id, base::BindOnce([](bool success) {
if (success) {
foo();
} else {
bar();
}
});
```
Done
const base::UnguessableToken& fill_id,Yoav Weiss (@Shopify)nit: `FillId`?
The style guide suggests it should be snake_case: https://google.github.io/styleguide/cppguide.html#Variable_Names
void WebDocument::TriggerAutofillEvent(Yoav Weiss (@Shopify)nit: `Dispatch`?
Done
For my own education: In JavaScript, that translate to `e.refill === null`, right?
I believe so, yeah.
RefillFunction* function =
MakeGarbageCollected<RefillFunction>(const_cast<AutofillEvent*>(this));
v8::Local<v8::Function> v8_function = function->ToV8Function(script_state);
refill_function_ = V8VoidFunction::Create(v8_function);Related to my other comment in `AutofillAgent::RequestRefill()`: Should `refill_function_` return a `Promise<void>`? (I don't know what that looks like in C++.)
Revamped
Doy ou have thoughts on whether we should also introduce a `base::Feature` for `AutofillEvent` in `runtime_enabled_features.json5`? Would the rollout work only by origin trial or also a Finch study?
I assume OT is the way to go but I don't have first-hand experience with OTs.
If we go with OT, I'm wondering if we need an additional `base::Feature` for extra safety?
For my education: Is that a base::Feature that must be enabled for `AutofillEvent` to be exposed?
It's a runtimeEnabledFeature (that has a base::Feature equivalent) that must be enabled for AutofillEvent to be exposed.
Related details
- Christoph Schwering
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. |
Christoph Schwering (Gerrit)
Christoph Schwering added 2 comments![]( "Open in Gerrit")
I'll take a proper look later (but there's a build error at the moment anyway).
const base::UnguessableToken& fill_id,Yoav Weiss (@Shopify)nit: `FillId`?
The style guide suggests it should be snake_case: https://google.github.io/styleguide/cppguide.html#Variable_Names
I meant the type :)
I think within Autofill land where we have the strong alias, we should use `FillId` rather than `base::UnguessableToken`
Related details
- Christoph Schwering
- Yoav Weiss (@Shopify)
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. |
Christoph Schwering (Gerrit)
Christoph Schwering added 3 comments![]( "Open in Gerrit")
autofill_driver->RequestRefill(fill_id, std::move(callback));[`RequestRefill()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/autofill_agent.h;l=163-171;drc=e8436cccafc34d6e4d9c060f682345060579d208) already exists. It's intentionally implemented without a Mojo response callback.
if (action_type == mojom::FormActionType::kFill &&
action_persistence == mojom::ActionPersistence::kFill) {
pending_refills_.Fulfill(fill_id);
}Keep this?
RuntimeEnabled=AutofillEventYoav Weiss (@Shopify)Doy ou have thoughts on whether we should also introduce a `base::Feature` for `AutofillEvent` in `runtime_enabled_features.json5`? Would the rollout work only by origin trial or also a Finch study?
I assume OT is the way to go but I don't have first-hand experience with OTs.
If we go with OT, I'm wondering if we need an additional `base::Feature` for extra safety?
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=667?q=AutofillEvent%20-f:test%20-f:out&ss=chromium%2Fchromium%2Fsrc generates a base feature automatically.
- Yoav Weiss (@Shopify)
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 4 comments![]( "Open in Gerrit")
autofill_driver->RequestRefill(fill_id, std::move(callback));[`RequestRefill()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/autofill_agent.h;l=163-171;drc=e8436cccafc34d6e4d9c060f682345060579d208) already exists. It's intentionally implemented without a Mojo response callback.
Removed the spurious bits. Apologies!
if (action_type == mojom::FormActionType::kFill &&
action_persistence == mojom::ActionPersistence::kFill) {
pending_refills_.Fulfill(fill_id);
}Yoav Weiss (@Shopify)Keep this?
Done
const base::UnguessableToken& fill_id,Yoav Weiss (@Shopify)nit: `FillId`?
Christoph SchweringThe style guide suggests it should be snake_case: https://google.github.io/styleguide/cppguide.html#Variable_Names
I meant the type :)
I think within Autofill land where we have the strong alias, we should use `FillId` rather than `base::UnguessableToken`
Got it!
RuntimeEnabled=AutofillEventYoav Weiss (@Shopify)Doy ou have thoughts on whether we should also introduce a `base::Feature` for `AutofillEvent` in `runtime_enabled_features.json5`? Would the rollout work only by origin trial or also a Finch study?
I assume OT is the way to go but I don't have first-hand experience with OTs.
If we go with OT, I'm wondering if we need an additional `base::Feature` for extra safety?
Christoph Schweringhttps://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=667?q=AutofillEvent%20-f:test%20-f:out&ss=chromium%2Fchromium%2Fsrc generates a base feature automatically.
Thanks! What do you think about Finch vs OT?
I think an OT makes more sense, given that this is an API developers need to deliberately use.
Related details
- Christoph Schwering
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
for (auto& pair : field_data) {
AutofillFieldData* data = AutofillFieldData::Create();
data->setField(pair.first.Get());
data->setValue(std::move(pair.second));
field_data_.push_back(data);
}opt: Should we use the [projection-based initialization](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/heap/collection_support/heap_vector.h;l=79-80;drc=26177a9b97540a3b036577b9489b70022c4c8a0c)?
Done
Christoph Schwering (Gerrit)
Christoph Schwering added 6 comments![]( "Open in Gerrit")
THanks! Just a couple of nits.
autofill_driver->RequestRefill(FillId(fill_id));nit: just `fill_id`
if (action_type == mojom::FormActionType::kFill) {Should we add `&& base::FeatureList::IsEnabled(blink::features::kAutofillEvent)`?
std::vector<std::pair<WebFormControlElement, WebString>> field_data;nit: `field_data` sounds a lot like `FormFieldData`.
Should we rename the `field_data` to `autofill_values` here and all the way to `AutofillEvent`?
That'd match the name `AutofillEvent::autofillValues()`.
for (const auto& field : fields) {nit: `const FormFieldData::FillData`
RuntimeEnabled=AutofillEventYoav Weiss (@Shopify)Doy ou have thoughts on whether we should also introduce a `base::Feature` for `AutofillEvent` in `runtime_enabled_features.json5`? Would the rollout work only by origin trial or also a Finch study?
I assume OT is the way to go but I don't have first-hand experience with OTs.
If we go with OT, I'm wondering if we need an additional `base::Feature` for extra safety?
Christoph Schweringhttps://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=667?q=AutofillEvent%20-f:test%20-f:out&ss=chromium%2Fchromium%2Fsrc generates a base feature automatically.
Yoav Weiss (@Shopify)Thanks! What do you think about Finch vs OT?
I think an OT makes more sense, given that this is an API developers need to deliberately use.
- Yoav Weiss (@Shopify)
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 4 comments![]( "Open in Gerrit")
autofill_driver->RequestRefill(FillId(fill_id));Yoav Weiss (@Shopify)nit: just `fill_id`
Done
if (action_type == mojom::FormActionType::kFill) {Should we add `&& base::FeatureList::IsEnabled(blink::features::kAutofillEvent)`?
There's already a flag check further down, but we can add one here as well.
std::vector<std::pair<WebFormControlElement, WebString>> field_data;nit: `field_data` sounds a lot like `FormFieldData`.
Should we rename the `field_data` to `autofill_values` here and all the way to `AutofillEvent`?
That'd match the name `AutofillEvent::autofillValues()`.
Done
for (const auto& field : fields) {Yoav Weiss (@Shopify)nit: `const FormFieldData::FillData`
Done
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
ASSERT_TRUE(manager->WaitForAutofillFill(/*num_expected_fills=*/2));When I run the test `AutofillEventHandlerBrowserTest.CompleteAutofillFlow` with only one CPU core, this assertion times out -- even when I increase [these timeouts](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/common/autofill_constants.h;l=89-98;drc=8b920155b9142d8e79e07083f47ed1b4e2dd1491):
```
#14 0x558445e19468 autofill::TestAutofillManagerWaiter::Wait() [../../components/autofill/core/browser/foundations/test_autofill_manager_waiter.cc:365:22]
#15 0x55843423fec8 autofill::AutofillEventHandlerBrowserTest::TestAutofillManager::WaitForAutofillFill() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:59:36]
#16 0x55843423b5fb autofill::AutofillEventHandlerBrowserTest::NavigateAndTriggerAutofill() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:158:26]
#17 0x558434237d92 autofill::AutofillEventHandlerBrowserTest_CompleteAutofillFlow_Test::RunTestOnMainThread() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:186:3]
```
For some reason the [diagnostic message](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/test_autofill_manager_waiter.cc;l=360;drc=3d6df2f5cdb862de83bce3b4a0dea62311701090) is missing. The test isn't mocking time, is it? (In that case, we don't have those diagnostics for a [reason](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/test_autofill_manager_waiter.h;l=361-362;drc=4d61c530e1e440d78af6b82da0bb2472f06576a7).)
I've debugged it a little and suspect that the bot is too slow to reparse the form within [these 200 ms](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler.cc;l=66;drc=5fe91acd428f2c0b9ca9af8a2ec0d72ef28d3d4b). That's plausible because parsing happens on a low-priority sequence runner.
We could address this as follows:
1. Wait for `OnFormsSeen()` (as you do now)
2. Wait for first `OnDidAutofill()`
3. Wait for another `OnFormsSeen()` triggered by the DOM
4. Only now let JavaScript call `RequestRefill()`.
5. Wait for the refill.
---
Besides that, you may also want to do [this](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/autofill/autofill_across_iframes_browsertest.cc;l=85-86;drc=4d61c530e1e440d78af6b82da0bb2472f06576a7) to effectively disable the first two of [these two](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/common/autofill_constants.h;l=89-98;drc=8b920155b9142d8e79e07083f47ed1b4e2dd1491) timeouts. We don't have anything do disable the third because it's in the renderer process.
Related details
- Yoav Weiss (@Shopify)
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. |
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
ASSERT_TRUE(manager->WaitForAutofillFill(/*num_expected_fills=*/2));When I run the test `AutofillEventHandlerBrowserTest.CompleteAutofillFlow` with only one CPU core, this assertion times out -- even when I increase [these timeouts](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/common/autofill_constants.h;l=89-98;drc=8b920155b9142d8e79e07083f47ed1b4e2dd1491):
```
#14 0x558445e19468 autofill::TestAutofillManagerWaiter::Wait() [../../components/autofill/core/browser/foundations/test_autofill_manager_waiter.cc:365:22]
#15 0x55843423fec8 autofill::AutofillEventHandlerBrowserTest::TestAutofillManager::WaitForAutofillFill() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:59:36]
#16 0x55843423b5fb autofill::AutofillEventHandlerBrowserTest::NavigateAndTriggerAutofill() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:158:26]
#17 0x558434237d92 autofill::AutofillEventHandlerBrowserTest_CompleteAutofillFlow_Test::RunTestOnMainThread() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:186:3]
```For some reason the [diagnostic message](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/test_autofill_manager_waiter.cc;l=360;drc=3d6df2f5cdb862de83bce3b4a0dea62311701090) is missing. The test isn't mocking time, is it? (In that case, we don't have those diagnostics for a [reason](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/test_autofill_manager_waiter.h;l=361-362;drc=4d61c530e1e440d78af6b82da0bb2472f06576a7).)
I've debugged it a little and suspect that the bot is too slow to reparse the form within [these 200 ms](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler.cc;l=66;drc=5fe91acd428f2c0b9ca9af8a2ec0d72ef28d3d4b). That's plausible because parsing happens on a low-priority sequence runner.
We could address this as follows:
1. Wait for `OnFormsSeen()` (as you do now)
2. Wait for first `OnDidAutofill()`
3. Wait for another `OnFormsSeen()` triggered by the DOM
4. Only now let JavaScript call `RequestRefill()`.
5. Wait for the refill.---
Besides that, you may also want to do [this](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/autofill/autofill_across_iframes_browsertest.cc;l=85-86;drc=4d61c530e1e440d78af6b82da0bb2472f06576a7) to effectively disable the first two of [these two](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/common/autofill_constants.h;l=89-98;drc=8b920155b9142d8e79e07083f47ed1b4e2dd1491) timeouts. We don't have anything do disable the third because it's in the renderer process.
When I increase the 200 ms delay to 4 s, the test runs through on my machine with a single CPU core. I guess that supports the hypothesis in the previous comment.
Related details
- Yoav Weiss (@Shopify)
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. |
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
ASSERT_TRUE(manager->WaitForAutofillFill(/*num_expected_fills=*/2));Christoph SchweringWhen I run the test `AutofillEventHandlerBrowserTest.CompleteAutofillFlow` with only one CPU core, this assertion times out -- even when I increase [these timeouts](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/common/autofill_constants.h;l=89-98;drc=8b920155b9142d8e79e07083f47ed1b4e2dd1491):
```
#14 0x558445e19468 autofill::TestAutofillManagerWaiter::Wait() [../../components/autofill/core/browser/foundations/test_autofill_manager_waiter.cc:365:22]
#15 0x55843423fec8 autofill::AutofillEventHandlerBrowserTest::TestAutofillManager::WaitForAutofillFill() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:59:36]
#16 0x55843423b5fb autofill::AutofillEventHandlerBrowserTest::NavigateAndTriggerAutofill() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:158:26]
#17 0x558434237d92 autofill::AutofillEventHandlerBrowserTest_CompleteAutofillFlow_Test::RunTestOnMainThread() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:186:3]
```For some reason the [diagnostic message](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/test_autofill_manager_waiter.cc;l=360;drc=3d6df2f5cdb862de83bce3b4a0dea62311701090) is missing. The test isn't mocking time, is it? (In that case, we don't have those diagnostics for a [reason](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/test_autofill_manager_waiter.h;l=361-362;drc=4d61c530e1e440d78af6b82da0bb2472f06576a7).)
I've debugged it a little and suspect that the bot is too slow to reparse the form within [these 200 ms](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler.cc;l=66;drc=5fe91acd428f2c0b9ca9af8a2ec0d72ef28d3d4b). That's plausible because parsing happens on a low-priority sequence runner.
We could address this as follows:
1. Wait for `OnFormsSeen()` (as you do now)
2. Wait for first `OnDidAutofill()`
3. Wait for another `OnFormsSeen()` triggered by the DOM
4. Only now let JavaScript call `RequestRefill()`.
5. Wait for the refill.---
Besides that, you may also want to do [this](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/autofill/autofill_across_iframes_browsertest.cc;l=85-86;drc=4d61c530e1e440d78af6b82da0bb2472f06576a7) to effectively disable the first two of [these two](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/common/autofill_constants.h;l=89-98;drc=8b920155b9142d8e79e07083f47ed1b4e2dd1491) timeouts. We don't have anything do disable the third because it's in the renderer process.
When I increase the 200 ms delay to 4 s, the test runs through on my machine with a single CPU core. I guess that supports the hypothesis in the previous comment.
s/triggered by the DOM/triggered by JavaScript updating the DOM/
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
ASSERT_TRUE(manager->WaitForAutofillFill(/*num_expected_fills=*/2));Christoph SchweringWhen I run the test `AutofillEventHandlerBrowserTest.CompleteAutofillFlow` with only one CPU core, this assertion times out -- even when I increase [these timeouts](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/common/autofill_constants.h;l=89-98;drc=8b920155b9142d8e79e07083f47ed1b4e2dd1491):
```
#14 0x558445e19468 autofill::TestAutofillManagerWaiter::Wait() [../../components/autofill/core/browser/foundations/test_autofill_manager_waiter.cc:365:22]
#15 0x55843423fec8 autofill::AutofillEventHandlerBrowserTest::TestAutofillManager::WaitForAutofillFill() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:59:36]
#16 0x55843423b5fb autofill::AutofillEventHandlerBrowserTest::NavigateAndTriggerAutofill() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:158:26]
#17 0x558434237d92 autofill::AutofillEventHandlerBrowserTest_CompleteAutofillFlow_Test::RunTestOnMainThread() [../../chrome/browser/autofill/autofill_event_handler_browsertest.cc:186:3]
```For some reason the [diagnostic message](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/test_autofill_manager_waiter.cc;l=360;drc=3d6df2f5cdb862de83bce3b4a0dea62311701090) is missing. The test isn't mocking time, is it? (In that case, we don't have those diagnostics for a [reason](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/test_autofill_manager_waiter.h;l=361-362;drc=4d61c530e1e440d78af6b82da0bb2472f06576a7).)
I've debugged it a little and suspect that the bot is too slow to reparse the form within [these 200 ms](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler.cc;l=66;drc=5fe91acd428f2c0b9ca9af8a2ec0d72ef28d3d4b). That's plausible because parsing happens on a low-priority sequence runner.
We could address this as follows:
1. Wait for `OnFormsSeen()` (as you do now)
2. Wait for first `OnDidAutofill()`
3. Wait for another `OnFormsSeen()` triggered by the DOM
4. Only now let JavaScript call `RequestRefill()`.
5. Wait for the refill.---
Besides that, you may also want to do [this](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/autofill/autofill_across_iframes_browsertest.cc;l=85-86;drc=4d61c530e1e440d78af6b82da0bb2472f06576a7) to effectively disable the first two of [these two](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/common/autofill_constants.h;l=89-98;drc=8b920155b9142d8e79e07083f47ed1b4e2dd1491) timeouts. We don't have anything do disable the third because it's in the renderer process.
Christoph SchweringWhen I increase the 200 ms delay to 4 s, the test runs through on my machine with a single CPU core. I guess that supports the hypothesis in the previous comment.
s/triggered by the DOM/triggered by JavaScript updating the DOM/
Thanks!!
Added waits. Now waiting to see if the bots are happy 😊
Related details
- Christoph Schwering
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yoav Weiss (@Shopify)
| Commit-Queue | +1 |
1 comment
- Christoph Schwering
- Yoav Weiss (@Shopify)
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
OK, got them passing, but I'm not 100% confident the flakiness is over..
Related details
- Christoph Schwering
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. |
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 3 comments![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
3 comments
LGTM. I can try to look more into the flakiness locally later today.
(As discussed offline:) The test is still flaky on my machine but I haven't added LOG statements yet.
// for (const FormFieldData::FillData& autofill_value : autofill_values) {nit: remove?
Related details
- Yoav Weiss (@Shopify)
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
// for (const FormFieldData::FillData& autofill_value : autofill_values) {Yoav Weiss (@Shopify)nit: remove?
Done
Related details
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. |
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 2 comments![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
2 comments
ASSERT_TRUE(manager->WaitForAutofillFill(/*num_expected_fills=*/1));Re test flakiness:
I hadn't realized that the dynamic modification of the form happens asynchronously after the Autofill.
Since it happens asynchronously, the autofill triggers not one but *two* `AutofillManager::OnFormsSeen()` events:
- The first is triggered synchronously in `AutofillAgent::ApplyFieldsAction()`; at the time the form has only 8 fields.
- The second is triggered (roughly 100 ms later) when the form is modified dynamically; now the form has 11 elements.
To fix that, we have two options, I think:
- Option 1: We make the dynamic change synchronous. Then only on `OnFormsSeen()` call will happen.
- Option 2: We add another `manager->WaitForAutofillFill(/*num_expected_fills=*/1)` here.
No strong preference on which option. I guess I'd go with Option 2 and change the code comment
```
// Wait for the form rescan after DOM modification.
```
to something like
```
// Wait for the form rescan after the DOM modification that happens
// asynchronously after the autofill.
```
//
// We must wait until the FormStructure in the cache actually contains the
// newly added fields (11 total: 8 original + 3 new). Simply waiting for
// OnAfterFormsSeen is insufficient because:
// 1. Form extraction on the renderer is throttled (100ms delay)
// 2. Form parsing on the browser is asynchronous
// 3. There may be multiple FormsSeen events in flight, and we might catch
// a stale one before the DOM mutation's form parsing completes
const size_t kExpectedFieldCount = 11;
while (true) {
ASSERT_TRUE(manager->WaitForFormsSeen(/*min_num_awaited_calls=*/1));
const std::vector<const FormStructure*> structures =
test_api(*manager).form_structures();
if (!structures.empty() &&
structures.front()->field_count() >= kExpectedFieldCount) {
break;
}
// The form parsing hasn't caught up yet. Wait for the next FormsSeen.
}I believe that fixes the flakiness, too. But I'd rather go with Option 1 or 2 mentioned above to avoid the loop and to make it more explicit which `OnFormsSeen()` events happen and why.
Related details
- Yoav Weiss (@Shopify)
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. |