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

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

BUG=346210
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271005

[vasilii, 2014-05-12 16:59:10]

Hi Garrett,

please review. The question is if it's OK to report slightly incorrect value for
the deleted password from AddLogin().

[vasilii, 2014-05-13 16:47:06]

The first practical outcome. One of the LoginDatabaseTest tests was buggy.

[Garrett Casto, 2014-05-14 07:46:34]

Do you want to change NativeBackend* at the same time? I would think that it
would make it harder to reason about sync errors if the different platforms
reported changes to sync differently.

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.cc:55: const Pickle&
usernames_pickle,
Any reason why you have |username_pickle| as a separate param? Not sure why it
shouldn't be deduced from |form|. If it's just because SerializeVector is
currently a member of LoginDatabase, I would just make it a free function.

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.h:37: // changes applied
({}, {ADD}, {DELETE, ADD}). In the latter case the previous
nit: It's REMOVE not DELETE.

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.h:38: // password value
isn't read. Thus only the UNIQUE key columns contain the
The last two sentences are pretty confusing. I think what your trying to say is
that if the function returns {REMOVE, ADD} then the returned
PasswordStoreChangeList will have the REMOVE associated with the form that was
added not the one that was removed. Would you mind trying to clarify the
language a little bit?

I don't think that this is a problem in practice, since sync only cares about
the key, though documenting this irregularity is good.

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:27:
PasswordStoreChangeList ChangeListFromForm(const PasswordForm& form) {
nit: Doesn't seem like the name quite captures exactly what this function does,
since you could imagine multiple types of change lists from this form ({},
{ADD}, {DELETE, ADD} at least). AddChangeForForm maybe?

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:73:
non_html_auth.date_created = now;
Was the lack of date_created causing test failures?

[vasilii, 2014-05-14 11:20:17]

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.cc:55: const Pickle&
usernames_pickle,
On 2014/05/14 07:46:35, Garrett Casto wrote:
> Any reason why you have |username_pickle| as a separate param? Not sure why it
> shouldn't be deduced from |form|. If it's just because SerializeVector is
> currently a member of LoginDatabase, I would just make it a free function.

Done.

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.h:37: // changes applied
({}, {ADD}, {DELETE, ADD}). In the latter case the previous
On 2014/05/14 07:46:35, Garrett Casto wrote:
> nit: It's REMOVE not DELETE.

Done.

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.h:38: // password value
isn't read. Thus only the UNIQUE key columns contain the
On 2014/05/14 07:46:35, Garrett Casto wrote:
> The last two sentences are pretty confusing. I think what your trying to say
is
> that if the function returns {REMOVE, ADD} then the returned
> PasswordStoreChangeList will have the REMOVE associated with the form that was
> added not the one that was removed. Would you mind trying to clarify the
> language a little bit?
> 
> I don't think that this is a problem in practice, since sync only cares about
> the key, though documenting this irregularity is good.

Done.

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:27:
PasswordStoreChangeList ChangeListFromForm(const PasswordForm& form) {
On 2014/05/14 07:46:35, Garrett Casto wrote:
> nit: Doesn't seem like the name quite captures exactly what this function
does,
> since you could imagine multiple types of change lists from this form ({},
> {ADD}, {DELETE, ADD} at least). AddChangeForForm maybe?

Done.

https://codereview.chromium.org/283563002/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:73:
non_html_auth.date_created = now;
On 2014/05/14 07:46:35, Garrett Casto wrote:
> Was the lack of date_created causing test failures?

Yes. RemoveLoginsCreatedBetween did nothing.

[Garrett Casto, 2014-05-14 18:34:48]

Mostly done, still wondering if it makes more sense to bundle all of the
PasswordStore::AddLogin() changes in one CL, or if they should be separate. It
doesn't seem like having them be separate should make anything worse, but I just
want to verify.

https://codereview.chromium.org/283563002/diff/60001/components/password_mana...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/283563002/diff/60001/components/password_mana...
components/password_manager/core/browser/login_database.h:85: // Serialization
routines for vectors.
These routines really shouldn't be externally visible and the tests that call
them directly should instead make sure that saving and loading a form with
interesting other_possible_usernames will match correctly. If you don't want to
do this now, please put a TODO for me to clean this up.

https://codereview.chromium.org/283563002/diff/60001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/283563002/diff/60001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:45: return
LoginDatabase::SerializeVector(vec);
No need to do call forwarding if the function isn't private. Caller should just
call LoginDatabase::SerializeVector() directly.

[vasilii, 2014-05-15 08:36:51]

I found a bug in both NativeBackendKWallet and NativeBackendGnome. They
serialize and deserialize the date as time_t. Therefore, we truncate
microseconds to seconds and lose precision. This is another example how sync
engine can't bring the local data to the cloud state.

https://codereview.chromium.org/283563002/diff/60001/components/password_mana...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/283563002/diff/60001/components/password_mana...
components/password_manager/core/browser/login_database.h:85: // Serialization
routines for vectors.
On 2014/05/14 18:34:48, Garrett Casto wrote:
> These routines really shouldn't be externally visible and the tests that call
> them directly should instead make sure that saving and loading a form with
> interesting other_possible_usernames will match correctly. If you don't want
to
> do this now, please put a TODO for me to clean this up.

Done.

https://codereview.chromium.org/283563002/diff/60001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/283563002/diff/60001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:45: return
LoginDatabase::SerializeVector(vec);
On 2014/05/14 18:34:48, Garrett Casto wrote:
> No need to do call forwarding if the function isn't private. Caller should
just
> call LoginDatabase::SerializeVector() directly.

Done.

[Garrett Casto, 2014-05-15 20:48:52]

LGTM

Nice catch on the time serialization issue, did you file a bug for it?

https://codereview.chromium.org/283563002/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/283563002/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:35: return !changes->empty()
&&
Nit: I prefer adding parens here "(!changes->empty()" so that the next line
should indent to line up instead of just 4 characters.

[vasilii, 2014-05-16 08:18:32]

I'll file the bug.

https://codereview.chromium.org/283563002/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/283563002/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:35: return !changes->empty()
&&
On 2014/05/15 20:48:52, Garrett Casto wrote:
> Nit: I prefer adding parens here "(!changes->empty()" so that the next line
> should indent to line up instead of just 4 characters.

Done.

[vasilii, 2014-05-16 08:18:47]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-05-16 08:19:01]

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

[commit-bot: I haz the power, 2014-05-16 13:28:17]

Change committed as 271005