Address autofill API [chromium/src : main]
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
| 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")
This is identical to https://chromium-review.googlesource.com/c/chromium/src/+/6200613, with the following differences:
- The AutofillEvent now has a browser test
- Removed the extra `Lift` from ContentAutofillDriver
- Brought back `WebString key = control_element.NameForAutofill();` in form_autofill_util and removed the TODO (which was incorrect)
- Bring back the direct call to `RetriggerAutofillImpl` from BrowserAutofillManager (instead of `ScheduleRefill`), as we don't need to schedule a refill in the future, but to trigger it immediately (as a response to the promise resolving).
- Add a null check to web_form_element.
I'll add comments in the code itself to make the above clearer and easier to review.
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yoav Weiss (@Shopify)
| Commit-Queue | +1 |
3 comments
WebString key = control_element.NameForAutofill();@schw...@google.com - I brought back the name here, as this is the fields that are web-exposed to developers, and they should have meaningful names.
form_filler_->RetriggerAutofillImpl(@schw...@google.com - This calls RetriggerAutofillImpl rather than ScheduleRefill as I believe we need to trigger it immediately, not in some point in the future.
if (!element) {@schw...@google.com - Added this null check, as some tests arrive here without an HTMLFormElement.
Related details
- Christoph Schwering
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 10 comments![]( "Open in Gerrit")
// Retrigger an autofill after the onautofill event fired and the Promise it
// passed to `waitForIt` was resolved.I suspect most readers won't know what this is referring to, especially what `waitForIt` is. (At least I don't 😊.)
bool waiting_for_autofill_retrigger_ = false;How many times may JS delay the autofill? Once, right?
bool DispatchAutofillEvent(const blink::WebDocument& document,Please document what this does.
WebString key = control_element.NameForAutofill();@schw...@google.com - I brought back the name here, as this is the fields that are web-exposed to developers, and they should have meaningful names.
I see! There will a specification for what the keys should be, right? I think we don't guarantee stability of `NameForAutofill()` so perhaps we should implement our own fallback from `name` to `id` or vice versa, if we want such a fallback at all.
.TriggerAutofillEvent(is_undo, field_values);std::move() or make the parameter a const ref?
void RetriggerAutofillImpl(const FormData& form,Keep `TriggerRefill()`?
if (!element) {@schw...@google.com - Added this null check, as some tests arrive here without an HTMLFormElement.
It seems odd to me to have a meaningful operation on null `WebFormElement`s. Analogous with [this](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/form_autofill_util.cc;l=1882-1891;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), I think the null case should be handled by `WebDocument`.
void waitForIt(ScriptState*, ScriptPromise<IDLUndefined>);What is "it"?
class AutofillEvent : public Event {Should this be documented?
std::unordered_map<String, String> converted_form_values;Do we really want `std::unordered_map` here? base/containers/README.md suggests it for no scenario. And we already take the `std::map` by value here; couldn't we just pass it on?
Related details
- 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")
// Retrigger an autofill after the onautofill event fired and the Promise it
// passed to `waitForIt` was resolved.I suspect most readers won't know what this is referring to, especially what `waitForIt` is. (At least I don't 😊.)
It's the function you can call inside the event, to pass a promise that tells the browser autofill can refill. I need to rename it (both here and in the proposal). This was a placeholder..
bool waiting_for_autofill_retrigger_ = false;How many times may JS delay the autofill? Once, right?
Indeed
WebString key = control_element.NameForAutofill();Christoph Schwering@schw...@google.com - I brought back the name here, as this is the fields that are web-exposed to developers, and they should have meaningful names.
I see! There will a specification for what the keys should be, right? I think we don't guarantee stability of `NameForAutofill()` so perhaps we should implement our own fallback from `name` to `id` or vice versa, if we want such a fallback at all.
Yeah, we probably want to create a more thought-through mapping..
void waitForIt(ScriptState*, ScriptPromise<IDLUndefined>);What is "it"?
- Christoph Schwering
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")
const FormFieldData& trigger_field = form.fields()[0];That's a security issue because the triggering field's origin matters when there are multiple frames involved.
In `BrowserAutofillManager`, that issue is handled by the refill logic, which remembers the origin in the `RefillContex`.
In `AndroidAutofillManager` the closest to that I know is [`current_field_`](https://source.chromium.org/chromium/chromium/src/+/main:components/android_autofill/browser/android_autofill_provider.h;l=327-333;drc=524850a07d7849a97763c3b848063d3dfdbd653d), but I guess just using `current_field_` isn't sufficent.
// Retrigger an autofill after the onautofill event fired and the Promise it
// passed to `waitForIt` was resolved.Yoav Weiss (@Shopify)I suspect most readers won't know what this is referring to, especially what `waitForIt` is. (At least I don't 😊.)
It's the function you can call inside the event, to pass a promise that tells the browser autofill can refill. I need to rename it (both here and in the proposal). This was a placeholder..
I see. I guess we should abstract from this here in the mojo documentation.
Related details
- 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. |