Convert PasswordForm::password_value to ProcessBoundU16String [chromium/src : main]
Jeffrey Gour (Gerrit)
Jeffrey Gour voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jeffrey Gour
| Commit-Queue | +1 |
1 comment
Hi @vsem...@google.com. Here is the CL for the Phase 2 of my design for migrating to using ProcessBoundString for passwords. Could you please review it?
Thank you!
Related details
- Viktor Semeniuk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Viktor Semeniuk (Gerrit)
Viktor Semeniuk added 14 comments![]( "Open in Gerrit")
I didn't review the CL fully yet. Left a few initial comments. Thanks!
This CL is the implements the second "pillar" (or phase 2) for migratingThis CL implements the second
The first pillar was implement in https://crrev.com/c/7922887.implemented
are mechanical changes to adjust to the new type for forfor
significant changes to them beyond mechanical updates, followed but aby?
assigned to from a const ref to a an actual L-value variable to support thean
usernames, current_username, current_password, account_email_);Just provide password in place.
form_to_save.password_value = stored_password_;How does it work if the copy assignment is deleted?
arg.password_value.value() == base::UTF8ToUTF16(password);`PasswordString` already implements `operator==(const PasswordString&, const std::u16string&)` so `arg.password_value == base::UTF8ToUTF16(password);` should work, right?
const password_manager::PasswordString& password) override;Can we pass PasswordString by value here an below? How difficult would it be to do so?
const password_manager::PasswordString& password) = 0;Should we pass by value here? I know it's a bit more work, but let's be consistent
base::BindRepeating(&WriteToClipboard,
password_form.password_value.value(),Please add a TODO to bind `PasswordString` instead of plain text password.
if (password_dropdown_) {
new_password = password_manager::PasswordString(
std::u16string(password_dropdown_->GetText()));
}Avoid copy-initializing new_password. Only create a PasswordString if `password_dropdown_` is present
```
password_manager::PasswordString new_password =
password_dropdown_
? password_manager::PasswordString(
std::u16string(password_dropdown_->GetText()))
: password_manager::PasswordString(
controller_.pending_password().password_value.value());
```
PasswordString password_value;Could you please add explicit unit tests in `password_form_unittest.cc` verifying copy, move, and equality semantics of `PasswordForm` with `PasswordString` to guarantee that move-clearing and copy-re-encryption behave as expected
Related details
- Jeffrey Gour
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. |
Jeffrey Gour (Gerrit)
Jeffrey Gour voted and added 14 comments![]( "Open in Gerrit")
Votes added by Jeffrey Gour
| Commit-Queue | +1 |
14 comments
Replied to all the comments and resolved most. The two unresolved I'd like some additional thoughts from you on. Thank you for your help!
This CL is the implements the second "pillar" (or phase 2) for migratingThis CL implements the second
Acknowledged
The first pillar was implement in https://crrev.com/c/7922887.Jeffrey Gourimplemented
Acknowledged
are mechanical changes to adjust to the new type for forJeffrey Gourfor
Acknowledged
significant changes to them beyond mechanical updates, followed but aJeffrey Gourby?
Acknowledged
assigned to from a const ref to a an actual L-value variable to support theJeffrey Gouran
Acknowledged
usernames, current_username, current_password, account_email_);Just provide password in place.
Acknowledged
How does it work if the copy assignment is deleted?
Copy assignment isn't deleted anymore to support these cases. It's one of the changes I made in this PR though I forgot to call out it in the description. Will add that.
arg.password_value.value() == base::UTF8ToUTF16(password);`PasswordString` already implements `operator==(const PasswordString&, const std::u16string&)` so `arg.password_value == base::UTF8ToUTF16(password);` should work, right?
Acknowledged
const password_manager::PasswordString& password) override;Can we pass PasswordString by value here an below? How difficult would it be to do so?
See my reply on your comment about this on password_model_delegate.h. Basically could you elaborate on why you think this would be beneficial?
const password_manager::PasswordString& password) = 0;Should we pass by value here? I know it's a bit more work, but let's be consistent
Could you elaborate on why you think passing by value would be better here? The caller for this is SaveUpdateBubbleController::OnSaveClicked() which populates the password parameter with GetPendingPassword().password_value. GetPendingPassword() returns a const PasswordForm&, and so we can't move the password_value out of it. Plus it looks to me like it's by design that this holds onto this PasswordForm object. As a result, passing by value here would mean creating a copy of The PasswordString. Then from SavePassword it's passed through a stack of calls, namely UpdatePasswordFormUsernameAndPassword -> PasswordFormManager::OnUpdatePasswordFromPrompt where it's assigned to the password_value of a different form. I would imagine we'd change all of these to pass by value (I'm not sure why we would to change just the first call of the stack). Now these could all be std::move to avoid the copy, but that's basically additional complexity for the same effect imho. With passing by const ref one copy is made when assigning to the new form at the end of the stack vs with by value one copy is made for the first call of the stack (assuming moves are used everywhere else).
Am I missing something here?
base::BindRepeating(&WriteToClipboard,
password_form.password_value.value(),Please add a TODO to bind `PasswordString` instead of plain text password.
Acknowledged
if (password_dropdown_) {
new_password = password_manager::PasswordString(
std::u16string(password_dropdown_->GetText()));
}Avoid copy-initializing new_password. Only create a PasswordString if `password_dropdown_` is present
```
password_manager::PasswordString new_password =
password_dropdown_
? password_manager::PasswordString(
std::u16string(password_dropdown_->GetText()))
: password_manager::PasswordString(
controller_.pending_password().password_value.value());
```
Acknowledged
Could you please add explicit unit tests in `password_form_unittest.cc` verifying copy, move, and equality semantics of `PasswordForm` with `PasswordString` to guarantee that move-clearing and copy-re-encryption behave as expected
- Jeffrey Gour
- Viktor Semeniuk
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. |