Address autofill API - initial implementation [chromium/src : main]
Christoph Schwering (Gerrit)
Christoph Schwering added 7 comments![]( "Open in Gerrit")
void RetriggerAutofill(const FormData&) override;The implementation doesn't (and probably shouldn't) fall under Group 2a.
FormData lifted_form_data = Lift(*this, form_data);
WithNewVersion(lifted_form_data);
GetAutofillManager().RetriggerAutofill(lifted_form_data);Shouldn't this follow the pattern from the other events?
// TODO: A single autofill event can probably trigger filling fields in
// multiple forms. We should iterate the fields, and create a mapping of formsYes, that's correct.
// to field keys and values. Then we need to trigger the event on each one of
// the forms. If any of the forms had `waitForIt` called, we should call offI'm not sure about that. The browser has a different definition of form than the renderer.
When the user does an autofill operation, it concerns a single form according to the browser process's understanding of a form.
This autofill operation may lead to multiple ApplyFieldsAction() calls; at most one per document ([documented here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/autofill_driver_router.h)).
Each ApplyFieldsAction() call may fill form controls that are associated to different and/or no form elements. That's due to shadow DOM ([documented here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/README.md)).
// Attempts to refill the form that was changed dynamically. Should only be
// called if `FormFiller::ShouldTriggerRefill()` returns true.That's not the case anymore, is it?
virtual void TriggerRefill(const FormData&) = 0;Please name this `RetriggerAutofillImpl` (or `FooImpl` if the other function is `Foo`).
form_filler_->TriggerRefill(form,Shouldn't this call ScheduleRefill()?
Related details
- Daniel Kift
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
WebString key = control_element.NameForAutofill();Autofill doesn't (almost) only uses the name and id attributes for heuristics. This should probably be a `FieldRendererId`, which in the browser process must be lifted to `FieldGlobalId`.
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
void RetriggerAutofill(const FormData&) override;The implementation doesn't (and probably shouldn't) fall under Group 2a.
- Christoph Schwering
- Daniel Kift
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
virtual void TriggerRefill(const FormData&) = 0;Please name this `RetriggerAutofillImpl` (or `FooImpl` if the other function is `Foo`).
Done
Related details
- Christoph Schwering
- Daniel Kift
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
FormData lifted_form_data = Lift(*this, form_data);
WithNewVersion(lifted_form_data);
GetAutofillManager().RetriggerAutofill(lifted_form_data);Shouldn't this follow the pattern from the other events?
Done
WebString key = control_element.NameForAutofill();Autofill doesn't (almost) only uses the name and id attributes for heuristics. This should probably be a `FieldRendererId`, which in the browser process must be lifted to `FieldGlobalId`.
Passed a FieldRenderId (converted to a string). I'm not sure where I should Lift it on the browser side though, so I'd appreciate guidance there
// Attempts to refill the form that was changed dynamically. Should only be
// called if `FormFiller::ShouldTriggerRefill()` returns true.That's not the case anymore, is it?
Yup
Shouldn't this call ScheduleRefill()?
Done
Related details
- Christoph Schwering
- Daniel Kift
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
void RetriggerAutofillImpl(const FormData& form) override {}I need to actually implement (and test) this!!
Related details
- Christoph Schwering
- Daniel Kift
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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
return GetFormByRendererId(fields[0].host_form_id)I now see that I'm not getting the form, as the host_form_id here is 0. What's a good way to grab the right value? (this used to work, but I guess things changed)
Related details
- Christoph Schwering
- Daniel Kift
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
FormData lifted_form_data = Lift(*this, form);
WithNewVersion(lifted_form_data);This is done automatically by `RouteToManager()` [here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/browser/content_autofill_driver.cc;l=279;drc=2ca52c732385c3f987f8ad1140bbf5d7491bdfe4). I mean, you can just write `form` below, right?
WebString key = control_element.NameForAutofill();Yoav Weiss (@Shopify)Autofill doesn't (almost) only uses the name and id attributes for heuristics. This should probably be a `FieldRendererId`, which in the browser process must be lifted to `FieldGlobalId`.
Passed a FieldRenderId (converted to a string). I'm not sure where I should Lift it on the browser side though, so I'd appreciate guidance there
This happens in `ContentAutofillDriver::RouteToManager()` when it calls [this overload](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/browser/content_autofill_driver.cc;l=110-112;drc=2ca52c732385c3f987f8ad1140bbf5d7491bdfe4) of `Lift()`.
---
Btw my sentence in the original comment was butchered:
Autofill doesn't (almost) only uses the name and id attributes for heuristics.
should have been
Autofill uses the name and id attributes almost only for heuristics (that is, for determining field types).
Related details
- Daniel Kift
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
FormData lifted_form_data = Lift(*this, form);
WithNewVersion(lifted_form_data);This is done automatically by `RouteToManager()` [here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/browser/content_autofill_driver.cc;l=279;drc=2ca52c732385c3f987f8ad1140bbf5d7491bdfe4). I mean, you can just write `form` below, right?
Indeed! This fixed an issue I was seeing where the form ID was arriving as 0..
// to field keys and values. Then we need to trigger the event on each one of
// the forms. If any of the forms had `waitForIt` called, we should call offI'm not sure about that. The browser has a different definition of form than the renderer.
When the user does an autofill operation, it concerns a single form according to the browser process's understanding of a form.
This autofill operation may lead to multiple ApplyFieldsAction() calls; at most one per document ([documented here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/autofill_driver_router.h)).
Each ApplyFieldsAction() call may fill form controls that are associated to different and/or no form elements. That's due to shadow DOM ([documented here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/README.md)).
OK, so if the form is split to multiple renderers, ApplyFieldsAction would trigger the event on each of them? That makes sense..
WebString key = control_element.NameForAutofill();Yoav Weiss (@Shopify)Autofill doesn't (almost) only uses the name and id attributes for heuristics. This should probably be a `FieldRendererId`, which in the browser process must be lifted to `FieldGlobalId`.
Christoph SchweringPassed a FieldRenderId (converted to a string). I'm not sure where I should Lift it on the browser side though, so I'd appreciate guidance there
This happens in `ContentAutofillDriver::RouteToManager()` when it calls [this overload](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/browser/content_autofill_driver.cc;l=110-112;drc=2ca52c732385c3f987f8ad1140bbf5d7491bdfe4) of `Lift()`.
---
Btw my sentence in the original comment was butchered:
Autofill doesn't (almost) only uses the name and id attributes for heuristics.
should have been
Autofill uses the name and id attributes almost only for heuristics (that is, for determining field types).
Oh, there was some misunderstanding here. The key here is not passed to the browser, but is passed to the AutofillEvent as the value to communicate to web developers. As such I brought it back to be `NameForAutofill` which is more human readable than the ID
return GetFormByRendererId(fields[0].host_form_id)I now see that I'm not getting the form, as the host_form_id here is 0. What's a good way to grab the right value? (this used to work, but I guess things changed)
- Christoph Schwering
- Daniel Kift
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 2 comments![]( "Open in Gerrit")
// TODO: A single autofill event can probably trigger filling fields in
// multiple forms. We should iterate the fields, and create a mapping of formsYoav Weiss (@Shopify)Yes, that's correct.
Resolving
// to field keys and values. Then we need to trigger the event on each one of
// the forms. If any of the forms had `waitForIt` called, we should call offYoav Weiss (@Shopify)I'm not sure about that. The browser has a different definition of form than the renderer.
When the user does an autofill operation, it concerns a single form according to the browser process's understanding of a form.
This autofill operation may lead to multiple ApplyFieldsAction() calls; at most one per document ([documented here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/autofill_driver_router.h)).
Each ApplyFieldsAction() call may fill form controls that are associated to different and/or no form elements. That's due to shadow DOM ([documented here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/README.md)).
OK, so if the form is split to multiple renderers, ApplyFieldsAction would trigger the event on each of them? That makes sense..
Resolving (and removing the TODO)
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
Due to Gerrit complications (working on multiple machines, as well as multiple authors for the CL), I'll continue work on this in https://chromium-review.googlesource.com/c/chromium/src/+/6965138
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |