Rename PasswordForm::is_zero_click to PasswordForm::skip_next_zero_click.

Rename PasswordForm::is_zero_click to PasswordForm::skip_next_zero_click.

In addition to that the patch restores the unique key in the LoginDatabase which was incorrectly abolished in https://codereview.chromium.org/818443004. The former key was (origin, signon_realm, username_element, username_value password_element, submit_element). The correct one used by Sync is without submit_element.

BUG=400674, 368075
Committed: https://crrev.com/13664fea1fbd2b6df3c406f09ecfd44c14341db9
Cr-Commit-Position: refs/heads/master@{#314558}

[vasilii, 2015-02-03 13:40:52]

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

[vasilii, 2015-02-03 13:41:51]

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

[vasilii, 2015-02-03 13:41:52]

Hi guys,

please review.

[Mike West, 2015-02-03 14:15:39]

mkwst@chromium.org changed required reviewers:
+ gcasto@chromium.org

[Mike West, 2015-02-03 14:15:41]

The actual change LGTM. I'd like gcasto@ to take a look at the SQL bits, though.
It's been a while...

Is there a good reason to bundle the fix for the unique key in this CL? If it
would require another migration, then sure, bundle it, but it seems quite
distinct from renaming the field.

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

https://codereview.chromium.org/894583003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:233: {
"skip_next_zero_click", GNOME_KEYRING_ATTRIBUTE_TYPE_UINT32 },
Is there no bool type?

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

https://codereview.chromium.org/894583003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:121:
{"skip_next_zero_click", SECRET_SCHEMA_ATTRIBUTE_INTEGER},
Ditto.

https://codereview.chromium.org/894583003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:414: &error,
Thanks for reformatting this. It's a good deal more readable now.

https://codereview.chromium.org/894583003/diff/20001/components/autofill/core...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/894583003/diff/20001/components/autofill/core...
components/autofill/core/common/password_form.h:221: // If true, Chrome will not
return this credential to a site automatically.
Can you add something about the API to which this is bound? Perhaps "If true,
Chrome will not return this credential to a site in response to
'navigator.credentials.request()' without user interaction."?

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

https://codereview.chromium.org/894583003/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:188: query << "CREATE
INDEX logins_signon ON " << table_name << " (signon_realm)";
I'm deferring to gcasto@ on these bits. It has been a LOOOOOONG time since I did
anything with SQL.

https://codereview.chromium.org/894583003/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:356: "date_synced,
display_name, avatar_url, federation_url, is_zero_click"
In theory, we should be negating `is_zero_click`, right?

We never used it for anything, though, so I suppose there's no way a user could
have changed the setting...

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

https://codereview.chromium.org/894583003/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1139: "ORDER
BY username_value, date_created DESC"));
Hrm. It seems like this fix ought to be in a separate CL.

https://codereview.chromium.org/894583003/diff/20001/components/test/data/pas...
File components/test/data/password_manager/login_db_v10.sql (right):

https://codereview.chromium.org/894583003/diff/20001/components/test/data/pas...
components/test/data/password_manager/login_db_v10.sql:58: 'theerikchen', /*
username_value */
Why this change?

https://codereview.chromium.org/894583003/diff/20001/components/test/data/pas...
components/test/data/password_manager/login_db_v10.sql:61: 'non-empty', /*
submit_element */
And this one?

[vasilii, 2015-02-03 19:49:18]

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

https://codereview.chromium.org/894583003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:233: {
"skip_next_zero_click", GNOME_KEYRING_ATTRIBUTE_TYPE_UINT32 },
On 2015/02/03 14:15:40, Mike West wrote:
> Is there no bool type?

There are just a string and a number :-D
https://developer.gnome.org/gnome-keyring/stable/gnome-keyring-Item-Attribute...

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

https://codereview.chromium.org/894583003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:121:
{"skip_next_zero_click", SECRET_SCHEMA_ATTRIBUTE_INTEGER},
On 2015/02/03 14:15:41, Mike West wrote:
> Ditto.

As libsecret is just a wrapper around Gnome keyring, the answer is the same.

https://codereview.chromium.org/894583003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:414: &error,
On 2015/02/03 14:15:41, Mike West wrote:
> Thanks for reformatting this. It's a good deal more readable now.

Acknowledged.

https://codereview.chromium.org/894583003/diff/20001/components/autofill/core...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/894583003/diff/20001/components/autofill/core...
components/autofill/core/common/password_form.h:221: // If true, Chrome will not
return this credential to a site automatically.
On 2015/02/03 14:15:41, Mike West wrote:
> Can you add something about the API to which this is bound? Perhaps "If true,
> Chrome will not return this credential to a site in response to
> 'navigator.credentials.request()' without user interaction."?

Done.

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

https://codereview.chromium.org/894583003/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:188: query << "CREATE
INDEX logins_signon ON " << table_name << " (signon_realm)";
On 2015/02/03 14:15:41, Mike West wrote:
> I'm deferring to gcasto@ on these bits. It has been a LOOOOOONG time since I
did
> anything with SQL.

I'm jealous :-)

https://codereview.chromium.org/894583003/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:356: "date_synced,
display_name, avatar_url, federation_url, is_zero_click"
On 2015/02/03 14:15:41, Mike West wrote:
> In theory, we should be negating `is_zero_click`, right?
> 
> We never used it for anything, though, so I suppose there's no way a user
could
> have changed the setting...

You are right. We know that is_zero_click==false for everybody and it should
stay like this for skip_next_zero_click.
You may notice that I dropped the previous value entirely while reading from one
of the backends.

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

https://codereview.chromium.org/894583003/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1139: "ORDER
BY username_value, date_created DESC"));
On 2015/02/03 14:15:41, Mike West wrote:
> Hrm. It seems like this fix ought to be in a separate CL.

The new thing is "date_created DESC". I needed it to pass the migration test for
my version. I deliberately added two forms with the same username, origin etc.
but the date is different. The migration code should glue these forms.

https://codereview.chromium.org/894583003/diff/20001/components/test/data/pas...
File components/test/data/password_manager/login_db_v10.sql (right):

https://codereview.chromium.org/894583003/diff/20001/components/test/data/pas...
components/test/data/password_manager/login_db_v10.sql:58: 'theerikchen', /*
username_value */
On 2015/02/03 14:15:41, Mike West wrote:
> Why this change?

I can leave it as it was before. But the test won't be strong enough. If you
look at the SQL query for migration, you'll see "INSERT OR REPLACE". It's very
important ("OR REPLACE") because previous versions may contain rows which will
violate the new 'UNIQUE' constraint. This test simulates the described
situation.

https://codereview.chromium.org/894583003/diff/20001/components/test/data/pas...
components/test/data/password_manager/login_db_v10.sql:61: 'non-empty', /*
submit_element */
On 2015/02/03 14:15:41, Mike West wrote:
> And this one?

The 2 forms here have the same (origin_url, username_element, username_value,
password_element, signon_realm) but different "submit_element". submit_element
used to be a part of the UNIQUE column set.

[vasilii, 2015-02-03 19:54:03]

On 2015/02/03 14:15:41, Mike West wrote:
> The actual change LGTM. I'd like gcasto@ to take a look at the SQL bits,
though.
> It's been a while...
> 
> Is there a good reason to bundle the fix for the unique key in this CL? If it
> would require another migration, then sure, bundle it, but it seems quite
> distinct from renaming the field.

It would require another migration. As I need to restore the constraint anyway,
I think it's a good time to revive it in a right way.

[Garrett Casto, 2015-02-04 06:58:40]

LGTM with comments. Nothing major.

https://codereview.chromium.org/894583003/diff/40001/components/autofill/core...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/894583003/diff/40001/components/autofill/core...
components/autofill/core/common/password_form.h:224: bool skip_next_zero_click;
As someone that isn't particularly involved here, why is this
|skip_next_zero_click| instead of |skip_zero_click|? The description doesn't
make it seems like this is a one time thing, but instead a state that exists
until the user selects this particular credential.

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

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.cc:155:
std::ostringstream query;
The style guide discourages streams other than for logging. I'm actually
surprised that there are a few hundred uses of ostringstream in the repo, but
it's still an order of magnitude less than the number of StringPrintf() calls.

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.cc:187:
std::ostringstream query;
Same here.

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.cc:342:
!db_.Execute("CREATE INDEX logins_signon ON logins (signon_realm)"))
Nit: Might as well use CreateIndexOnSignonRealm() here.

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

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1168:
versions.push_back("9_without_use_additional_auth_field");
I had originally wanted this additional datafile added because version 9 (and
version 8) can actually be different depending on when they were created because
of crbug.com/423716. I wanted to verify that the update code from 9->10 could
properly handle both cases. I guess at this point we probably won't change this
code, but I think that there is still some value to having this tested in case
we do. So I'd prefer that you keep this if it isn't overly difficult.

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

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1188: //
Double add should fail due to constraints in the database.
Nit: I'm not sure if I would consider this failing exactly. What about something
like "Adding the same field will remove conflicting forms"?

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1211: // Two
forms were merged.
Can you comment here that the test data is setup on this version to cause a
unique key collision. Otherwise this seems pretty random.

[vasilii, 2015-02-04 12:41:32]

New patchsets have been uploaded after l-g-t-m from gcasto@chromium.org

[vasilii, 2015-02-04 12:44:02]

https://codereview.chromium.org/894583003/diff/40001/components/autofill/core...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/894583003/diff/40001/components/autofill/core...
components/autofill/core/common/password_form.h:224: bool skip_next_zero_click;
On 2015/02/04 06:58:40, Garrett Casto wrote:
> As someone that isn't particularly involved here, why is this
> |skip_next_zero_click| instead of |skip_zero_click|? The description doesn't
> make it seems like this is a one time thing, but instead a state that exists
> until the user selects this particular credential.

Done.

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

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.cc:155:
std::ostringstream query;
On 2015/02/04 06:58:40, Garrett Casto wrote:
> The style guide discourages streams other than for logging. I'm actually
> surprised that there are a few hundred uses of ostringstream in the repo, but
> it's still an order of magnitude less than the number of StringPrintf() calls.

Done.

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.cc:187:
std::ostringstream query;
On 2015/02/04 06:58:40, Garrett Casto wrote:
> Same here.

Done.

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database.cc:342:
!db_.Execute("CREATE INDEX logins_signon ON logins (signon_realm)"))
On 2015/02/04 06:58:40, Garrett Casto wrote:
> Nit: Might as well use CreateIndexOnSignonRealm() here.

Done.

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

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1168:
versions.push_back("9_without_use_additional_auth_field");
On 2015/02/04 06:58:40, Garrett Casto wrote:
> I had originally wanted this additional datafile added because version 9 (and
> version 8) can actually be different depending on when they were created
because
> of crbug.com/423716. I wanted to verify that the update code from 9->10 could
> properly handle both cases. I guess at this point we probably won't change
this
> code, but I think that there is still some value to having this tested in case
> we do. So I'd prefer that you keep this if it isn't overly difficult.

Done.

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

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1188: //
Double add should fail due to constraints in the database.
On 2015/02/04 06:58:40, Garrett Casto wrote:
> Nit: I'm not sure if I would consider this failing exactly. What about
something
> like "Adding the same field will remove conflicting forms"?

Done.

https://codereview.chromium.org/894583003/diff/40001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1211: // Two
forms were merged.
On 2015/02/04 06:58:40, Garrett Casto wrote:
> Can you comment here that the test data is setup on this version to cause a
> unique key collision. Otherwise this seems pretty random.

Done.

[vasilii, 2015-02-04 12:44:20]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2015-02-04 12:45:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894583003/60001

[commit-bot: I haz the power, 2015-02-04 13:50:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 13:50:05]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vasilii, 2015-02-04 13:57:02]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2015-02-04 13:57:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894583003/80001

[commit-bot: I haz the power, 2015-02-04 14:59:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-04 15:01:01]

Patchset 5 (id:??) landed as
https://crrev.com/13664fea1fbd2b6df3c406f09ecfd44c14341db9
Cr-Commit-Position: refs/heads/master@{#314558}