Ignore autocomplete=off when password manager is saving the generated passwords during account crea…

Ignore autocomplete=off when password manager is saving the generated passwords during account creation. Note that for the entire feature to work we also need to ignore autocomplete=off when autofill manager is autofilling the generated passwords during log-in, which will be addressed in later CLs.


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

[zysxqn, 2012-12-06 00:36:40]

Can't figure out why glcpp-parse.c is listed here. I tried to sync the master
and merge to my working client but this file is still in the change list. Please
advice me how to remove this if you know, or just review the other relevant
files if it doesn't matter.

[Garrett Casto, 2012-12-06 08:06:14]

Could you add a test for the password_manager change?

Also, we should loop in someone with knowledge of one_click_singin_helper to see
if they want to keep autcomplete=off passwords filtered out (I'm guessing it
doesn't matter for them, but we should check). It looks like rogerta@ is a
reasonable person from the change history.

https://codereview.chromium.org/11446028/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/11446028/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager.cc:184: // Don't save the
password if it is not generated password and autocomplete is
This sentence doesn't read right. I think you want to either say "not a
generated password" or "not automatically generated". I also don't think that
you need the "for it".

https://codereview.chromium.org/11446028/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager.cc:186: if
(!manager->HasGeneratedPassword() && !form.password_should_autocomplete) {
No braces here.

[zysxqn, 2012-12-06 22:39:27]

Test added. Please have another look

[Ilya Sherman, 2012-12-06 22:55:58]

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager.cc:185: // is turned off.
Optional nit: I would suggest phrasing this as follows:

"Always save generated passwords, as the user expresses explicit intent for
Chrome to manage such passwords.  For other passwords, respect the autocomplete
attribute."

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
File chrome/browser/password_manager/password_manager_unittest.cc (right):

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:80: PasswordForm
MakeSimpleForm(bool autocomplete = true) {
nit: Default function parameters are disallowed by the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

The way I'd recommend doing this is not passing a parameter to this function,
and instead explicitly setting form.password_should_autocomplete = false on the
value returned from this function, for the tests that are testing that case.

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:378:
TEST_F(PasswordManagerTest, FormNotSubmitAutocompleteOff) {
nit: Perhaps "Saved" rather than "Submit", or else omit "Not"?  (This also
applies on line 403 below.)

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:382: // This is
mostly copied from FormSubmitEmptyStore.
No need to include this line in the comment, though it suggests that extracting
shared functionality into a helper function might be useful for these tests.

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:401: }
This test isn't actually testing that the form isn't saved.  You probably want
to include the following lines:

EXPECT_CALL(delegate_,
            AddSavePasswordInfoBarIfPermitted(_)).Times(Exactly(0));
EXPECT_CALL(*store_, AddLogin(FormMatches(form))).Times(Exactly(0));

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:423: // consent.
The form should be saved once navigation occurs.
Where has the user given consent?  Does this test pass when run in isolation?

https://codereview.chromium.org/11446028/diff/4001/chrome/renderer/autofill/p...
File chrome/renderer/autofill/password_generation_manager.cc (right):

https://codereview.chromium.org/11446028/diff/4001/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager.cc:106: if
(forms[i].isNull())
Please add/update tests to reflect this change, if possible.

[Ilya Sherman, 2012-12-06 23:40:37]

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
File chrome/browser/password_manager/password_manager_unittest.cc (right):

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:423: // consent.
The form should be saved once navigation occurs.
On 2012/12/06 22:55:59, Ilya Sherman wrote:
> Where has the user given consent?  Does this test pass when run in isolation?

Never mind, I think I understand what this means: The user has given consent by
asking Chrome to generate the password.  I think the comment could probably be
improved to make that a little clearer -- I read this to mean that the user had
already given consent by previously saving a password for this website/username
combo, or something like that.

[zysxqn, 2012-12-06 23:54:49]

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager.cc:185: // is turned off.
On 2012/12/06 22:55:59, Ilya Sherman wrote:
> Optional nit: I would suggest phrasing this as follows:
> 
> "Always save generated passwords, as the user expresses explicit intent for
> Chrome to manage such passwords.  For other passwords, respect the
autocomplete
> attribute."

Done.

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
File chrome/browser/password_manager/password_manager_unittest.cc (right):

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:80: PasswordForm
MakeSimpleForm(bool autocomplete = true) {
On 2012/12/06 22:55:59, Ilya Sherman wrote:
> nit: Default function parameters are disallowed by the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments
> 
> The way I'd recommend doing this is not passing a parameter to this function,
> and instead explicitly setting form.password_should_autocomplete = false on
the
> value returned from this function, for the tests that are testing that case.

Done.

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:378:
TEST_F(PasswordManagerTest, FormNotSubmitAutocompleteOff) {
On 2012/12/06 22:55:59, Ilya Sherman wrote:
> nit: Perhaps "Saved" rather than "Submit", or else omit "Not"?  (This also
> applies on line 403 below.)

Done.

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:382: // This is
mostly copied from FormSubmitEmptyStore.
On 2012/12/06 22:55:59, Ilya Sherman wrote:
> No need to include this line in the comment, though it suggests that
extracting
> shared functionality into a helper function might be useful for these tests.

Done.

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:401: }
On 2012/12/06 22:55:59, Ilya Sherman wrote:
> This test isn't actually testing that the form isn't saved.  You probably want
> to include the following lines:
> 
> EXPECT_CALL(delegate_,
>             AddSavePasswordInfoBarIfPermitted(_)).Times(Exactly(0));
> EXPECT_CALL(*store_, AddLogin(FormMatches(form))).Times(Exactly(0));

Done.

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:423: // consent.
The form should be saved once navigation occurs.
On 2012/12/06 22:55:59, Ilya Sherman wrote:
> Where has the user given consent?  Does this test pass when run in isolation?

This is copied from GeneratedPasswordFormSubmitEmptyStore. I think this
statement means user has given consent to save the password by using the
generated password. Yeah, for this test this is just "simulated" by calling
manager()->SetFormHasGeneratedpassword(form) above.

https://codereview.chromium.org/11446028/diff/4001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager_unittest.cc:423: // consent.
The form should be saved once navigation occurs.
On 2012/12/06 23:40:37, Ilya Sherman wrote:
> On 2012/12/06 22:55:59, Ilya Sherman wrote:
> > Where has the user given consent?  Does this test pass when run in
isolation?
> 
> Never mind, I think I understand what this means: The user has given consent
by
> asking Chrome to generate the password.  I think the comment could probably be
> improved to make that a little clearer -- I read this to mean that the user
had
> already given consent by previously saving a password for this
website/username
> combo, or something like that.

Done.

https://codereview.chromium.org/11446028/diff/4001/chrome/renderer/autofill/p...
File chrome/renderer/autofill/password_generation_manager.cc (right):

https://codereview.chromium.org/11446028/diff/4001/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager.cc:106: if
(forms[i].isNull())
On 2012/12/06 22:55:59, Ilya Sherman wrote:
> Please add/update tests to reflect this change, if possible.

Done.

[Ilya Sherman, 2012-12-07 01:52:16]

LGTM, but please also wait for Garrett and Roger's review.  Thanks.

https://codereview.chromium.org/11446028/diff/11016/chrome/renderer/autofill/...
File chrome/renderer/autofill/password_generation_manager_browsertest.cc
(right):

https://codereview.chromium.org/11446028/diff/11016/chrome/renderer/autofill/...
chrome/renderer/autofill/password_generation_manager_browsertest.cc:108:
"autocomplete = 'off' size = 5/>"
nit: Please align the quotation marks, and then uses spaces after the quotation
mark to align "type" and "autocomplete"

[Garrett Casto, 2012-12-07 19:31:21]

lgtm

https://codereview.chromium.org/11446028/diff/11016/chrome/renderer/autofill/...
File chrome/renderer/autofill/password_generation_manager_browsertest.cc
(right):

https://codereview.chromium.org/11446028/diff/11016/chrome/renderer/autofill/...
chrome/renderer/autofill/password_generation_manager_browsertest.cc:108:
"autocomplete = 'off' size = 5/>"
On 2012/12/07 01:52:16, Ilya Sherman wrote:
> nit: Please align the quotation marks, and then uses spaces after the
quotation
> mark to align "type" and "autocomplete"

+1

[zysxqn, 2012-12-10 20:53:28]

-rogerta, +timsteele

[tim (not reviewing), 2012-12-11 01:23:04]

A naming nit, but otherwise LGTM

https://codereview.chromium.org/11446028/diff/11017/content/public/common/pas...
File content/public/common/password_form.h (right):

https://codereview.chromium.org/11446028/diff/11017/content/public/common/pas...
content/public/common/password_form.h:104: bool password_should_autocomplete;
This makes it sound more authoritative than it is -- maybe
form_autocomplete_attribute_value?

[zysxqn, 2012-12-11 01:52:32]

https://codereview.chromium.org/11446028/diff/11017/content/public/common/pas...
File content/public/common/password_form.h (right):

https://codereview.chromium.org/11446028/diff/11017/content/public/common/pas...
content/public/common/password_form.h:104: bool password_should_autocomplete;
On 2012/12/11 01:23:04, timsteele wrote:
> This makes it sound more authoritative than it is -- maybe
> form_autocomplete_attribute_value?

I can get your point: whether we should autocomplete is not solely determined by
this value, and this just indicates whether the autocomplete property is set to
off or not. But it is convenient to make this property a bool value. How about
naming it to "password_autocomplete_set"?

[commit-bot: I haz the power, 2012-12-11 19:22:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/11446028/21001

[commit-bot: I haz the power, 2012-12-11 19:22:40]

Presubmit check for 11446028-21001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public
    content/renderer

Presubmit checks took 1.1s to calculate.

[zysxqn, 2012-12-11 19:25:05]

+ avi@ for approval of content/

[Avi (use Gerrit), 2012-12-11 20:41:06]

content lgtm

[commit-bot: I haz the power, 2012-12-11 20:44:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/11446028/21001

[commit-bot: I haz the power, 2012-12-12 05:19:53]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-12 06:44:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/11446028/21001

[commit-bot: I haz the power, 2012-12-12 07:54:52]

Change committed as 172557