Issue 10168017: Only enable password generation if password manager and autofill are both

Issue 10168017: Only enable password generation if password manager and autofill are both (Closed)

Created:
8 years, 8 months ago by Garrett Casto

Modified:
8 years, 8 months ago

Reviewers:
gcasto (DO NOT USE), Ilya Sherman, Brian Ryner

CC:
chromium-reviews, dhollowa+watch_chromium.org, darin-cc_chromium.org, dyu1, brettw-cc_chromium.org, Ilya Sherman

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Only enable password generation if password manager and autofill are both enabled. BUG=124112 TEST=Ran unittests Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133808

Patch Set 2 : Update comments #

Patch Set 3 : Fix browsertest #

Patch Set 4 : Fix linux_asan #

Total comments: 15

Patch Set 5 : Address comments #

Patch Set 6 : Finish addressing comments #

Patch Set 7 : Remove autofill requirement #

Total comments: 1

Patch Set 8 : More comments #

Created: 8 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+75 lines, -55 lines)			Patch
M	chrome/browser/autofill/autofill_manager.h	View	1 2 3 4	3 chunks	+12 lines, -11 lines	0 comments	Download
M	chrome/browser/autofill/autofill_manager.cc	View	1 2 3 4 5 6 7	4 chunks	+22 lines, -14 lines	0 comments	Download
M	chrome/browser/autofill/autofill_manager_unittest.cc	View	1 2 3 4 5 6	6 chunks	+21 lines, -10 lines	0 comments	Download
M	chrome/browser/password_manager/password_manager.h	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	chrome/browser/password_manager/password_manager.cc	View	1 2 3 4 5	2 chunks	+5 lines, -5 lines	0 comments	Download
M	chrome/common/autofill_messages.h	View		1 chunk	+2 lines, -3 lines	0 comments	Download
M	chrome/renderer/autofill/password_generation_manager.h	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/renderer/autofill/password_generation_manager.cc	View		3 chunks	+6 lines, -5 lines	0 comments	Download
M	chrome/renderer/autofill/password_generation_manager_browsertest.cc	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

Ilya Sherman

http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/autofill_manager.cc File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/autofill_manager.cc#newcode250 chrome/browser/autofill/autofill_manager.cc:250: // password manager to all be enabled. nit: This ...

8 years, 8 months ago (2012-04-24 00:46:54 UTC) #2

Garrett Casto

8 years, 8 months ago (2012-04-24 18:49:56 UTC) #3

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

http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:250: // password manager to all be
enabled.
On 2012/04/24 00:46:54, Ilya Sherman wrote:
> nit: This currently reads as "requires sync for ... the password manager to
...
> be enabled".  Perhaps word this as "Password generation requires the password
> manager and sync for passwords and autofill to all be enabled" ?
> 
> (Also, why does Autofill data need to be being synced -- why isn't passwords
> data sufficient?)

Autofill data doesn't need to be synced, autofill just needs to be enabled. The
three things that I'm trying to enumerate are "sync for passwords" "autofill"
and "password manager". If this wording is confusing, I'm fine with changing it
but your proposed change has a different meaning :)

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

http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:119: // password generation. This is
a separate function to aid with testing.
On 2012/04/24 00:46:54, Ilya Sherman wrote:
> nit: Should the reference to "sync" in this comment also be updated?

Done.

http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.h:177: // changed from
password_generation_enabled_. If it has changed or if
On 2012/04/24 00:46:54, Ilya Sherman wrote:
> nit: password_generation_enabled_ -> |password_generation_enabled_|

Done.

http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager_unittest.cc (left):

http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager_unittest.cc:2920: // Disable syncing.
This should be sent.
On 2012/04/24 00:46:54, Ilya Sherman wrote:
> Hmm, why is this case (disabling sync) removed from the test?

Re-added. Just got lost in the shuffle.

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

http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager_unittest.cc:589: TearDown();
On 2012/04/24 00:46:54, Ilya Sherman wrote:
> Hmm, I'm not sure this is safe.  For example,
> chrome_render_view_host_test_harness.cc:49 calls
"ash::Shell::DeleteInstance();"
> if ash is enabled -- are you sure that's safe to call into multiple times, and
> to run test code after calling it the first time?
> 

I'm not sure how this is different from the normal state of affairs. My
understanding is that between tests in a testcase, all that happens is
TearDown/SetUp. This passes the default trybots plus linux_asan, though
admittedly I don't know if any of them have ash enabled.

> I think instead of re-using SetUp and TearDown, you'll want to extract just
the
> parts specific to this test, and call those bits directly.

So it actually turns out that there is a much easier way to do this.
TestingProfile::set_incognito(). I missed it on my first read through :(. So
this is all a moot point.

http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager_unittest.cc:2944: // This should still
leave the feature disabled, and shouldn't send anything.
On 2012/04/24 00:46:54, Ilya Sherman wrote:
> nit: The "still" in this comment seems a little bit misleading to me, since
> you're actually creating a brand new AutofillManager as part of
> GoOffTheRecord().

N/A

http://codereview.chromium.org/10168017/diff/20001/chrome/browser/password_ma...
File chrome/browser/password_manager/password_manager.h (right):

http://codereview.chromium.org/10168017/diff/20001/chrome/browser/password_ma...
chrome/browser/password_manager/password_manager.h:41: bool IsEnabled() const;
On 2012/04/24 00:46:54, Ilya Sherman wrote:
> nit: Please move this function's definition within the implementation file as
> well.

Done.

Ilya Sherman

8 years, 8 months ago (2012-04-24 20:30:26 UTC) #4

gcasto (DO NOT USE)

On Tue, Apr 24, 2012 at 1:30 PM, <isherman@chromium.org> wrote: > > http://codereview.chromium.**org/10168017/diff/20001/** > chrome/browser/autofill/**autofill_manager.cc<http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/autofill_manager.cc> ...

8 years, 8 months ago (2012-04-24 21:52:38 UTC) #5

On Tue, Apr 24, 2012 at 1:30 PM, <isherman@chromium.org> wrote:

>
> http://codereview.chromium.**org/10168017/diff/20001/**
>
chrome/browser/autofill/**autofill_manager.cc<http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/autofill_manager.cc>
> File chrome/browser/autofill/**autofill_manager.cc (right):
>
> http://codereview.chromium.**org/10168017/diff/20001/**
>
chrome/browser/autofill/**autofill_manager.cc#newcode250<http://codereview.chromium.org/10168017/diff/20001/chrome/browser/autofill/autofill_manager.cc#newcode250>
> chrome/browser/autofill/**autofill_manager.cc:250: // password manager to
> all be enabled.
> On 2012/04/24 18:49:56, Garrett Casto wrote:
>
>> On 2012/04/24 00:46:54, Ilya Sherman wrote:
>> > nit: This currently reads as "requires sync for ... the password
>>
> manager to
>
>> ...
>> > be enabled".  Perhaps word this as "Password generation requires the
>>
> password
>
>> > manager and sync for passwords and autofill to all be enabled" ?
>> >
>> > (Also, why does Autofill data need to be being synced -- why isn't
>>
> passwords
>
>> > data sufficient?)
>>
>
>  Autofill data doesn't need to be synced, autofill just needs to be
>>
> enabled. The
>
>> three things that I'm trying to enumerate are "sync for passwords"
>>
> "autofill"
>
>> and "password manager". If this wording is confusing, I'm fine with
>>
> changing it
>
>> but your proposed change has a different meaning :)
>>
>
> Ah, yeah, I totally misread that comment.  If you mean that the Autofill
> feature needs to be enabled, I would definitely re-arrange it so that
> password sync is listed last, since Autofill is also a syncable type.
>
> It's not clear to me, though, why Autofill needs to be enabled in the
> first place.  Password saving can be enabled even with Autofill
> disabled, and password generation sure feels like a feature that just
> cares about passwords.  What am I missing?
>
>
I thought that password autofill was tied into autofill, but looking
through right now it seems like that's just not true. I've removed the
autofill requirement. This probably also suggests that the code for
password generation should live somewhere in browser/password_manager, but
I can deal with that later.

http://codereview.chromium.**org/10168017/<http://codereview.chromium.org/101...
>

Ilya Sherman

LGTM http://codereview.chromium.org/10168017/diff/33002/chrome/browser/autofill/autofill_manager.cc File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10168017/diff/33002/chrome/browser/autofill/autofill_manager.cc#newcode250 chrome/browser/autofill/autofill_manager.cc:250: // to all be enabled. nit: "all" -> ...

8 years, 8 months ago (2012-04-24 21:57:44 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gcasto@chromium.org/10168017/24003

8 years, 8 months ago (2012-04-24 22:03:54 UTC) #7

Change committed as 133808

Expand Messages | Collapse Messages