[Autofill] Implement Credit Card Signin Promo

[Autofill] Implement Credit Card Signin Promo

A follow-up change will split the slightly long promo over 2-lines for Views, Mac, Android.

Also, Android and iOS will need bridges into Java and ObjC, so this is deferred to another change.

BUG=626442
Committed: https://crrev.com/4d08afd67d6c706fa47d5633187687e690b2fed0
Cr-Commit-Position: refs/heads/master@{#404470}

[Mathieu, 2016-07-07 16:51:01]

Description was changed from

==========
[Autofill] Implement Credit Card Signin Promo

A follow-up change will split the slightly long promo over 2-lines for Views,
Mac, Android.

BUG=625310
==========

to

==========
[Autofill] Implement Credit Card Signin Promo

A follow-up change will split the slightly long promo over 2-lines for Views,
Mac, Android.

Also, Android and iOS will need bridges into Java and ObjC, so this is deferred
to another change.

BUG=625310
==========

[Mathieu, 2016-07-07 16:53:47]

Patchset #2 (id:20001) has been deleted

[Mathieu, 2016-07-07 16:53:53]

Patchset #1 (id:1) has been deleted

[Mathieu, 2016-07-07 17:00:24]

mathp@chromium.org changed reviewers:
+ achuith@chromium.org, atwilson@chromium.org, estade@chromium.org,
torne@chromium.org

[Mathieu, 2016-07-07 17:00:24]

Hi, PTAL, Thanks!

estade@: chrome/browser/ui/autofill and overall logic

atwilson@: components/signin/core/browser (x-small)

torne@: android_webview (x-small)

achuith@: chrome/browser/ui/webui/signin/ (x-small)

[Mathieu, 2016-07-07 20:23:46]

Description was changed from

==========
[Autofill] Implement Credit Card Signin Promo

A follow-up change will split the slightly long promo over 2-lines for Views,
Mac, Android.

Also, Android and iOS will need bridges into Java and ObjC, so this is deferred
to another change.

BUG=625310
==========

to

==========
[Autofill] Implement Credit Card Signin Promo

A follow-up change will split the slightly long promo over 2-lines for Views,
Mac, Android.

Also, Android and iOS will need bridges into Java and ObjC, so this is deferred
to another change.

BUG=626442
==========

[achuithb, 2016-07-07 21:25:42]

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

[commit-bot: I haz the power, 2016-07-07 21:26:53]

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

[achuithb, 2016-07-07 21:27:01]

On 2016/07/07 17:00:24, Mathieu Perreault wrote:
> Hi, PTAL, Thanks!
> 
> estade@: chrome/browser/ui/autofill and overall logic
> 
> atwilson@: components/signin/core/browser (x-small)
> 
> torne@: android_webview (x-small)
> 
> achuith@: chrome/browser/ui/webui/signin/ (x-small)

lgtm for c/b/ui/webui/signin

[commit-bot: I haz the power, 2016-07-07 22:13:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 22:13:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Andrew T Wilson (Slow), 2016-07-08 08:24:57]

signin/core/browser lgtm

[Torne, 2016-07-08 10:09:58]

android_webview LGTM

[Evan Stade, 2016-07-08 17:34:59]

https://codereview.chromium.org/2124343002/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/2124343002/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:350: if (!profile)
can this actually be null? many other places in this file (constructor included)
assume it can't be.

https://codereview.chromium.org/2124343002/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:360: Browser* browser =
chrome::FindBrowserWithWebContents(web_contents());
ditto -- can this validly be null? I think it could only be null for an app
window maybe? Seems like we shouldn't offer the signin option in the first place
if this isn't going to do anything.

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

https://codereview.chromium.org/2124343002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:280: if (!autofill_field ||
autofill_field->Type().group() != CREDIT_CARD) {
nit: return autofill_field && autofill_field->Type().group() == CREDIT_CARD &&
client_->ShouldShowSigninPromo();

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

https://codereview.chromium.org/2124343002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:97: // Whether we should
show the sign in promo, based on the triggered |field|
nit: for consistency, this should be "signin" everywhere and not "sign-in" or
"sign in". ("sign in" is the verb form but I don't see any usages of the verb
here)

https://codereview.chromium.org/2124343002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:98: // inside the |form|.
Will also check whether sign-in is possible for this
nit: the last sentence is a bit redundant/confusing imho

https://codereview.chromium.org/2124343002/diff/60001/components/signin/core/...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/2124343002/diff/60001/components/signin/core/...
components/signin/core/browser/signin_metrics.h:141:
ACCESS_POINT_AUTOFILL_DROPDOWN,
don't you need to add this to the SigninAccessPoint enum in histograms.xml?

https://codereview.chromium.org/2124343002/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2124343002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:158: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
you don't need to fill this in?

[Mathieu, 2016-07-08 18:05:35]

Patchset #3 (id:80001) has been deleted

[Mathieu, 2016-07-08 18:09:38]

Patchset #3 (id:100001) has been deleted

[Mathieu, 2016-07-08 18:10:01]

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

[Mathieu, 2016-07-08 18:10:02]

Thanks! PTAL.

+rkaplow metrics

https://codereview.chromium.org/2124343002/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/2124343002/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:350: if (!profile)
On 2016/07/08 17:34:58, Evan Stade wrote:
> can this actually be null? many other places in this file (constructor
included)
> assume it can't be.

Done.

https://codereview.chromium.org/2124343002/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.cc:360: Browser* browser =
chrome::FindBrowserWithWebContents(web_contents());
On 2016/07/08 17:34:58, Evan Stade wrote:
> ditto -- can this validly be null? I think it could only be null for an app
> window maybe? Seems like we shouldn't offer the signin option in the first
place
> if this isn't going to do anything.

Good point. I'll check for existence of Browser in ShouldShowSigninFlow and
assume for this function that if there's a browser, there's a window().

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

https://codereview.chromium.org/2124343002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:280: if (!autofill_field ||
autofill_field->Type().group() != CREDIT_CARD) {
On 2016/07/08 17:34:58, Evan Stade wrote:
> nit: return autofill_field && autofill_field->Type().group() == CREDIT_CARD &&
> client_->ShouldShowSigninPromo();

Neater thanks

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

https://codereview.chromium.org/2124343002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:97: // Whether we should
show the sign in promo, based on the triggered |field|
On 2016/07/08 17:34:58, Evan Stade wrote:
> nit: for consistency, this should be "signin" everywhere and not "sign-in" or
> "sign in". ("sign in" is the verb form but I don't see any usages of the verb
> here)

Done.

https://codereview.chromium.org/2124343002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:98: // inside the |form|.
Will also check whether sign-in is possible for this
On 2016/07/08 17:34:58, Evan Stade wrote:
> nit: the last sentence is a bit redundant/confusing imho

Done.

https://codereview.chromium.org/2124343002/diff/60001/components/signin/core/...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/2124343002/diff/60001/components/signin/core/...
components/signin/core/browser/signin_metrics.h:141:
ACCESS_POINT_AUTOFILL_DROPDOWN,
On 2016/07/08 17:34:59, Evan Stade wrote:
> don't you need to add this to the SigninAccessPoint enum in histograms.xml?

Done.

https://codereview.chromium.org/2124343002/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2124343002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:158: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
On 2016/07/08 17:34:59, Evan Stade wrote:
> you don't need to fill this in?

Someone had forgotten to run extract_actions.py. It's been fixed in the
meantime.

[Evan Stade, 2016-07-08 18:39:20]

lgtm

[rkaplow, 2016-07-08 19:05:47]

lgtm

[Mathieu, 2016-07-08 19:06:23]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-07-08 19:06:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, atwilson@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/2124343002/#ps120001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-07-08 19:06:44]

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-08 19:54:23]

Description was changed from

==========
[Autofill] Implement Credit Card Signin Promo

A follow-up change will split the slightly long promo over 2-lines for Views,
Mac, Android.

Also, Android and iOS will need bridges into Java and ObjC, so this is deferred
to another change.

BUG=626442
==========

to

==========
[Autofill] Implement Credit Card Signin Promo

A follow-up change will split the slightly long promo over 2-lines for Views,
Mac, Android.

Also, Android and iOS will need bridges into Java and ObjC, so this is deferred
to another change.

BUG=626442
==========

[commit-bot: I haz the power, 2016-07-08 19:54:24]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-07-08 19:54:42]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 19:56:22]

Description was changed from

==========
[Autofill] Implement Credit Card Signin Promo

A follow-up change will split the slightly long promo over 2-lines for Views,
Mac, Android.

Also, Android and iOS will need bridges into Java and ObjC, so this is deferred
to another change.

BUG=626442
==========

to

==========
[Autofill] Implement Credit Card Signin Promo

A follow-up change will split the slightly long promo over 2-lines for Views,
Mac, Android.

Also, Android and iOS will need bridges into Java and ObjC, so this is deferred
to another change.

BUG=626442

Committed: https://crrev.com/4d08afd67d6c706fa47d5633187687e690b2fed0
Cr-Commit-Position: refs/heads/master@{#404470}
==========

[commit-bot: I haz the power, 2016-07-08 19:56:24]

Patchset 3 (id:??) landed as
https://crrev.com/4d08afd67d6c706fa47d5633187687e690b2fed0
Cr-Commit-Position: refs/heads/master@{#404470}