Move MayContainSensitiveData to HTMLFormControlElement [chromium/src : main]
Nan Lin (Gerrit)
Nan Lin voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 3 comments![]( "Open in Gerrit")
bool MayContainSensitiveData() const;I think this should probably have some sort of comment saying what its definition of "sensitive" is and when it's appropriate to use. (For example, I could imagine some people expecting that dates could be sensitive.)
case mojom::blink::FormControlType::kSelectOne:I don't understand why `kSelectOne` is in one category but `kSelectMultiple` is in the other.
(Also, other than the inclusion of `kSelectOne`, I *think* this is equivalent to `blink::Element::IsTextControl()`.)
return false;I'm inclined to think this should be `NOTREACHED()` rather than `return false`.
Related details
- Nan Lin
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. |
Nan Lin (Gerrit)
Nan Lin added 4 comments![]( "Open in Gerrit")
I think this should probably have some sort of comment saying what its definition of "sensitive" is and when it's appropriate to use. (For example, I could imagine some people expecting that dates could be sensitive.)
Updated with the comment, it's mostly tied to autofill. Thanks.
I don't understand why `kSelectOne` is in one category but `kSelectMultiple` is in the other.
(Also, other than the inclusion of `kSelectOne`, I *think* this is equivalent to `blink::Element::IsTextControl()`.)
This mostly mirrors [ToAutofillFormControlType](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/form_autofill_util.cc;drc=a636e7dace1ec22bdd5eb4ab27ccbca9712020d3;bpv=1;bpt=1;l=2287). The only difference was `kInputDate` which was not included as it's not possible for credit card and passwords. As we move to renderer/core, it makes sense to include this as well to be complete. Thanks for catching.
I'm inclined to think this should be `NOTREACHED()` rather than `return false`.
- David Baron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
Hi Stephen, could you PTAL at //components/autofill, thanks!
Related details
- David Baron
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
2 comments
bool IsPotentialSensitiveAutofillControl() const;I think it's probably better to add an `ly` to the name so that it's `IsPotentiallySensitiveAutofillControl`.
Related details
- Nan Lin
- Stephen McGruer
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. |
Nan Lin (Gerrit)
Nan Lin added 2 comments![]( "Open in Gerrit")
I think it's probably better to add an `ly` to the name so that it's `IsPotentiallySensitiveAutofillControl`.
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |