[EVP] Show a status indicator in the input field (part 1) [chromium/src : main]
Christian Biesinger (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Nicolás Peña
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolás Peña (Gerrit)
Nicolás Peña added 6 comments![]( "Open in Gerrit")
if (element) {Consider early returns instead to avoid nesting.
blink::WebInputElement::EmailVerificationState::kNone);Isnt this the default state? Or what's the difference?
void WebInputElement::SetEmailVerificationState(EmailVerificationState state) {Do we need both enums? If we could consolidate, perhaps we wouldn't need this method
container->AppendChild(indicator);Please fix this WARNING reported by autoreview issue finding: If the `email_verification_state_` is updated before the shadow subtree is created (e.g. shadow tree creation is scheduled lazily), `UpdateEmailVerificationIndicator()` in `SetEmailVerificationState` returns early. Then when `CreateShadowSubtree()` is eventually called, the new `indicator` will default to `state="none"` regardless of the current `email_verification_state_`.
You should call `UpdateEmailVerificationIndicator();` here to ensure the indicator's attributes reflect the actual state.
if (GetElement().IsInCanvasSubtree()) {In this case I guess we could early return before querying `indicator`?
Related details
- Christian Biesinger
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. |
Christian Biesinger (Gerrit)
Christian Biesinger added 7 comments![]( "Open in Gerrit")
Is there a screenshot? :)
Consider early returns instead to avoid nesting.
Done
blink::WebInputElement::EmailVerificationState::kNone);Isnt this the default state? Or what's the difference?
I removed it, but not for the reason you mentioned :-) This function doesn't get called with an empty token.
We should clear the state when the email address changes but that can be done in a followup.
void WebInputElement::SetEmailVerificationState(EmailVerificationState state) {Do we need both enums? If we could consolidate, perhaps we wouldn't need this method
Done, but we do need this method as a bridge between internal blink code and components/autofill/content/renderer.
(Gemini thinks this of your suggestion: `This structure is exceptionally clean`)
Please fix this WARNING reported by autoreview issue finding: If the `email_verification_state_` is updated before the shadow subtree is created (e.g. shadow tree creation is scheduled lazily), `UpdateEmailVerificationIndicator()` in `SetEmailVerificationState` returns early. Then when `CreateShadowSubtree()` is eventually called, the new `indicator` will default to `state="none"` regardless of the current `email_verification_state_`.
You should call `UpdateEmailVerificationIndicator();` here to ensure the indicator's attributes reflect the actual state.
Done
if (GetElement().IsInCanvasSubtree()) {In this case I guess we could early return before querying `indicator`?
We could avoid setting the state attribute entirely maybe, but IMO it's more consistent to still set it in this case.
(we need `indicator` on the next line to call `setAttribute`)
Related details
- Nicolás Peña
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. |
Nicolás Peña (Gerrit)
Nicolás Peña voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nicolás Peña
| Code-Review | +1 |
1 comment
if (GetElement().IsInCanvasSubtree()) {Christian BiesingerIn this case I guess we could early return before querying `indicator`?
We could avoid setting the state attribute entirely maybe, but IMO it's more consistent to still set it in this case.
(we need `indicator` on the next line to call `setAttribute`)
Actually thinking about the comment below more, doesnt the website get the token before the form is submitted anyways? Why do we need to treat canvas subtrees differently?
Related details
- Christian Biesinger
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. |
Christian Biesinger (Gerrit)
Christian Biesinger added 2 comments![]( "Open in Gerrit")
+Joey for forms
+pdr for blink/public
+Jihad for components
if (GetElement().IsInCanvasSubtree()) {Christian BiesingerIn this case I guess we could early return before querying `indicator`?
Nicolás PeñaWe could avoid setting the state attribute entirely maybe, but IMO it's more consistent to still set it in this case.
(we need `indicator` on the next line to call `setAttribute`)
Actually thinking about the comment below more, doesnt the website get the token before the form is submitted anyways? Why do we need to treat canvas subtrees differently?
It does not, no. That is, it would only get the token immediately before submit. In addition, once this is fully implemented, the website would be able to tell the difference between "user denied permission" and "user is not logged in" based on the indicator.
Related details
- Jihad Hanna
- Joey Arhar
- Nicolás Peña
- Philip Rogers
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. |
Nicolás Peña (Gerrit)
Nicolás Peña added 1 comment![]( "Open in Gerrit")
if (GetElement().IsInCanvasSubtree()) {Christian BiesingerIn this case I guess we could early return before querying `indicator`?
Nicolás PeñaWe could avoid setting the state attribute entirely maybe, but IMO it's more consistent to still set it in this case.
(we need `indicator` on the next line to call `setAttribute`)
Christian BiesingerActually thinking about the comment below more, doesnt the website get the token before the form is submitted anyways? Why do we need to treat canvas subtrees differently?
It does not, no. That is, it would only get the token immediately before submit. In addition, once this is fully implemented, the website would be able to tell the difference between "user denied permission" and "user is not logged in" based on the indicator.
- Christian Biesinger
- Jihad Hanna
- Joey Arhar
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 3 comments![]( "Open in Gerrit")
if (!container) {
return;
}Is this element ever null? Based on a quick skim, I would expect that TextFieldInputType::CreateShadowSubtree would create this element, right?
indicator->setAttribute(AtomicString("state"), AtomicString("none"));Maybe would be nicer to use data-state since state isn't a standardized attribute?
RuntimeEnabledFeatures::EmailVerificationStatusIndicatorEnabled(would it make sense to just make the EmailVerificationStatusIndicator flag depend_on EmailVerificationProtocol instead of having to check two flags here? Or is that not possible when they're set up to use origin trials?
Related details
- Christian Biesinger
- Jihad Hanna
- Philip Rogers
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. |