Autofill Credit Card Signin Promo: Put behind Feature

Now behind a Feature and also will not show if the number of impressions is higher than some value passed by Variations params.

BUG=626442
TEST=AutofillManagerTest

Committed: https://crrev.com/97020bfe9122ac4e0c0ede110b328db5d1193037
Cr-Commit-Position: refs/heads/master@{#405615}

[Mathieu, 2016-07-13 20:30:46]

Patchset #1 (id:1) has been deleted

[Mathieu, 2016-07-13 20:30:52]

Patchset #1 (id:20001) has been deleted

[Mathieu, 2016-07-13 20:31:11]

Description was changed from

==========
Autofill: draft about signin promo impressions

BUG=
==========

to

==========
Autofill: draft about signin promo impressions

BUG=626442
TEST=AutofillManagerTest
==========

[Mathieu, 2016-07-13 20:31:11]

mathp@chromium.org changed reviewers:
+ sebsg@chromium.org

[Mathieu, 2016-07-13 20:32:15]

Description was changed from

==========
Autofill: draft about signin promo impressions

BUG=626442
TEST=AutofillManagerTest
==========

to

==========
Now behind a Feature and also will not show if the number of impressions is
higher than some value passed by Variations params.

BUG=626442
TEST=AutofillManagerTest
==========

[Mathieu, 2016-07-13 20:35:29]

Patchset #1 (id:40001) has been deleted

[Mathieu, 2016-07-13 20:35:48]

Hi Seb, have a look?

[sebsg, 2016-07-14 15:32:13]

LGTM with a few small nits. Thanks!

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:298: // The promo is
guaranteed to be shown. Increment the impression count.
Nit: I'm not sure about the phrasing. From what I understand, if we get here (in
the if) the promo will be shown but if we don't, the promo will never be shown. 

The word guaranteed makes me feel like the other way can sometime show the promo
too.

I'd replace by: The promo will be shown. Increment...

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:4951:
TEST_F(AutofillManagerTest, ShouldShowCreditCardSigninPromo_CreditCardField) {
Nit: add _NoLimit as a suffix to be more consistent with the rest of the tests.

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:4994:
ShouldShowCreditCardSigninPromo_CreditCardField_WithUnmetLimit) {
Nit: I think ...Field_UnmetLimit gives the same information with less
characters. But that's just me being picky.

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:5019:
.WillOnce(testing::Return(false));
Could you clarify that you are testing 2 separate ways of showing or not showing
the promo here? It was not obvious to me when I read it. Thanks!

[Mathieu, 2016-07-14 21:51:30]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[Mathieu, 2016-07-14 21:51:44]

Thanks! Will submit soon.

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:298: // The promo is
guaranteed to be shown. Increment the impression count.
On 2016/07/14 15:32:13, sebsg wrote:
> Nit: I'm not sure about the phrasing. From what I understand, if we get here
(in
> the if) the promo will be shown but if we don't, the promo will never be
shown. 
> 
> The word guaranteed makes me feel like the other way can sometime show the
promo
> too.
> 
> I'd replace by: The promo will be shown. Increment...
> 

Done.

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:4951:
TEST_F(AutofillManagerTest, ShouldShowCreditCardSigninPromo_CreditCardField) {
On 2016/07/14 15:32:13, sebsg wrote:
> Nit: add _NoLimit as a suffix to be more consistent with the rest of the
tests.

Done.

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:4994:
ShouldShowCreditCardSigninPromo_CreditCardField_WithUnmetLimit) {
On 2016/07/14 15:32:13, sebsg wrote:
> Nit: I think ...Field_UnmetLimit gives the same information with less
> characters. But that's just me being picky.

Done.

https://codereview.chromium.org/2146823003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:5019:
.WillOnce(testing::Return(false));
On 2016/07/14 15:32:13, sebsg wrote:
> Could you clarify that you are testing 2 separate ways of showing or not
showing
> the promo here? It was not obvious to me when I read it. Thanks!

Done.

[commit-bot: I haz the power, 2016-07-14 21:52:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2016-07-14 21:55:49]

The CQ bit was unchecked by mathp@chromium.org

[Mathieu, 2016-07-14 21:55:50]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-07-14 21:55:50]

The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org
Link to the patchset: https://codereview.chromium.org/2146823003/#ps80001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-07-14 21:56:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 23:05:42]

Description was changed from

==========
Now behind a Feature and also will not show if the number of impressions is
higher than some value passed by Variations params.

BUG=626442
TEST=AutofillManagerTest
==========

to

==========
Now behind a Feature and also will not show if the number of impressions is
higher than some value passed by Variations params.

BUG=626442
TEST=AutofillManagerTest
==========

[commit-bot: I haz the power, 2016-07-14 23:05:43]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-07-14 23:07:21]

Description was changed from

==========
Now behind a Feature and also will not show if the number of impressions is
higher than some value passed by Variations params.

BUG=626442
TEST=AutofillManagerTest
==========

to

==========
Now behind a Feature and also will not show if the number of impressions is
higher than some value passed by Variations params.

BUG=626442
TEST=AutofillManagerTest

Committed: https://crrev.com/97020bfe9122ac4e0c0ede110b328db5d1193037
Cr-Commit-Position: refs/heads/master@{#405615}
==========

[commit-bot: I haz the power, 2016-07-14 23:07:22]

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