Don't allow the empty new password to overwrite the existing one.

Don't allow the empty new password to overwrite the existing one.

BUG=404012, 410798
Committed: https://crrev.com/d2de6078e7f2ce097c3f31b1832d7260f93cd92c
Cr-Commit-Position: refs/heads/master@{#293492}

[vasilii, 2014-09-04 12:23:34]

vasilii@chromium.org changed reviewers:
+ vabr@chromium.org

[vasilii, 2014-09-04 12:23:35]

Hi Vaclav,

please review.

[vabr (Chromium), 2014-09-04 13:28:03]

Thanks, Vasilii!

Looks good, but could you please add a test?

Cheers,
Vaclav

https://codereview.chromium.org/545433003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/545433003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.cc:180: if
(!form.new_password_element.empty() && form.new_password_value.empty()) {
nit: Given that the code inside the if-branch is more than just a return, and is
duplicated in these two if-branches, what about merging them in just one
if-branch?

[vasilii, 2014-09-04 15:51:55]

https://codereview.chromium.org/545433003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/545433003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.cc:180: if
(!form.new_password_element.empty() && form.new_password_value.empty()) {
On 2014/09/04 13:28:03, vabr (Chromium) wrote:
> nit: Given that the code inside the if-branch is more than just a return, and
is
> duplicated in these two if-branches, what about merging them in just one
> if-branch?

Done.

[vabr (Chromium), 2014-09-04 20:14:14]

LGTM with comments.
Thanks, Vasilii!
Vaclav

https://codereview.chromium.org/545433003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/545433003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:801:
TEST_F(PasswordManagerTest, UpdateWithEmptyPassword) {
nit: Should the name rather be "DoNotUpdateWithEmptyPassword"? It should also be
explained, why "empty password", when the current password in the created form
is not empty.
(Feel free to de-duplicate information requested here with the information
requested in the comment below.)

https://codereview.chromium.org/545433003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:806:
PasswordForm form(MakeSimpleForm());
nit: Please consider emphasising in a comment, that the created form has an
empty (but existing) new password field, and why it is important (what could/did
go wrong). Instead of long description of the problem, a URL to the bugreport
might be a good idea.

https://codereview.chromium.org/545433003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:819: //
Now the password manager waits for the navigation to complete.
optional nit: Should the comment mention that by clearing the observed forms, it
signals a successful login?

[vabr (Chromium), 2014-09-05 08:37:04]

I updated the description by adding the number of the bug engedy@ created for
this issue (http://crbug.com/410798).

[vasilii, 2014-09-05 09:08:10]

https://codereview.chromium.org/545433003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/545433003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:801:
TEST_F(PasswordManagerTest, UpdateWithEmptyPassword) {
On 2014/09/04 20:14:14, vabr (Chromium) wrote:
> nit: Should the name rather be "DoNotUpdateWithEmptyPassword"? It should also
be
> explained, why "empty password", when the current password in the created form
> is not empty.
> (Feel free to de-duplicate information requested here with the information
> requested in the comment below.)

Done.

https://codereview.chromium.org/545433003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:806:
PasswordForm form(MakeSimpleForm());
On 2014/09/04 20:14:14, vabr (Chromium) wrote:
> nit: Please consider emphasising in a comment, that the created form has an
> empty (but existing) new password field, and why it is important (what
could/did
> go wrong). Instead of long description of the problem, a URL to the bugreport
> might be a good idea.

Done.

https://codereview.chromium.org/545433003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager_unittest.cc:819: //
Now the password manager waits for the navigation to complete.
On 2014/09/04 20:14:14, vabr (Chromium) wrote:
> optional nit: Should the comment mention that by clearing the observed forms,
it
> signals a successful login?

Done.

[vasilii, 2014-09-05 09:09:12]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-05 09:10:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/545433003/40001

[commit-bot: I haz the power, 2014-09-05 10:22:13]

Committed patchset #3 (id:40001) as 00f9a502386554167a43b0e2306b89314fdcc585

[commit-bot: I haz the power, 2014-09-10 03:38:33]

Patchset 3 (id:??) landed as
https://crrev.com/d2de6078e7f2ce097c3f31b1832d7260f93cd92c
Cr-Commit-Position: refs/heads/master@{#293492}