Password Login Database: report correct changes from UpdateLogin().

Password Login Database: report correct changes from UpdateLogin().

Don't update username_element and password_element in UpdateLogin().

BUG=349138, 344337
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272303

[vasilii, 2014-05-19 17:40:03]

Hi Garrett,

please review the CL.

[vasilii, 2014-05-20 15:44:06]

I changed UpdateLogin(). This fixes the bug introduced in
https://codereview.chromium.org/169173005.

[vasilii, 2014-05-21 08:49:35]

Garrett?

[Garrett Casto, 2014-05-21 19:14:20]

Apologies for the delay, the last few days have been very busy.

BTW, could you add bug 344337 to this CL?

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:607: if
(RawAddLogin(form)) {
Now that we completely overwrite the saved form, is there any reason for this to
be in the loop? Seems like it should just be done once afterwards (assuming
!forms.empty()).

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x_unittest.cc (right):

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:736:
TEST_F(NativeBackendGnomeTest, BasicUpdateLogin) {
Would you mind adding a test that calling UpdateLogin() when there are no
matching forms doesn't add the form to the database? Seems like a hole in the
testing coverage.

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
(right):

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:543:
TEST_F(NativeBackendKWalletTest, BasicUpdateLogin) {
As with the other test, can you check that updating a non-matching login doesn't
add?

[vasilii, 2014-05-22 11:38:13]

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:607: if
(RawAddLogin(form)) {
On 2014/05/21 19:14:20, Garrett Casto wrote:
> Now that we completely overwrite the saved form, is there any reason for this
to
> be in the loop? Seems like it should just be done once afterwards (assuming
> !forms.empty()).

Done.

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x_unittest.cc (right):

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:736:
TEST_F(NativeBackendGnomeTest, BasicUpdateLogin) {
On 2014/05/21 19:14:20, Garrett Casto wrote:
> Would you mind adding a test that calling UpdateLogin() when there are no
> matching forms doesn't add the form to the database? Seems like a hole in the
> testing coverage.

Done.

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
(right):

https://codereview.chromium.org/299443002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:543:
TEST_F(NativeBackendKWalletTest, BasicUpdateLogin) {
On 2014/05/21 19:14:20, Garrett Casto wrote:
> As with the other test, can you check that updating a non-matching login
doesn't
> add?

Done.

[Garrett Casto, 2014-05-22 18:41:29]

lgtm

[commit-bot: I haz the power, 2014-05-22 18:43:09]

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

[commit-bot: I haz the power, 2014-05-22 20:27:34]

Change committed as 272303