Check user state and settings *before* forcing credit card upload on.

Check user state and settings *before* forcing credit card upload on.

The command-line flag should enable the feature, not force it on despite the user settings and logged-in state.

BUG=571631
Committed: https://crrev.com/9ce4ada90007521f2719d892f1db77bc9ebac2e0
Cr-Commit-Position: refs/heads/master@{#366668}

[Justin Donnelly, 2015-12-22 17:04:38]

jdonnelly@chromium.org changed reviewers:
+ estade@chromium.org

[Justin Donnelly, 2015-12-22 17:59:15]

Description was changed from

==========
Check user state and settings *before* forcing credit card upload on.

The command-line flag should enable the feature, not force it on despite the
user settings and logged-in state.

BUG=571631
==========

to

==========
Check user state and settings *before* forcing credit card upload on.

The command-line flag should enable the feature, not force it on despite the
user settings and logged-in state.

BUG=571631
==========

[Justin Donnelly, 2015-12-22 17:59:16]

jdonnelly@chromium.org changed reviewers:
+ isherman@chromium.org
- estade@chromium.org

[Ilya Sherman, 2015-12-22 21:32:06]

https://codereview.chromium.org/1545683003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/1545683003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:71: }
What Finch/variations group do you expect to be reported if any of these early
returns are reached?

[Justin Donnelly, 2015-12-22 21:38:29]

https://codereview.chromium.org/1545683003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/1545683003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:71: }
On 2015/12/22 21:32:05, Ilya Sherman wrote:
> What Finch/variations group do you expect to be reported if any of these early
> returns are reached?

My (limited, possibly incorrect) understanding is that users won't get reported
to be part of any group (experiment, control, default) if they don't reach the
FindFullName call. If it were moved after these checks, the population sizes
could change over time, not match between control and experiment, etc.

[Ilya Sherman, 2015-12-22 21:52:02]

LGTM

https://codereview.chromium.org/1545683003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/1545683003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:71: }
On 2015/12/22 21:38:29, Justin Donnelly wrote:
> On 2015/12/22 21:32:05, Ilya Sherman wrote:
> > What Finch/variations group do you expect to be reported if any of these
early
> > returns are reached?
> 
> My (limited, possibly incorrect) understanding is that users won't get
reported
> to be part of any group (experiment, control, default) if they don't reach the
> FindFullName call. If it were moved after these checks, the population sizes
> could change over time, not match between control and experiment, etc.

Yes, your understanding is correct, and I see your point.  Thanks :)

[Justin Donnelly, 2015-12-22 22:10:01]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2015-12-22 22:10:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1545683003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1545683003/20001

[commit-bot: I haz the power, 2015-12-22 22:15:17]

Description was changed from

==========
Check user state and settings *before* forcing credit card upload on.

The command-line flag should enable the feature, not force it on despite the
user settings and logged-in state.

BUG=571631
==========

to

==========
Check user state and settings *before* forcing credit card upload on.

The command-line flag should enable the feature, not force it on despite the
user settings and logged-in state.

BUG=571631
==========

[commit-bot: I haz the power, 2015-12-22 22:15:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-22 22:16:32]

Description was changed from

==========
Check user state and settings *before* forcing credit card upload on.

The command-line flag should enable the feature, not force it on despite the
user settings and logged-in state.

BUG=571631
==========

to

==========
Check user state and settings *before* forcing credit card upload on.

The command-line flag should enable the feature, not force it on despite the
user settings and logged-in state.

BUG=571631

Committed: https://crrev.com/9ce4ada90007521f2719d892f1db77bc9ebac2e0
Cr-Commit-Position: refs/heads/master@{#366668}
==========

[commit-bot: I haz the power, 2015-12-22 22:16:33]

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

[Evan Stade, 2015-12-29 17:46:43]

estade@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2015-12-29 17:46:44]

lgtm

https://codereview.chromium.org/1545683003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/1545683003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:79: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
this one can still be above

[Justin Donnelly, 2015-12-29 18:35:19]

https://codereview.chromium.org/1545683003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/1545683003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:79: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/12/29 17:46:44, Evan Stade wrote:
> this one can still be above

True. But I prefer to keep it with the other command-line check for readability.