[Password Generation] Add negative votes for crowdsourcing

[Password Generation] Add negative votes for crowdsourcing

Login pages with more than 2 text fields can send signals
that looks like account creation pages. This change adds the ability
to inform the Autofill server that the saved credentials have been used
on the original form, which indicates that it is likely not an
account creation form.

BUG=454028
Committed: https://crrev.com/9b34aee9ee614eea78b4383832047bc5d36f438e
Cr-Commit-Position: refs/heads/master@{#316256}

[Garrett Casto, 2015-02-12 09:16:53]

gcasto@chromium.org changed reviewers:
+ dvadym@chromium.org, vabr@chromium.org

[vabr (Chromium), 2015-02-12 10:08:27]

This LGTM, with a couple of comments.

Cheers,
Vaclav

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

https://codereview.chromium.org/920683002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:1050:
CreateVersion5Pickle(form, &pickle);
5->6

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

https://codereview.chromium.org/920683002/diff/20001/components/autofill/core...
components/autofill/core/common/password_form.h:208: enum GenerationUploadStatus
{
I explicitly acknowledge the choice of enum over "enum class" here, because of
implicit conversion to int being handy for serialisation.

https://codereview.chromium.org/920683002/diff/20001/components/autofill/core...
components/autofill/core/common/password_form.h:208: enum GenerationUploadStatus
{
Declaration order: types should be before member variables.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

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

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:777: if
(!pending->form_data.fields.empty() &&
The current code relies on the fact that forms with fields.empty() will also not
have generation_upload_status set to ..._SENT.
It would be clearer to just separate the .empty() check in its own if-block,
with an early return.

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

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:245:
void AccountCreationUploadTest(const autofill::FormData& observed_form_data,
Please add a comment about what this method does.

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:248:
autofill::ServerFieldType* field_type) {
nit: This should be const, to make it clear the pointer is here to allow nulls,
not to allow modification.

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:251:
EXPECT_CALL(*client_with_store.mock_driver(), IsOffTheRecord())
nit: Unless you really care about seeing this call, you should write
ON_CALL(...).WillByDefault(Return(false));

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:271:
SimulateResponseFromPasswordStore(&form_manager, result);
If you rebase to ToT, this method was already replaced by just calling
OnGetPasswordStoreResults on form_manager. Also note that |result| now needs to
be a ScopedVector.

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:273:
if (field_type) {
Should we explicitly add a similar EXPECT_CALL(...).Times(0) for cases when
field_type is null? While it might not be necessary due to default settings of
the mock, it would be clearer to know that this check is supposed to fail if
there are uploads and it is not told which value to expect.

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

https://codereview.chromium.org/920683002/diff/20001/components/test/data/pas...
components/test/data/password_manager/login_db_v10.sql:1: PRAGMA
foreign_keys=OFF;
Just to avoid confusion -- this was broken, and you are fixing this, or are
these changes needed for the rest of the CL?

[Garrett Casto, 2015-02-12 22:13:53]

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

[Garrett Casto, 2015-02-12 22:15:36]

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

https://codereview.chromium.org/920683002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:1050:
CreateVersion5Pickle(form, &pickle);
On 2015/02/12 10:08:25, vabr (Chromium) wrote:
> 5->6

Done.

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

https://codereview.chromium.org/920683002/diff/20001/components/autofill/core...
components/autofill/core/common/password_form.h:208: enum GenerationUploadStatus
{
On 2015/02/12 10:08:25, vabr (Chromium) wrote:
> I explicitly acknowledge the choice of enum over "enum class" here, because of
> implicit conversion to int being handy for serialisation.

Yeah, didn't explicitly mention this, but there doesn't seem to be better to use
enum class here.

https://codereview.chromium.org/920683002/diff/20001/components/autofill/core...
components/autofill/core/common/password_form.h:208: enum GenerationUploadStatus
{
On 2015/02/12 10:08:25, vabr (Chromium) wrote:
> Declaration order: types should be before member variables.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Done.

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

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:777: if
(!pending->form_data.fields.empty() &&
On 2015/02/12 10:08:25, vabr (Chromium) wrote:
> The current code relies on the fact that forms with fields.empty() will also
not
> have generation_upload_status set to ..._SENT.
> It would be clearer to just separate the .empty() check in its own if-block,
> with an early return.

Done.

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

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:245:
void AccountCreationUploadTest(const autofill::FormData& observed_form_data,
On 2015/02/12 10:08:27, vabr (Chromium) wrote:
> Please add a comment about what this method does.

Done.

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:248:
autofill::ServerFieldType* field_type) {
On 2015/02/12 10:08:27, vabr (Chromium) wrote:
> nit: This should be const, to make it clear the pointer is here to allow
nulls,
> not to allow modification.

Done.

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:251:
EXPECT_CALL(*client_with_store.mock_driver(), IsOffTheRecord())
On 2015/02/12 10:08:27, vabr (Chromium) wrote:
> nit: Unless you really care about seeing this call, you should write
> ON_CALL(...).WillByDefault(Return(false));

Done.

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:271:
SimulateResponseFromPasswordStore(&form_manager, result);
On 2015/02/12 10:08:27, vabr (Chromium) wrote:
> If you rebase to ToT, this method was already replaced by just calling
> OnGetPasswordStoreResults on form_manager. Also note that |result| now needs
to
> be a ScopedVector.

Done.

https://codereview.chromium.org/920683002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:273:
if (field_type) {
On 2015/02/12 10:08:25, vabr (Chromium) wrote:
> Should we explicitly add a similar EXPECT_CALL(...).Times(0) for cases when
> field_type is null? While it might not be necessary due to default settings of
> the mock, it would be clearer to know that this check is supposed to fail if
> there are uploads and it is not told which value to expect.

I think that by default you only get a warning not a failure, so I'll explicitly
set this.

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

https://codereview.chromium.org/920683002/diff/20001/components/test/data/pas...
components/test/data/password_manager/login_db_v10.sql:1: PRAGMA
foreign_keys=OFF;
On 2015/02/12 10:08:27, vabr (Chromium) wrote:
> Just to avoid confusion -- this was broken, and you are fixing this, or are
> these changes needed for the rest of the CL?

Ah, looks like I made a mistake here. This was sort of broken in that version
should be 10, not eleven, but for some reason it looks like my changes for v11
overwrote those for v10. Myst have accidentally moved a file to the wrong place.
Fixed.

[vabr (Chromium), 2015-02-13 09:02:39]

Thanks, Garrett, LGTM!

Vaclav

[Garrett Casto, 2015-02-13 19:06:58]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2015-02-13 19:07:58]

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

[commit-bot: I haz the power, 2015-02-13 19:12:32]

Committed patchset #4 (id:60001)

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

Patchset 4 (id:??) landed as
https://crrev.com/9b34aee9ee614eea78b4383832047bc5d36f438e
Cr-Commit-Position: refs/heads/master@{#316256}