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