Make password generation switched by a preference in chrome settings rather than a command line fla…

Make password generation switched by a preference in chrome settings rather than a command line flag.


BUG=
TEST= AutofillManager_Unittest,  PasswordgenerationManager_Browsertest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137573

[Garrett Casto, 2012-04-26 23:23:59]

So I was adding a lot of comments about how we should have just one IPC from
browser to renderer, but I decided I should probably just make one comment and
have you fix it everywhere :). There is no reason to have a separate
PasswordGenerationEnabled and PasswordSyncEnabled flows. Just one is enough.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.cc:214: true,
We are going to have this be an opt-in feature at first, so this should default
to false.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.cc:243: void
AutofillManager::SendPasswordSyncStateToRenderer(
Remove this.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.cc:280:
profile->GetPrefs()->GetBoolean(prefs::kPasswordGenerationEnabled) &&
This should check if password sync is enabled as well.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.cc:290: UpdatePasswordSyncState(host,
true);
This should just call UpdatePasswordGenerationState, not UpdatePasswordSyncState

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.cc:298: switch (type) {
I'm actually not totally sure that we need this observer. The password manager
for instance only checks if it's enabled/disabled when it's created.  Sync seems
different to me since if it's disabled, the feature will fail in a very bad way.
I guess since it's already here you can leave it.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.cc:315:
UpdatePasswordSyncState(web_contents()->GetRenderViewHost(),
Don't change this.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.h:123: virtual void
SendPasswordSyncStateToRenderer(
It looks like you have two versions of some of these functions that I moved in a
previous changelist. The *PasswordGeneration* functions should be around, but
the *PasswordSync* functions should be removed.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.h:365: bool password_sync_enabled_;
This should be removed.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager_unittest.cc:491: virtual void
SendPasswordSyncStateToRenderer(
Remove this.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/resources/optio...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10222017/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/browser_options.html:240: <div
class="checkbox">
This should be under "Passwords and forms" not "Privacy"

[Ilya Sherman, 2012-04-26 23:30:35]

Is this feature in fact ready for shipping?  I was expecting that this is
targeted at M21; are we targeting M20 instead?

[zysxqn, 2012-04-27 18:38:34]

Browser sends only one message to the renderer now.

Did not reply all your comments since most of them are referring to the issue
above.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.cc:214: true,
On 2012/04/26 23:23:59, Garrett Casto wrote:
> We are going to have this be an opt-in feature at first, so this should
default
> to false.

Done.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.cc:298: switch (type) {
On 2012/04/26 23:23:59, Garrett Casto wrote:
> I'm actually not totally sure that we need this observer. The password manager
> for instance only checks if it's enabled/disabled when it's created.  Sync
seems
> different to me since if it's disabled, the feature will fail in a very bad
way.
> I guess since it's already here you can leave it.

I feel that it is useful since users sometimes expect to see the effect
immediately when they change some preference, so I added it like this.

http://codereview.chromium.org/10222017/diff/1/chrome/browser/resources/optio...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10222017/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/browser_options.html:240: <div
class="checkbox">
On 2012/04/26 23:23:59, Garrett Casto wrote:
> This should be under "Passwords and forms" not "Privacy"

Done.

[zysxqn, 2012-05-01 18:30:48]

Thanks for the sample code. Please take another look.

[commit-bot: I haz the power, 2012-05-01 18:35:16]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Ilya Sherman, 2012-05-01 22:27:47]

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager.cc (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:246: // (1) Password sync is
enabled, and
nit: No need for "and" on this line -- just on the penultimate line in the list.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:248: // (3) Password generation
preference check box is checked.
nit: Please move this comment a bit further up still: immediately above the
method definition.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:250: return;
nit: Please add a blank line after this if-stmt.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:252: bool new_password_sync_enabled
=
nit: There is no "old_password_sync_enabled" to compare to, so please continue
to call this "password_sync_enabled".

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:290: switch (type) {
nit: Since this code currently only listens for exactly one type of
notification, please just start with DCHECK_EQ(chrome::NOTIFICATION_PREF_CHANGE,
type), and skip the switch stmt.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:294: DCHECK(*pref ==
prefs::kPasswordGenerationEnabled);
nit: DCHECK_EQ(prefs::kPasswordGenerationEnabled, *pref)

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:295:
UpdatePasswordGenerationState(web_contents()->GetRenderViewHost(), false);
Why do you always pass |false| for the second parameter?

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.cc:881: // Test code doens't need
registrar_.
nit: "doens't" -> "doesn't"

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager.h (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.h:122: // in password generation. This
is a separate function to aid with testing.
nit: Please remove "for use in password generation" -- it is redundant.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.h:127: 
nit: Please remove this extra blank line.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.h:185: // Determines that the current
state of password generation is, and if it
nit: "that" -> "what"

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.h:352: // If password generation should
be enabled. We cache this value so that we
nit: "should be" -> "is"

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.h:355: bool
password_generation_enabled_;
nit: Should this perhaps be a BooleanPrefMember instead?  You could even pass it
|this| as an observer, and do away with the PrefChangeRegistrar below.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager.h:356: // Handle preference change
event.
nit: This comment is currently redundant with the variable's type.  Perhaps
instead: "Listens for changes to the 'enabled' state for password generation."

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/aut...
chrome/browser/autofill/autofill_manager_unittest.cc:2989: }
nit: Please leave a blank line after this closing curly brace.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/res...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/res...
chrome/browser/resources/options2/browser_options.html:317:
metric="Options_PasswordGenerationCheckbox" type="checkbox">
Please run src/chrome/tools/extract_actions.py and to update the list of metric
name hashes to include this one.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/res...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/res...
chrome/browser/resources/options2/browser_options.js:1044: * @private
nit: Please include a comment describing the purpose/use of this function.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/res...
chrome/browser/resources/options2/browser_options.js:1053: }
nit: Why not set the visibility of the containing <div> element, rather than
setting the visibility individually for the child elements?

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/browser/ui/...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:1339:
switches::kEnablePasswordGeneration) ? true : false);
nit: Please omit the " ? true : false" -- you already have a boolean value.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/common/auto...
File chrome/common/autofill_messages.h (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/common/auto...
chrome/common/autofill_messages.h:124: 
nit: Please remove this extra blank line.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/common/pref...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/common/pref...
chrome/common/pref_names.cc:274: // Boolean that is true when auto password
generation is enabled.
nit: Please omit "auto"

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/common/pref...
File chrome/common/pref_names.h (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/common/pref...
chrome/common/pref_names.h:343: extern const char kPasswordGenerationEnabled[];
nit: Please add this at line 121 (and in the corresponding location in the
implementation file).

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/renderer/au...
File chrome/renderer/autofill/password_generation_manager.cc (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/renderer/au...
chrome/renderer/autofill/password_generation_manager.cc:27: // We don't want to
generate passwords if browser doesn't think so.
nit: Please preserve the previous comment, or else remove this entirely.  The
existing comment adds no information beyond the code itself.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/renderer/au...
chrome/renderer/autofill/password_generation_manager.cc:93:
OnPasswordGenerationEnabled)
Good catch, thanks for fixing this :)

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/renderer/au...
File chrome/renderer/autofill/password_generation_manager.h (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/renderer/au...
chrome/renderer/autofill/password_generation_manager.h:45: // True if auto
password generation is enabled for the profile associated
nit: Please omit "auto"

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/renderer/ch...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/renderer/ch...
chrome/renderer/chrome_content_renderer_client.cc:264: new
PasswordGenerationManager(render_view);
nit: Please omit the blank line between this and the closing curly brace.

https://chromiumcodereview.appspot.com/10222017/diff/11002/chrome/renderer/ch...
chrome/renderer/chrome_content_renderer_client.cc:264: new
PasswordGenerationManager(render_view);
nit: Please move this to be grouped with the other password- and
autofill-related observers.

[zysxqn, 2012-05-03 00:57:24]

Please have another look.

Note that I somehow messed up my client so I did several merges and syncs to
make it clean. As a result, if you look at the diffs you might see some changes
that are not made by me... Sorry for the inconvenience and please refer to my
comments if you are not sure whether a change is made by me or not. Sorry
again...

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:246: // (1) Password sync is
enabled, and
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: No need for "and" on this line -- just on the penultimate line in the
list.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:248: // (3) Password generation
preference check box is checked.
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please move this comment a bit further up still: immediately above the
> method definition.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:250: return;
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please add a blank line after this if-stmt.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:252: bool new_password_sync_enabled
=
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: There is no "old_password_sync_enabled" to compare to, so please continue
> to call this "password_sync_enabled".

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:290: switch (type) {
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Since this code currently only listens for exactly one type of
> notification, please just start with
DCHECK_EQ(chrome::NOTIFICATION_PREF_CHANGE,
> type), and skip the switch stmt.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:294: DCHECK(*pref ==
prefs::kPasswordGenerationEnabled);
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: DCHECK_EQ(prefs::kPasswordGenerationEnabled, *pref)

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:295:
UpdatePasswordGenerationState(web_contents()->GetRenderViewHost(), false);
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> Why do you always pass |false| for the second parameter?

In AutofillManager::RenderViewCreated() we call the function with |true| for the
second parameter, to indicate a newly created render view and to update the
password generation state no matter what's the old state.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:881: // Test code doens't need
registrar_.
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: "doens't" -> "doesn't"

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:122: // in password generation. This
is a separate function to aid with testing.
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please remove "for use in password generation" -- it is redundant.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:127: 
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please remove this extra blank line.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:185: // Determines that the current
state of password generation is, and if it
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: "that" -> "what"

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:352: // If password generation should
be enabled. We cache this value so that we
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: "should be" -> "is"

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:355: bool
password_generation_enabled_;
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Should this perhaps be a BooleanPrefMember instead?  You could even pass
it
> |this| as an observer, and do away with the PrefChangeRegistrar below.

Since it is actually affected by three things: prefs, sync states, and password
manager enabled states, I think keeping it as is is easier. thoughts?

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:356: // Handle preference change
event.
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: This comment is currently redundant with the variable's type.  Perhaps
> instead: "Listens for changes to the 'enabled' state for password generation."

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager_unittest.cc:2989: }
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please leave a blank line after this closing curly brace.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/resources/o...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:317:
metric="Options_PasswordGenerationCheckbox" type="checkbox">
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> Please run src/chrome/tools/extract_actions.py and to update the list of
metric
> name hashes to include this one.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/resources/o...
File chrome/browser/resources/options2/browser_options.js (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.js:1044: * @private
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please include a comment describing the purpose/use of this function.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.js:1053: }
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Why not set the visibility of the containing <div> element, rather than
> setting the visibility individually for the child elements?

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:1339:
switches::kEnablePasswordGeneration) ? true : false);
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please omit the " ? true : false" -- you already have a boolean value.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/common/autofill_mes...
File chrome/common/autofill_messages.h (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/common/autofill_mes...
chrome/common/autofill_messages.h:124: 
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please remove this extra blank line.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/common/pref_names.c...
chrome/common/pref_names.cc:274: // Boolean that is true when auto password
generation is enabled.
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please omit "auto"

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/common/pref_names.h...
chrome/common/pref_names.h:343: extern const char kPasswordGenerationEnabled[];
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please add this at line 121 (and in the corresponding location in the
> implementation file).

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/renderer/autofill/p...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager.cc:27: // We don't want to
generate passwords if browser doesn't think so.
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please preserve the previous comment, or else remove this entirely.  The
> existing comment adds no information beyond the code itself.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/renderer/autofill/p...
File chrome/renderer/autofill/password_generation_manager.h (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager.h:45: // True if auto
password generation is enabled for the profile associated
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please omit "auto"

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:264: new
PasswordGenerationManager(render_view);
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please omit the blank line between this and the closing curly brace.

Done.

http://codereview.chromium.org/10222017/diff/11002/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:264: new
PasswordGenerationManager(render_view);
On 2012/05/01 22:27:48, Ilya Sherman wrote:
> nit: Please move this to be grouped with the other password- and
> autofill-related observers.

Done.

[Ilya Sherman, 2012-05-04 06:04:50]

LGTM with a few more nits.  Thanks.

Please wait for Garrett's LGTM as well, in case he has any additional comments.

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:295:
UpdatePasswordGenerationState(web_contents()->GetRenderViewHost(), false);
On 2012/05/03 00:57:24, zysxqn wrote:
> On 2012/05/01 22:27:48, Ilya Sherman wrote:
> > Why do you always pass |false| for the second parameter?
> 
> In AutofillManager::RenderViewCreated() we call the function with |true| for
the
> second parameter, to indicate a newly created render view and to update the
> password generation state no matter what's the old state.

Sorry, I misread this as calling SendPasswordGenerationStateToRenderer() rather
than UpdatePasswordGenerationState().  Looks fine now that I'm reading it
correctly :)

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10222017/diff/11002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:355: bool
password_generation_enabled_;
On 2012/05/03 00:57:24, zysxqn wrote:
> On 2012/05/01 22:27:48, Ilya Sherman wrote:
> > nit: Should this perhaps be a BooleanPrefMember instead?  You could even
pass
> it
> > |this| as an observer, and do away with the PrefChangeRegistrar below.
> 
> Since it is actually affected by three things: prefs, sync states, and
password
> manager enabled states, I think keeping it as is is easier. thoughts?

Ok, that makes sense -- I hadn't noticed when I wrote this comment that this
boolean reflects the combination of multiple underlying states.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:281: }
nit: Why is it helpful to move this method implementation?  Ideally the
implementation file's method ordering would match the header; but otherwise,
it's preferable not to move code around, so that blame history is better
preserved.  In this case, it looks like neither the old nor the new location
match the header order, so we might as well keep the pre-existing one.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:189: // has changed from
password_generation_enabled_. If it has changed or if
nit: Looks like "has" can fit on the previous line.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:357: // states are changing rapidly.
nit: I believe that of the underlying states, on the sync state is changing
rapidly, so the previous phrasing is probably preferable.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager_unittest.cc:2938: }
nit: Why is it helpful to move this test?  Again, other things equal, it's
preferable not to move code around so as to better preserve blame history.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/resources/o...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:314: <div
class="checkbox" id="passwordGenerationCheckbox">
nit: Ids should use hyphenated-name-casing

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:316: <input
id="passwordGenerationEnabled" pref="password_generation.enabled"
nit: Ditto, though in this case perhaps we can remove the id altogether?

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/resources/o...
File chrome/browser/resources/options2/browser_options.js (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.js:1052: }
nit: No need for curly braces for single-line stmts.

http://codereview.chromium.org/10222017/diff/25002/chrome/common/autofill_mes...
File chrome/common/autofill_messages.h (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/common/autofill_mes...
chrome/common/autofill_messages.h:121: // Tells the renderer if password
generation preference is enabled.
nit: "preference" is too narrow, since it also depends on e.g. the Sync state. 
Perhaps just "Tells the renderer whether password generation is enabled."?

[zysxqn, 2012-05-04 18:52:54]

All addressed. Waiting for Garrett's LGTM.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:281: }
On 2012/05/04 06:04:50, Ilya Sherman wrote:
> nit: Why is it helpful to move this method implementation?  Ideally the
> implementation file's method ordering would match the header; but otherwise,
> it's preferable not to move code around, so that blame history is better
> preserved.  In this case, it looks like neither the old nor the new location
> match the header order, so we might as well keep the pre-existing one.

Done.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:189: // has changed from
password_generation_enabled_. If it has changed or if
On 2012/05/04 06:04:50, Ilya Sherman wrote:
> nit: Looks like "has" can fit on the previous line.

Done.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:357: // states are changing rapidly.
On 2012/05/04 06:04:50, Ilya Sherman wrote:
> nit: I believe that of the underlying states, on the sync state is changing
> rapidly, so the previous phrasing is probably preferable.

Done.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager_unittest.cc:2938: }
On 2012/05/04 06:04:50, Ilya Sherman wrote:
> nit: Why is it helpful to move this test?  Again, other things equal, it's
> preferable not to move code around so as to better preserve blame history.

Done.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/resources/o...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:314: <div
class="checkbox" id="passwordGenerationCheckbox">
On 2012/05/04 06:04:50, Ilya Sherman wrote:
> nit: Ids should use hyphenated-name-casing

Done.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:316: <input
id="passwordGenerationEnabled" pref="password_generation.enabled"
On 2012/05/04 06:04:50, Ilya Sherman wrote:
> nit: Ditto, though in this case perhaps we can remove the id altogether?

Done.

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/resources/o...
File chrome/browser/resources/options2/browser_options.js (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.js:1052: }
On 2012/05/04 06:04:50, Ilya Sherman wrote:
> nit: No need for curly braces for single-line stmts.

Done.

http://codereview.chromium.org/10222017/diff/25002/chrome/common/autofill_mes...
File chrome/common/autofill_messages.h (right):

http://codereview.chromium.org/10222017/diff/25002/chrome/common/autofill_mes...
chrome/common/autofill_messages.h:121: // Tells the renderer if password
generation preference is enabled.
On 2012/05/04 06:04:50, Ilya Sherman wrote:
> nit: "preference" is too narrow, since it also depends on e.g. the Sync state.

> Perhaps just "Tells the renderer whether password generation is enabled."?

Done.

[Ilya Sherman, 2012-05-04 21:27:03]

+jhawkins for WebUI approval

[James Hawkins, 2012-05-07 21:53:12]

webui LGTM

[Garrett Casto, 2012-05-11 23:01:28]

Mostly looks good. Just a few more things.

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:213: false,
I'd think that I'd prefer this default to true for now to make testing/debugging
easier. With the changes I mentioned in the renderer I believe that this is
safe. If the command line flag isn't set, the feature won't be enabled and the
pref won't be visible.

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:126: // This is a separate function
to aid with testing.
I'm assuming that this is changed because of a sync issue?

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:189: // changed from
password_generation_enabled_. If it has changed or if
Same as above?

http://codereview.chromium.org/10222017/diff/33002/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (left):

http://codereview.chromium.org/10222017/diff/33002/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:263: if
(CommandLine::ForCurrentProcess()->HasSwitch(
I would leave this behind a flag for now. I don't want to expose normal users to
possible bugs from password generation while this is still under development.

[zysxqn, 2012-05-11 23:52:50]

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:213: false,
On 2012/05/11 23:01:28, Garrett Casto wrote:
> I'd think that I'd prefer this default to true for now to make
testing/debugging
> easier. With the changes I mentioned in the renderer I believe that this is
> safe. If the command line flag isn't set, the feature won't be enabled and the
> pref won't be visible. 

Done.

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:126: // This is a separate function
to aid with testing.
On 2012/05/11 23:01:28, Garrett Casto wrote:
> I'm assuming that this is changed because of a sync issue? 

Yes, I don't think I have intended to change this. I guess the new version is
fine.

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:189: // changed from
password_generation_enabled_. If it has changed or if
On 2012/05/11 23:01:28, Garrett Casto wrote:
> Same as above?

Yes, again I don't think I have intended to change this. However, in this case I
think previous version is better. Changed.

http://codereview.chromium.org/10222017/diff/33002/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (left):

http://codereview.chromium.org/10222017/diff/33002/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:263: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/05/11 23:01:28, Garrett Casto wrote:
> I would leave this behind a flag for now. I don't want to expose normal users
to
> possible bugs from password generation while this is still under development. 

Done.

[Garrett Casto, 2012-05-14 20:24:10]

lgtm

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:126: // This is a separate function
to aid with testing.
On 2012/05/11 23:52:51, zysxqn wrote:
> On 2012/05/11 23:01:28, Garrett Casto wrote:
> > I'm assuming that this is changed because of a sync issue? 
> 
> Yes, I don't think I have intended to change this. I guess the new version is
> fine.

It actually only informs one renderer, so changing it to singular tense would be
nice.

[zysxqn, 2012-05-14 20:47:34]

Submitting...

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/10222017/diff/33002/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:126: // This is a separate function
to aid with testing.
On 2012/05/14 20:24:11, Garrett Casto wrote:
> On 2012/05/11 23:52:51, zysxqn wrote:
> > On 2012/05/11 23:01:28, Garrett Casto wrote:
> > > I'm assuming that this is changed because of a sync issue? 
> > 
> > Yes, I don't think I have intended to change this. I guess the new version
is
> > fine.
> 
> It actually only informs one renderer, so changing it to singular tense would
be
> nice.

Done.

[commit-bot: I haz the power, 2012-05-14 20:59:30]

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

[commit-bot: I haz the power, 2012-05-14 21:44:40]

Try job failure for 10222017-37003 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-14 22:32:59]

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

[commit-bot: I haz the power, 2012-05-14 23:16:37]

Try job failure for 10222017-37003 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-15 17:09:40]

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

[commit-bot: I haz the power, 2012-05-15 17:09:51]

Can't apply patch for file chrome/browser/autofill/autofill_manager.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/autofill/autofill_manager.cc
Hunk #3 FAILED at 245.
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/autofill/autofill_manager.cc.rej

[commit-bot: I haz the power, 2012-05-15 17:57:46]

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

[commit-bot: I haz the power, 2012-05-15 17:57:59]

Can't apply patch for file chrome/browser/autofill/autofill_manager.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/autofill/autofill_manager.cc
Hunk #3 FAILED at 245.
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/autofill/autofill_manager.cc.rej

[Brian Ryner, 2012-05-15 18:00:26]

If the problem is that the patch won't apply, it means there is a merge
conflict.  You'll need to sync your client, merge the changes, and upload a new
patch.

[commit-bot: I haz the power, 2012-05-15 21:48:19]

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

[commit-bot: I haz the power, 2012-05-15 21:49:01]

Can't apply patch for file chrome/browser/autofill/autofill_manager.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/autofill/autofill_manager.cc
Hunk #3 FAILED at 245.
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/autofill/autofill_manager.cc.rej

[commit-bot: I haz the power, 2012-05-15 22:02:02]

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

[commit-bot: I haz the power, 2012-05-15 22:46:11]

Try job failure for 10222017-34035 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[zysxqn, 2012-05-16 18:02:58]

I have fixed the unit test fail in my client. Ilya, do you want to take a quick
look at since autofill_external_delegate_unittest.cc has been changed.

[Ilya Sherman, 2012-05-16 18:10:38]

https://chromiumcodereview.appspot.com/10222017/diff/41010/chrome/browser/aut...
File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right):

https://chromiumcodereview.appspot.com/10222017/diff/41010/chrome/browser/aut...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:61: :
AutofillManager(tab_contents, personal_data) {}
nit: Rather than adding a new parameter for |personal_data|, just pass NULL as
the second argument to the AutofillManager constructor, since that's what all
the tests in this file want.

[zysxqn, 2012-05-16 18:25:02]

Done and submitting.

http://codereview.chromium.org/10222017/diff/41010/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right):

http://codereview.chromium.org/10222017/diff/41010/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_external_delegate_unittest.cc:61: :
AutofillManager(tab_contents, personal_data) {}
On 2012/05/16 18:10:38, Ilya Sherman wrote:
> nit: Rather than adding a new parameter for |personal_data|, just pass NULL as
> the second argument to the AutofillManager constructor, since that's what all
> the tests in this file want.

Done.

[commit-bot: I haz the power, 2012-05-16 18:25:42]

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

[commit-bot: I haz the power, 2012-05-16 19:47:02]

Try job failure for 10222017-48004 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-16 22:24:42]

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

[commit-bot: I haz the power, 2012-05-17 00:10:07]

Change committed as 137573