[Payments] Upload card UI now has a CVC prompt

When credit card upload to Google Payments is blocked due
to missing CVC in checkout flow, offer upload by requesting CVC
from user.

BUG=707104
Review-Url: https://codereview.chromium.org/2789843004
Cr-Commit-Position: refs/heads/master@{#464603}
Committed: https://chromium.googlesource.com/chromium/src/+/314052191919e73f2ed0c7b8014c842103d5688c

[Jared Saul, 2017-03-31 20:51:32]

jsaul@google.com changed reviewers:
+ csashi@google.com, jiahuiguo@google.com, mathp@chromium.org*,
szhangcs@google.com

[Jared Saul, 2017-03-31 20:51:33]

First draft of asking for CVC in the Upstream bubble, PTAL.  Unit tests to
follow (will be in this same CL).

[csashi, 2017-03-31 21:48:15]

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.cc:2402: "Enable requesting missing data during
Autofill credit card upload";
s/data/CVC

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.h:2607: // Name for the flag to enable
requesting CVC in the credit card upload offer to
"offer to save" (in quotes to make it easier to parse)

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.h:2608: // save bubble if it was not detected
during the checkout flow.
s/if it .../, if it was not detected in the form/?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (left):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/BUILD.gn#...
chrome/browser/ui/BUILD.gn:1433: "autofill/save_card_bubble_controller.h",
Don't you need the new location for the .h file?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/chrome_autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.h:59: const bool
upload_cvc_should_be_requested,
I think you can ditch the const.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.h:59: const bool
upload_cvc_should_be_requested,
s/upload_cvc_should_be_request/request_cvc_if_missing (also reads the same as
your flag).

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:141:
cvc_entered_by_user_ = cvc;
No need to trim here?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:58: const
base::string16 GetUserProvidedCvc() const override;
Don't need const base::string16.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:67: // Used to
check if an entered CVC value is a valid CVC for the current card.
I assume this is a simple check (# digits or some such) - i.e. you won't make a
network call. Do you want to state that - i.e. what does "valid" mean?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:68: bool
InputCvcIsValid(const base::string16& input_text) const override;
IsValidCVC ?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:221:
cvc_input_->set_default_width_in_chars(8);
s/8/kDefaultWidthForCVC?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:228:
l10n_util::GetStringUTF16(IDS_AUTOFILL_SAVE_CARD_PROMPT_ENTER_CVC)));
May be add a helper for creating this new view?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:256:
GetDialogClientView()->UpdateDialogButtons();
For my understanding, can you confirm that UpdateDialogButtons will not be
automatically called when contents change? Asking because you are not using
sender or new_contents, only triggering another callback.

Do we display an error if |new_contents| is not a valid CVC.

https://codereview.chromium.org/2789843004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_client.h:136: // contents of
|legal_message| to the user.  Display an additional CVC field
s/Display an additional field/Displays a CVC field/ ?

https://codereview.chromium.org/2789843004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2789843004/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:1044:
upload_cvc_should_be_requested_ ? AutofillMetrics::UPLOAD_OFFERED_NO_CVC
s/NO_CVC/CVC_MISSING or CVC_REQUESTED ?

https://codereview.chromium.org/2789843004/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:1221: // upload and
sometimes offering local save is a confusing user experience.
You can remove these 2 lines (rationale for these decisions outside the scope of
the code comments?)

https://codereview.chromium.org/2789843004/diff/1/components/autofill_strings...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2789843004/diff/1/components/autofill_strings...
components/autofill_strings.grdp:214: Please verify your CVC
I assume this is intentionally "verify" and not "Please enter your CVC"

[jiahuiguo, 2017-03-31 22:52:19]

Run trybots?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:39: const bool
upload_cvc_should_be_requested,
Don't need const here?

[Shanfeng, 2017-03-31 22:58:45]

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (left):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.h:1: // Copyright 2015
The Chromium Authors. All rights reserved.
Any reason to move this file?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:141:
cvc_entered_by_user_ = cvc;
Shouldn't we check non empty here?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:68: bool
InputCvcIsValid(const base::string16& input_text) const override;
On 2017/03/31 21:48:15, csashi wrote:
> IsValidCVC ?

+1

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:110:
base::string16 cvc_entered_by_user_;
Sync the name with Getter and Setter.

[Jared Saul, 2017-04-01 04:18:02]

Thanks for all of the comments!  Resolved most of them but left a few open for
discussion; PTAL.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.cc:2402: "Enable requesting missing data during
Autofill credit card upload";
On 2017/03/31 21:48:14, csashi wrote:
> s/data/CVC

Done.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.h:2607: // Name for the flag to enable
requesting CVC in the credit card upload offer to
On 2017/03/31 21:48:14, csashi wrote:
> "offer to save" (in quotes to make it easier to parse)

Done.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.h:2608: // save bubble if it was not detected
during the checkout flow.
On 2017/03/31 21:48:14, csashi wrote:
> s/if it .../, if it was not detected in the form/?

Done.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (left):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/BUILD.gn#...
chrome/browser/ui/BUILD.gn:1433: "autofill/save_card_bubble_controller.h",
On 2017/03/31 21:48:14, csashi wrote:
> Don't you need the new location for the .h file?

Not sure.  I thought about putting it but I don't want to add it if it's not
needed due to the move.  I assume the trybots will tell me if I need to.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/chrome_autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.h:59: const bool
upload_cvc_should_be_requested,
On 2017/03/31 21:48:15, csashi wrote:
> s/upload_cvc_should_be_request/request_cvc_if_missing (also reads the same as
> your flag).

I agree that it makes it match the flag better, but code-wise I'm not sure this
should change.  This value is literally "yes you should request CVC" or "no you
should not".  The "if_missing" check happens before this point.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.h:59: const bool
upload_cvc_should_be_requested,
On 2017/03/31 21:48:15, csashi wrote:
> I think you can ditch the const.

Done.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (left):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.h:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2017/03/31 22:58:45, Shanfeng wrote:
> Any reason to move this file?

Yeah, the presubmits don't pass if I leave it where it was (there's a rule for
what can be #included from what.)  Mathieu suggested moving it here.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:141:
cvc_entered_by_user_ = cvc;
On 2017/03/31 21:48:15, csashi wrote:
> No need to trim here?

Probably not because it's required to be digits-only and have a length of 3 or
4, but I'll add it anyway to be safe. 
 Thanks!

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:141:
cvc_entered_by_user_ = cvc;
On 2017/03/31 22:58:45, Shanfeng wrote:
> Shouldn't we check non empty here?

Yep I noticed that too; done.  Thanks!

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:39: const bool
upload_cvc_should_be_requested,
On 2017/03/31 22:52:19, jiahuiguo wrote:
> Don't need const here?

Done.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:58: const
base::string16 GetUserProvidedCvc() const override;
On 2017/03/31 21:48:15, csashi wrote:
> Don't need const base::string16.

Done.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:67: // Used to
check if an entered CVC value is a valid CVC for the current card.
On 2017/03/31 21:48:15, csashi wrote:
> I assume this is a simple check (# digits or some such) - i.e. you won't make
a
> network call. Do you want to state that - i.e. what does "valid" mean?

Done.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:68: bool
InputCvcIsValid(const base::string16& input_text) const override;
On 2017/03/31 22:58:45, Shanfeng wrote:
> On 2017/03/31 21:48:15, csashi wrote:
> > IsValidCVC ?
> 
> +1

InputCvcIsValid() is already a function in card_unmask_prompt_view, which is
where I took this from.  I don't mind changing it as long as we don't mind the
inconsistency; what do you think?

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:110:
base::string16 cvc_entered_by_user_;
On 2017/03/31 22:58:45, Shanfeng wrote:
> Sync the name with Getter and Setter.

Done.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:221:
cvc_input_->set_default_width_in_chars(8);
On 2017/03/31 21:48:15, csashi wrote:
> s/8/kDefaultWidthForCVC?

Sure.  Not 100% sure where this goes so I put it in the .h private section below
the other functions.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:228:
l10n_util::GetStringUTF16(IDS_AUTOFILL_SAVE_CARD_PROMPT_ENTER_CVC)));
On 2017/03/31 21:48:15, csashi wrote:
> May be add a helper for creating this new view?

Done.

https://codereview.chromium.org/2789843004/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:256:
GetDialogClientView()->UpdateDialogButtons();
On 2017/03/31 21:48:15, csashi wrote:
> For my understanding, can you confirm that UpdateDialogButtons will not be
> automatically called when contents change? Asking because you are not using
> sender or new_contents, only triggering another callback.
> 
> Do we display an error if |new_contents| is not a valid CVC.

Not sure what you mean.  Whenever the user changes the value in the CVC
textfield, ContentsChanged is called, which calls UpdateDialogButtons.  This
allows for the [Save] button to be disabled until the user types exactly 3
digits (and if they type more, it re-disables itself).  This is the same as how
card_unmask_prompt_view (Chrome Downstream) works today.

For question 2, we don't display an error, we just don't allow [Save] to be
clicked.

https://codereview.chromium.org/2789843004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_client.h:136: // contents of
|legal_message| to the user.  Display an additional CVC field
On 2017/03/31 21:48:15, csashi wrote:
> s/Display an additional field/Displays a CVC field/ ?

Done.

https://codereview.chromium.org/2789843004/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2789843004/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:1044:
upload_cvc_should_be_requested_ ? AutofillMetrics::UPLOAD_OFFERED_NO_CVC
On 2017/03/31 21:48:15, csashi wrote:
> s/NO_CVC/CVC_MISSING or CVC_REQUESTED ?

Ooh, I really like UPLOAD_OFFERED_CVC_MISSING, but I did it this way to be
consistent with UPLOAD_NOT_OFFERED_NO_CVC.  I can go either way on this one. 
What do you think, better name or consistency?

https://codereview.chromium.org/2789843004/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:1221: // upload and
sometimes offering local save is a confusing user experience.
On 2017/03/31 21:48:15, csashi wrote:
> You can remove these 2 lines (rationale for these decisions outside the scope
of
> the code comments?)

These lines were already here, they just got moved over a little.  I could
reorder them if you want, but I don't want to get rid of them entirely.

https://codereview.chromium.org/2789843004/diff/1/components/autofill_strings...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2789843004/diff/1/components/autofill_strings...
components/autofill_strings.grdp:214: Please verify your CVC
On 2017/03/31 21:48:15, csashi wrote:
> I assume this is intentionally "verify" and not "Please enter your CVC" 

Yeah, this is the mocked string Alex made and showed in the UI review doc.  I
expect this whole string to completely change.

[csashi, 2017-04-02 22:36:15]

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/chrome_autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.h:59: bool
upload_cvc_should_be_requested,
should_cvc_be_requested, or is_cvc_required_from_user may be easier to parse.

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:144:
cvc_entered_by_user_ = trimmed_cvc;
Why not replace these 3 lines with
base::TrimpWhitespace(cvc, base::TRIM_ALL, &cvc_entered_by_user_);

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:34: const int
kBubbleWidth = 395;
You can put the cvc 8 char width constant here.

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:245: return
request_cvc_view;
Not sure what the whitespace convention in Chrome is, but seems easier to read
if you add a line separator between 231-232, 238-239, 242-243.

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:94: const int
kDefaultWidthForCvc = 8;
This can go in the .cc file. Define it in the anonymous namespace.

https://codereview.chromium.org/2789843004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/2789843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:42: // not
detected on the page), to be populated in the final request.
You can remove the "(if it was not detected on the page)" from this comment.
This function is not responsible for the logic on whether CVC was detected on
the page or not.

https://codereview.chromium.org/2789843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:46: //
detected on the page).
Same as above.

https://codereview.chromium.org/2789843004/diff/20001/components/autofill_str...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2789843004/diff/20001/components/autofill_str...
components/autofill_strings.grdp:214: Please verify your CVC
If this is a place holder text, can we change it to "Please enter your CVC" or
add a TODO to fix this? FWIW, the constant you have chosen for this text says
"ENTER_CVC".

[Jared Saul, 2017-04-03 18:51:44]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 18:52:20]

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

[Jared Saul, 2017-04-03 18:53:00]

Added changes; thanks for the comments!  (Also started a dry run because I
forgot to click that button last week...)

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/chrome_autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/chrome_autofill_client.h:59: bool
upload_cvc_should_be_requested,
On 2017/04/02 22:36:15, csashi wrote:
> should_cvc_be_requested, or is_cvc_required_from_user may be easier to parse.

Done.

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:144:
cvc_entered_by_user_ = trimmed_cvc;
On 2017/04/02 22:36:15, csashi wrote:
> Why not replace these 3 lines with
> base::TrimpWhitespace(cvc, base::TRIM_ALL, &cvc_entered_by_user_);

Done, thanks.

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:34: const int
kBubbleWidth = 395;
On 2017/04/02 22:36:15, csashi wrote:
> You can put the cvc 8 char width constant here.

Done.

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:245: return
request_cvc_view;
On 2017/04/02 22:36:15, csashi wrote:
> Not sure what the whitespace convention in Chrome is, but seems easier to read
> if you add a line separator between 231-232, 238-239, 242-243.

SGTM, thanks!

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/2789843004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:94: const int
kDefaultWidthForCvc = 8;
On 2017/04/02 22:36:15, csashi wrote:
> This can go in the .cc file. Define it in the anonymous namespace.

Done.

https://codereview.chromium.org/2789843004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/2789843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:42: // not
detected on the page), to be populated in the final request.
On 2017/04/02 22:36:15, csashi wrote:
> You can remove the "(if it was not detected on the page)" from this comment.
> This function is not responsible for the logic on whether CVC was detected on
> the page or not.

Done.

https://codereview.chromium.org/2789843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:46: //
detected on the page).
On 2017/04/02 22:36:15, csashi wrote:
> Same as above.

Done.

https://codereview.chromium.org/2789843004/diff/20001/components/autofill_str...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2789843004/diff/20001/components/autofill_str...
components/autofill_strings.grdp:214: Please verify your CVC
On 2017/04/02 22:36:15, csashi wrote:
> If this is a place holder text, can we change it to "Please enter your CVC" or
> add a TODO to fix this? FWIW, the constant you have chosen for this text says
> "ENTER_CVC".

Added a TODO.  I don't want to change the actual text since this is what's in
Alex's mocks right now, and ENTER_CVC is exactly what's happening (VERIFY_CVC
would sound odd, and it doesn't look like the constant names typically exactly
match their text anyway).

[commit-bot: I haz the power, 2017-04-03 18:56:26]

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

[commit-bot: I haz the power, 2017-04-03 18:56:28]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[csashi, 2017-04-03 19:07:08]

lgtm

[Mathieu, 2017-04-03 19:22:27]

Description was changed from

==========
When credit card upload to Google Payments is blocked due to missing CVC in
checkout flow, offer upload by requesting CVC from user

When credit card upload to Google Payments is blocked due to missing CVC in
checkout flow, offer upload by requesting CVC from user

BUG=707104
==========

to

==========
When credit card upload to Google Payments is blocked due
to missing CVC in checkout flow, offer upload by requesting CVC
from user.

BUG=707104
==========

[Mathieu, 2017-04-03 19:46:08]

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

[Mathieu, 2017-04-03 19:46:10]

Hi Jared, this is very solid code! I only have nits, basically.

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2343: #if defined(OS_CHROMEOS) ||
defined(OS_LINUX) || defined(OS_MACOSX) || \
I don't think you need the ifdef, just kOsDesktop is fine.

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2345:
{"enable-autofill-credit-card-upload-request-cvc-if-missing",
how about enable-autofill-credit-card-upload-cvc-prompt

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:129: int
SaveCardBubbleControllerImpl::GetCvcImageRid() const {
Rid -> ResourceId?

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:102:
controller_->SetCvcEnteredByUser(cvc);
controller_->SetCvcEnteredByUser(cvc_input_->text()); ?

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:231:
std::unique_ptr<View> request_cvc_view(new View());
 std::unique_ptr<View> request_cvc_view = base::MakeUnique<views::View>();

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232:
request_cvc_view->SetLayoutManager(new views::BoxLayout(
please use the same pattern for BoxLayout... MakeUnique then .release()

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:243:
views::ImageView* cvc_image = new views::ImageView();
std::unique_ptr, MakeUnique and .release()

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:258: return
!controller_->ShouldRequestCvcFromUser() ||
return !cvc_input_ || controller_->InputCvcIsValid(cvc_input_->text())); ?

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:88: views::Textfield*
cvc_input_;
prefer cvc_textfield_ so that it's clear what the object is

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

https://codereview.chromium.org/2789843004/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1042:
AutofillMetrics::CardUploadDecisionMetric cardUploadDecisionMetric =
variable_casing

https://codereview.chromium.org/2789843004/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1109: upload_request_.cvc =
DCHECK(client_->GetSaveCardBubbleController()) ?

https://codereview.chromium.org/2789843004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/2789843004/diff/40001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:34: // Returns
the CVC image icon.
... resource ID.

https://codereview.chromium.org/2789843004/diff/40001/components/autofill_str...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2789843004/diff/40001/components/autofill_str...
components/autofill_strings.grdp:213: <!-- TODO(jsaul): Get the finalized
message from UX! -->
I would remove TODO (you can track yourself, it's not very useful to have it
here).

[Jared Saul, 2017-04-03 19:46:39]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 19:47:28]

Dry run: 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, 2017-04-03 19:58:56]

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

[commit-bot: I haz the power, 2017-04-03 19:58:57]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Mathieu, 2017-04-03 20:16:55]

BTW I edited your subject and description so that it conforms with the style

[Shanfeng, 2017-04-03 21:02:43]

lgtm

[Shanfeng, 2017-04-03 21:02:46]

lgtm

[Jared Saul, 2017-04-03 21:19:18]

Thanks for the insights, Mathieu!  PTAL to make sure I resolved your comments
correctly.

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2343: #if defined(OS_CHROMEOS) ||
defined(OS_LINUX) || defined(OS_MACOSX) || \
On 2017/04/03 19:46:09, Mathieu wrote:
> I don't think you need the ifdef, just kOsDesktop is fine.

Done.

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2345:
{"enable-autofill-credit-card-upload-request-cvc-if-missing",
On 2017/04/03 19:46:09, Mathieu wrote:
> how about enable-autofill-credit-card-upload-cvc-prompt

Done.

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:129: int
SaveCardBubbleControllerImpl::GetCvcImageRid() const {
On 2017/04/03 19:46:09, Mathieu wrote:
> Rid -> ResourceId?

Done.

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:102:
controller_->SetCvcEnteredByUser(cvc);
On 2017/04/03 19:46:09, Mathieu wrote:
> controller_->SetCvcEnteredByUser(cvc_input_->text()); ?

Done.

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:231:
std::unique_ptr<View> request_cvc_view(new View());
On 2017/04/03 19:46:09, Mathieu wrote:
>  std::unique_ptr<View> request_cvc_view = base::MakeUnique<views::View>();

Done.

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232:
request_cvc_view->SetLayoutManager(new views::BoxLayout(
On 2017/04/03 19:46:09, Mathieu wrote:
> please use the same pattern for BoxLayout... MakeUnique then .release()

How's this?

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:243:
views::ImageView* cvc_image = new views::ImageView();
On 2017/04/03 19:46:09, Mathieu wrote:
> std::unique_ptr, MakeUnique and .release()

Done.

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:258: return
!controller_->ShouldRequestCvcFromUser() ||
On 2017/04/03 19:46:09, Mathieu wrote:
> return !cvc_input_ || controller_->InputCvcIsValid(cvc_input_->text())); ?

Done.

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/2789843004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:88: views::Textfield*
cvc_input_;
On 2017/04/03 19:46:09, Mathieu wrote:
> prefer cvc_textfield_ so that it's clear what the object is

SGTM, done.

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

https://codereview.chromium.org/2789843004/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1042:
AutofillMetrics::CardUploadDecisionMetric cardUploadDecisionMetric =
On 2017/04/03 19:46:09, Mathieu wrote:
> variable_casing

Done, good catch.

https://codereview.chromium.org/2789843004/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1109: upload_request_.cvc =
On 2017/04/03 19:46:09, Mathieu wrote:
> DCHECK(client_->GetSaveCardBubbleController()) ?

Done.

https://codereview.chromium.org/2789843004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/2789843004/diff/40001/components/autofill/cor...
components/autofill/core/browser/ui/save_card_bubble_controller.h:34: // Returns
the CVC image icon.
On 2017/04/03 19:46:09, Mathieu wrote:
> ... resource ID.

Done.

https://codereview.chromium.org/2789843004/diff/40001/components/autofill_str...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2789843004/diff/40001/components/autofill_str...
components/autofill_strings.grdp:213: <!-- TODO(jsaul): Get the finalized
message from UX! -->
On 2017/04/03 19:46:09, Mathieu wrote:
> I would remove TODO (you can track yourself, it's not very useful to have it
> here).

Ok, removed then

[Jared Saul, 2017-04-04 21:57:53]

Added unit tests for the AutofillManager metrics, which involved updating
TestAutofillClient and creating MockSaveCardBubbleController.  PTAL, thanks!

[Jared Saul, 2017-04-04 21:58:03]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 21:59:04]

Dry run: 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, 2017-04-04 22:10:40]

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

[commit-bot: I haz the power, 2017-04-04 22:10:41]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Mathieu, 2017-04-04 23:49:20]

lgtm! Excited to see this.

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager_unittest.cc:1034:
{autofill::kAutofillUpstreamRequestCvcIfMissing, kAutofillUkmLogging},
nit: no autofill:: ?

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager_unittest.cc:4898:
EXPECT_CALL(autofill_client_, ConfirmSaveCreditCardLocally(_, _)).Times(0);
should we expect that Confirm...ToCloud is called?

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/test_autofill_client.cc (right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.cc:6: #include
<components/autofill/core/browser/ui/mock_save_card_bubble_controller.h>
include is wrong style

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/test_autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.h:17: #include
"components/autofill/core/browser/ui/save_card_bubble_controller.h"
could you see if it compiles without this? Usually the interface being
implemented provides this

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/ui/mock_save_card_bubble_controller.h
(right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/ui/mock_save_card_bubble_controller.h:8:
#include "components/autofill/core/browser/credit_card.h"
could you fwd declare CreditCard or is this needed because of the MOCK macro?

[jiahuiguo, 2017-04-05 00:47:06]

lgtm

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager_unittest.cc:4878:
test::CreateTestFormField("Card Name", "cardname", "", "text", &field);
Inline comment indicating which field is this? e.g. /* cvc */

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/ui/save_card_bubble_controller.h:67:
DISALLOW_COPY_AND_ASSIGN(SaveCardBubbleController);
Put this in private?

[Jared Saul, 2017-04-05 00:58:30]

Thanks!  Made some fixes and replies.

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager_unittest.cc:1034:
{autofill::kAutofillUpstreamRequestCvcIfMissing, kAutofillUkmLogging},
On 2017/04/04 23:49:19, Mathieu wrote:
> nit: no autofill:: ?

Done.

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager_unittest.cc:4878:
test::CreateTestFormField("Card Name", "cardname", "", "text", &field);
On 2017/04/05 00:47:06, jiahuiguo wrote:
> Inline comment indicating which field is this? e.g. /* cvc */

Discussed offline; this is part of the CreateTestFormField call and doesn't
reflect CVC.

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_manager_unittest.cc:4898:
EXPECT_CALL(autofill_client_, ConfirmSaveCreditCardLocally(_, _)).Times(0);
On 2017/04/04 23:49:19, Mathieu wrote:
> should we expect that Confirm...ToCloud is called?

I also wondered why there are no tests for that in this entire file.  When I try
to mock it, I get this error:

error: call to deleted constructor of 'std::unique_ptr<base::DictionaryValue,
std::default_delete<base::DictionaryValue> >'

The only example I could find of someone trying to mock a std::unique_ptr (not a
pointer to it or by reference) is here, and looks like they didn't have any
luck:
https://cs.chromium.org/chromium/src/third_party/webrtc/test/mock_voe_channel...

Ideas?

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/test_autofill_client.cc (right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.cc:6: #include
<components/autofill/core/browser/ui/mock_save_card_bubble_controller.h>
On 2017/04/04 23:49:20, Mathieu wrote:
> include is wrong style

Wow, where'd I pull this from, haha.  Nice catch.

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/test_autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.h:17: #include
"components/autofill/core/browser/ui/save_card_bubble_controller.h"
On 2017/04/04 23:49:20, Mathieu wrote:
> could you see if it compiles without this? Usually the interface being
> implemented provides this

Done; appears to be ok.

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/ui/mock_save_card_bubble_controller.h
(right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/ui/mock_save_card_bubble_controller.h:8:
#include "components/autofill/core/browser/credit_card.h"
On 2017/04/04 23:49:20, Mathieu wrote:
> could you fwd declare CreditCard or is this needed because of the MOCK macro?

I tried forward declaring it, but the compiler's not happy with it, throwing a
bunch of mock errors and telling me it was already forward-declared in
save_card_bubble_controller.h anyway.

Interestingly, it doesn't work without mocks either.  If I just stub out the
method myself, I get:

error: incomplete result type 'const autofill::CreditCard' in function
definition

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
File components/autofill/core/browser/ui/save_card_bubble_controller.h (right):

https://codereview.chromium.org/2789843004/diff/100001/components/autofill/co...
components/autofill/core/browser/ui/save_card_bubble_controller.h:67:
DISALLOW_COPY_AND_ASSIGN(SaveCardBubbleController);
On 2017/04/05 00:47:06, jiahuiguo wrote:
> Put this in private?

Hmm, weird that it's not already there.  I didn't modify this code, but I'll
make the change since I see everything else puts this in the private section.

[Jared Saul, 2017-04-05 00:58:48]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 00:59:43]

Dry run: 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, 2017-04-05 01:10:07]

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

[commit-bot: I haz the power, 2017-04-05 01:10:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Jared Saul, 2017-04-05 16:46:58]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 16:47:21]

Dry run: 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, 2017-04-05 17:07:51]

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

[commit-bot: I haz the power, 2017-04-05 17:07:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jared Saul, 2017-04-05 17:18:19]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 17:19:04]

Dry run: 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, 2017-04-05 17:43:54]

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

[commit-bot: I haz the power, 2017-04-05 17:43:55]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Jared Saul, 2017-04-05 19:30:24]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 19:31:00]

Dry run: 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, 2017-04-05 19:54:02]

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

[commit-bot: I haz the power, 2017-04-05 19:54:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Mathieu, 2017-04-05 20:47:04]

mathp@chromium.org changed reviewers:
- mathp@chromium.org

[Jared Saul, 2017-04-05 21:53:33]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 21:54:36]

Dry run: 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, 2017-04-05 22:20:44]

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

[commit-bot: I haz the power, 2017-04-05 22:20:45]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Jared Saul, 2017-04-05 23:02:16]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 23:03:45]

Dry run: 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, 2017-04-05 23:38:48]

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

[commit-bot: I haz the power, 2017-04-05 23:38:49]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jared Saul, 2017-04-06 21:32:03]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 21:34:20]

Dry run: 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, 2017-04-06 22:01:53]

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

[commit-bot: I haz the power, 2017-04-06 22:01:54]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Jared Saul, 2017-04-06 22:16:34]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 22:17:50]

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

[Jared Saul, 2017-04-06 23:06:57]

jsaul@google.com changed reviewers:
+ estade@chromium.org, groby@chromium.org, isherman@chromium.org,
jdonnelly@chromium.org, mathp@chromium.org, sgurun@chromium.org
- mathp@chromium.org*

[Jared Saul, 2017-04-06 23:06:57]

jsaul@google.com changed required reviewers:
+ estade@chromium.org, groby@chromium.org, isherman@chromium.org,
jdonnelly@chromium.org, mathp@chromium.org, sgurun@chromium.org

[Jared Saul, 2017-04-06 23:06:58]

Adding additional reviewers for the following areas, PTAL (the changes are very
small for everyone but estade@):

sgurun@ - android_webview/native/...
groby@ - chrome/browser/ui/cocoa/autofill/...
estade@ - chrome/browser/ui/views/autofill/...
jdonnelly@ - ios/chrome/browser/ui/autofill/...
isherman@ - tools/metrics/histograms/histograms.xml (rkaplow@ is OOO)

Thank you very much!

[sgurun-gerrit only, 2017-04-06 23:13:30]

On 2017/04/06 23:06:58, Jared Saul wrote:
> Adding additional reviewers for the following areas, PTAL (the changes are
very
> small for everyone but estade@):
> 
> sgurun@ - android_webview/native/...
> groby@ - chrome/browser/ui/cocoa/autofill/...
> estade@ - chrome/browser/ui/views/autofill/...
> jdonnelly@ - ios/chrome/browser/ui/autofill/...
> isherman@ - tools/metrics/histograms/histograms.xml (rkaplow@ is OOO)
> 
> Thank you very much!

aw lgtm

[Ilya Sherman, 2017-04-06 23:55:38]

histograms.xml lgtm

[commit-bot: I haz the power, 2017-04-07 00:55:14]

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

[commit-bot: I haz the power, 2017-04-07 00:55:16]

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

[Evan Stade, 2017-04-07 02:14:40]

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:67: const bool
should_cvc_be_requested,
POD arguments needn't be marked const

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:141:
DCHECK(!cvc.empty());
why not dcheck validity?

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:80:
bool() /* should_cvc_be_requested */,
This should be false, rather than bool().

Could you add test coverage for the request CVC case?

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:33: // Fixed width of
the bubble.
[1/2] this is in dip

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:35: // Fixed width of
the CVC TextField.
nit: TextField has odd capitalization

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:36: const int
kDefaultWidthForCvc = 8;
[2/2] but this is in characters. This distinction should be documented. Although
I think it's even better to share this constant with the 8 in
card_unmask_prompt_view, or heck share the whole textfield init stanza.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:100: if
(controller_->ShouldRequestCvcFromUser()) {
nit: no curlies

also I would slightly prefer this to check the null-ness of cvc_textfield_
instead of asking the controller again.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:213: if
(controller_->ShouldRequestCvcFromUser()) {
nit: no curlies

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:237: cvc_textfield_ =
new views::Textfield();
aside: I wish we had a way to set the limit on number of characters input

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:244:
std::unique_ptr<views::ImageView> cvc_image =
nit: we don't typically use unique_ptr for views like this

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:262:
controller_->InputCvcIsValid(cvc_textfield_->text());
for future consideration: it would be nice if there were some indication the
input cvc were invalid when users can't figure out why the ok button is disabled

[Jared Saul, 2017-04-08 01:05:36]

Thanks for the comments, Evan.  I asked a follow-up question in some of them;
PTAL.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:67: const bool
should_cvc_be_requested,
On 2017/04/07 02:14:39, Evan Stade wrote:
> POD arguments needn't be marked const

Done.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:141:
DCHECK(!cvc.empty());
On 2017/04/07 02:14:39, Evan Stade wrote:
> why not dcheck validity?

Good idea, thanks.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:80:
bool() /* should_cvc_be_requested */,
On 2017/04/07 02:14:39, Evan Stade wrote:
> This should be false, rather than bool().
> 
> Could you add test coverage for the request CVC case?

Added tests for ShouldRequestCvcFromUser()'s value and the metrics happy path
case.  Question: Should I reproduce all of the metrics tests?  If so, is there
an easy to way to true/false parameterize tests in Chrome so I don't need to
duplicate the code itself?

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:33: // Fixed width of
the bubble.
On 2017/04/07 02:14:39, Evan Stade wrote:
> [1/2] this is in dip

Acknowledged.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:35: // Fixed width of
the CVC TextField.
On 2017/04/07 02:14:39, Evan Stade wrote:
> nit: TextField has odd capitalization

Whoops you're right, it's "Textfield".  Thanks.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:36: const int
kDefaultWidthForCvc = 8;
On 2017/04/07 02:14:39, Evan Stade wrote:
> [2/2] but this is in characters. This distinction should be documented.
Although
> I think it's even better to share this constant with the 8 in
> card_unmask_prompt_view, or heck share the whole textfield init stanza.

I'm all for sharing the entire textfield init stanza.  Where do you think it
should be placed?  (In the meantime, documented the distinction.)

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:100: if
(controller_->ShouldRequestCvcFromUser()) {
On 2017/04/07 02:14:39, Evan Stade wrote:
> nit: no curlies
> 
> also I would slightly prefer this to check the null-ness of cvc_textfield_
> instead of asking the controller again.

Done and done.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:213: if
(controller_->ShouldRequestCvcFromUser()) {
On 2017/04/07 02:14:39, Evan Stade wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:237: cvc_textfield_ =
new views::Textfield();
On 2017/04/07 02:14:39, Evan Stade wrote:
> aside: I wish we had a way to set the limit on number of characters input

Acknowledged (agreed).

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:244:
std::unique_ptr<views::ImageView> cvc_image =
On 2017/04/07 02:14:39, Evan Stade wrote:
> nit: we don't typically use unique_ptr for views like this

Done, changed to match card_unmask_prompt_views.cc.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:262:
controller_->InputCvcIsValid(cvc_textfield_->text());
On 2017/04/07 02:14:39, Evan Stade wrote:
> for future consideration: it would be nice if there were some indication the
> input cvc were invalid when users can't figure out why the ok button is
disabled

csashi@ said the same thing, both for this and for card_unmask_prompt_view. 
It's something we should definitely consider for an upcoming improvement.  Just
spitballing ideas here--any idea if there's precedent for disabled buttons to
have tooltip text explaining why they're disabled?

[Jared Saul, 2017-04-08 01:05:53]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-08 01:06:18]

Dry run: 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, 2017-04-08 02:21:05]

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

[commit-bot: I haz the power, 2017-04-08 02:21:06]

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

[Justin Donnelly, 2017-04-10 03:02:50]

ios/chrome/browser/ui/autofill lgtm

[Evan Stade, 2017-04-10 18:32:09]

have to run, will be back to finish review soon

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:80:
bool() /* should_cvc_be_requested */,
On 2017/04/08 01:05:36, Jared Saul wrote:
> On 2017/04/07 02:14:39, Evan Stade wrote:
> > This should be false, rather than bool().
> > 
> > Could you add test coverage for the request CVC case?
> 
> Added tests for ShouldRequestCvcFromUser()'s value and the metrics happy path
> case.  Question: Should I reproduce all of the metrics tests?  If so, is there
> an easy to way to true/false parameterize tests in Chrome so I don't need to
> duplicate the code itself?

you can use TEST_P for parameterization but what you have is fine.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:36: const int
kDefaultWidthForCvc = 8;
On 2017/04/08 01:05:36, Jared Saul wrote:
> On 2017/04/07 02:14:39, Evan Stade wrote:
> > [2/2] but this is in characters. This distinction should be documented.
> Although
> > I think it's even better to share this constant with the 8 in
> > card_unmask_prompt_view, or heck share the whole textfield init stanza.
> 
> I'm all for sharing the entire textfield init stanza.  Where do you think it
> should be placed?  (In the meantime, documented the distinction.)

probably a static fn in chrome/browser/ui/views/autofill/view_util.h

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:262:
controller_->InputCvcIsValid(cvc_textfield_->text());
On 2017/04/08 01:05:36, Jared Saul wrote:
> On 2017/04/07 02:14:39, Evan Stade wrote:
> > for future consideration: it would be nice if there were some indication the
> > input cvc were invalid when users can't figure out why the ok button is
> disabled
> 
> csashi@ said the same thing, both for this and for card_unmask_prompt_view. 
> It's something we should definitely consider for an upcoming improvement. 
Just
> spitballing ideas here--any idea if there's precedent for disabled buttons to
> have tooltip text explaining why they're disabled?

https://cs.chromium.org/chromium/src/chrome/browser/ui/views/autofill/card_un...

https://codereview.chromium.org/2789843004/diff/300001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2789843004/diff/300001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:73: bool
should_cvc_be_requested) {
you can give this a default value

[groby-ooo-7-16, 2017-04-10 20:41:10]

c/b/ui/cocoa RS LGTM

https://codereview.chromium.org/2789843004/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm (right):

https://codereview.chromium.org/2789843004/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:23: class
TestSaveCardBubbleController : public SaveCardBubbleController {
fwiw: It looks like this is shareable code, not cocoa-specific.

I'm also fairly certain you'd be better off w/ GMOCK  by now :)

[Evan Stade, 2017-04-10 21:53:17]

https://codereview.chromium.org/2789843004/diff/300001/components/autofill/co...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/300001/components/autofill/co...
components/autofill/core/browser/autofill_client.h:22: #include
"components/autofill/core/browser/ui/save_card_bubble_controller.h"
I suspect you can get by with a forward declaration.

https://codereview.chromium.org/2789843004/diff/300001/components/autofill/co...
components/autofill/core/browser/autofill_client.h:120: #if !defined(OS_ANDROID)
it would be great if you could avoid these ifdefs as much as possible (in
headers, interfaces, etc.) by using forward declarations and just returning null
for Android.

[Jared Saul, 2017-04-11 00:51:54]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 00:52:36]

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

[Jared Saul, 2017-04-11 00:53:06]

Thanks!  Responded to all comments except for the two newest in
autofill_client.h, will investigate those next.

(Also working on getting ASAN running to debug the ASAN trybot failure.)

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:80:
bool() /* should_cvc_be_requested */,
On 2017/04/10 18:32:09, Evan Stade wrote:
> On 2017/04/08 01:05:36, Jared Saul wrote:
> > On 2017/04/07 02:14:39, Evan Stade wrote:
> > > This should be false, rather than bool().
> > > 
> > > Could you add test coverage for the request CVC case?
> > 
> > Added tests for ShouldRequestCvcFromUser()'s value and the metrics happy
path
> > case.  Question: Should I reproduce all of the metrics tests?  If so, is
there
> > an easy to way to true/false parameterize tests in Chrome so I don't need to
> > duplicate the code itself?
> 
> you can use TEST_P for parameterization but what you have is fine.

Acknowledged.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:36: const int
kDefaultWidthForCvc = 8;
On 2017/04/10 18:32:09, Evan Stade wrote:
> On 2017/04/08 01:05:36, Jared Saul wrote:
> > On 2017/04/07 02:14:39, Evan Stade wrote:
> > > [2/2] but this is in characters. This distinction should be documented.
> > Although
> > > I think it's even better to share this constant with the 8 in
> > > card_unmask_prompt_view, or heck share the whole textfield init stanza.
> > 
> > I'm all for sharing the entire textfield init stanza.  Where do you think it
> > should be placed?  (In the meantime, documented the distinction.)
> 
> probably a static fn in chrome/browser/ui/views/autofill/view_util.h

Done; created view_util.h, PTAL.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:262:
controller_->InputCvcIsValid(cvc_textfield_->text());
On 2017/04/10 18:32:09, Evan Stade wrote:
> On 2017/04/08 01:05:36, Jared Saul wrote:
> > On 2017/04/07 02:14:39, Evan Stade wrote:
> > > for future consideration: it would be nice if there were some indication
the
> > > input cvc were invalid when users can't figure out why the ok button is
> > disabled
> > 
> > csashi@ said the same thing, both for this and for card_unmask_prompt_view. 
> > It's something we should definitely consider for an upcoming improvement. 
> Just
> > spitballing ideas here--any idea if there's precedent for disabled buttons
to
> > have tooltip text explaining why they're disabled?
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/autofill/card_un...

Acknowledged.  Though for the record, that happens *after* the CVC is entered
and the button is clicked (and auth fails).  Looks like that view still doesn't
have anything beforehand that implies CVC is what's preventing the button from
being clicked, hmm.

https://codereview.chromium.org/2789843004/diff/300001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2789843004/diff/300001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:73: bool
should_cvc_be_requested) {
On 2017/04/10 18:32:09, Evan Stade wrote:
> you can give this a default value

Done.

https://codereview.chromium.org/2789843004/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm (right):

https://codereview.chromium.org/2789843004/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:23: class
TestSaveCardBubbleController : public SaveCardBubbleController {
On 2017/04/10 20:41:10, groby wrote:
> fwiw: It looks like this is shareable code, not cocoa-specific.
> 
> I'm also fairly certain you'd be better off w/ GMOCK  by now :)

Yeah, it doesn't look cocoa-specific to me either.  I'll try to investigate this
as a followup if I get some free cycles, thanks.  In the meantime, converted the
blank methods to use GMOCK.

[commit-bot: I haz the power, 2017-04-11 03:20:21]

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

[commit-bot: I haz the power, 2017-04-11 03:20:23]

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

[Evan Stade, 2017-04-11 15:28:05]

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:262:
controller_->InputCvcIsValid(cvc_textfield_->text());
On 2017/04/11 00:53:06, Jared Saul wrote:
> On 2017/04/10 18:32:09, Evan Stade wrote:
> > On 2017/04/08 01:05:36, Jared Saul wrote:
> > > On 2017/04/07 02:14:39, Evan Stade wrote:
> > > > for future consideration: it would be nice if there were some indication
> the
> > > > input cvc were invalid when users can't figure out why the ok button is
> > > disabled
> > > 
> > > csashi@ said the same thing, both for this and for
card_unmask_prompt_view. 
> > > It's something we should definitely consider for an upcoming improvement. 
> > Just
> > > spitballing ideas here--any idea if there's precedent for disabled buttons
> to
> > > have tooltip text explaining why they're disabled?
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/autofill/card_un...
> 
> Acknowledged.  Though for the record, that happens *after* the CVC is entered
> and the button is clicked (and auth fails).  Looks like that view still
doesn't
> have anything beforehand that implies CVC is what's preventing the button from
> being clicked, hmm.

yes. Since this CVC validity check is local and not done on the server you can
do the check whenever you feel is appropriate --- like when CVC loses focus, or
when the user presses "enter" or hovers the mouse over the button or something.

https://codereview.chromium.org/2789843004/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/view_util.h (right):

https://codereview.chromium.org/2789843004/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/view_util.h:15: static const int
kDefaultWidthForCvc = 8;
I would just inline this. The code itself is very self-documenting:

  textfield->set_default_width_in_chars(8);

[Jared Saul, 2017-04-11 22:57:48]

Thanks for the comments!  I believe I resolved them all; all that's currently
left is the ASAN trybot failure.

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:262:
controller_->InputCvcIsValid(cvc_textfield_->text());
On 2017/04/11 15:28:05, Evan Stade wrote:
> On 2017/04/11 00:53:06, Jared Saul wrote:
> > On 2017/04/10 18:32:09, Evan Stade wrote:
> > > On 2017/04/08 01:05:36, Jared Saul wrote:
> > > > On 2017/04/07 02:14:39, Evan Stade wrote:
> > > > > for future consideration: it would be nice if there were some
indication
> > the
> > > > > input cvc were invalid when users can't figure out why the ok button
is
> > > > disabled
> > > > 
> > > > csashi@ said the same thing, both for this and for
> card_unmask_prompt_view. 
> > > > It's something we should definitely consider for an upcoming
improvement. 
> > > Just
> > > > spitballing ideas here--any idea if there's precedent for disabled
buttons
> > to
> > > > have tooltip text explaining why they're disabled?
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/autofill/card_un...
> > 
> > Acknowledged.  Though for the record, that happens *after* the CVC is
entered
> > and the button is clicked (and auth fails).  Looks like that view still
> doesn't
> > have anything beforehand that implies CVC is what's preventing the button
from
> > being clicked, hmm.
> 
> yes. Since this CVC validity check is local and not done on the server you can
> do the check whenever you feel is appropriate --- like when CVC loses focus,
or
> when the user presses "enter" or hovers the mouse over the button or
something.

Acknowledged.  Wrote myself a note to look into this for here and the unmask
view in the future; I like the loses-focus-or-user-presses-enter idea.

https://codereview.chromium.org/2789843004/diff/300001/components/autofill/co...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/300001/components/autofill/co...
components/autofill/core/browser/autofill_client.h:22: #include
"components/autofill/core/browser/ui/save_card_bubble_controller.h"
On 2017/04/10 21:53:16, Evan Stade wrote:
> I suspect you can get by with a forward declaration.

Done.

https://codereview.chromium.org/2789843004/diff/300001/components/autofill/co...
components/autofill/core/browser/autofill_client.h:120: #if !defined(OS_ANDROID)
On 2017/04/10 21:53:16, Evan Stade wrote:
> it would be great if you could avoid these ifdefs as much as possible (in
> headers, interfaces, etc.) by using forward declarations and just returning
null
> for Android.

Done.

https://codereview.chromium.org/2789843004/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/view_util.h (right):

https://codereview.chromium.org/2789843004/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/view_util.h:15: static const int
kDefaultWidthForCvc = 8;
On 2017/04/11 15:28:05, Evan Stade wrote:
> I would just inline this. The code itself is very self-documenting:
> 
>   textfield->set_default_width_in_chars(8);

Done.

[Jared Saul, 2017-04-12 01:51:41]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 01:52:04]

Dry run: 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, 2017-04-12 02:18:39]

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

[commit-bot: I haz the power, 2017-04-12 02:18:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2017-04-12 15:21:31]

just nits, otherwise lg

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:52:
should_cvc_be_requested_ = false;
nit: i'm slightly sketched out that all of these variables are left
uninitialized at construction time. It would be nice to init them and I'd do it
in the header rather than a ctor initializer list.

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right):

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:440: cvc_input_ =
autofill::CreateCvcTextfield();
aren't we already in the autofill:: namespace?

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:101:
controller_->OnSaveButton();
it looks like you only call SetCvcEnteredByUser right before OnSaveButton. Can
you combine the calls? i.e. just add a param to OnSaveButton (which incidentally
I would have named something like OnAccept)

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:229:
base::MakeUnique<views::BoxLayout>(views::BoxLayout::kHorizontal, 0, 0,
nit: making a unique pointer and then immediately (in the same line) releasing
it seems odd to me

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:259: const
base::string16& new_contents) {
nit: DCHECK_EQ(cvc_textfield_, sender);

https://codereview.chromium.org/2789843004/diff/380001/components/autofill/co...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/380001/components/autofill/co...
components/autofill/core/browser/autofill_client.h:137: // contents of
|legal_message| to the user.  Display a CVC field in the bubble
nit: single space after . (there is not consistency throughout the codebase but
there should be consistency within each file)

https://codereview.chromium.org/2789843004/diff/380001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2789843004/diff/380001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1107: #if
!defined(OS_ANDROID)
still necessary?

https://codereview.chromium.org/2789843004/diff/380001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1124: #if
!defined(OS_ANDROID)
still necessary?

[Jared Saul, 2017-04-12 22:49:22]

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:52:
should_cvc_be_requested_ = false;
On 2017/04/12 15:21:31, Evan Stade wrote:
> nit: i'm slightly sketched out that all of these variables are left
> uninitialized at construction time. It would be nice to init them and I'd do
it
> in the header rather than a ctor initializer list.

Done for the three primitives.

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right):

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:440: cvc_input_ =
autofill::CreateCvcTextfield();
On 2017/04/12 15:21:31, Evan Stade wrote:
> aren't we already in the autofill:: namespace?

Yep; removed.

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:101:
controller_->OnSaveButton();
On 2017/04/12 15:21:31, Evan Stade wrote:
> it looks like you only call SetCvcEnteredByUser right before OnSaveButton. Can
> you combine the calls? i.e. just add a param to OnSaveButton (which
incidentally
> I would have named something like OnAccept)

Done.

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:229:
base::MakeUnique<views::BoxLayout>(views::BoxLayout::kHorizontal, 0, 0,
On 2017/04/12 15:21:31, Evan Stade wrote:
> nit: making a unique pointer and then immediately (in the same line) releasing
> it seems odd to me

Fixed.

https://codereview.chromium.org/2789843004/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:259: const
base::string16& new_contents) {
On 2017/04/12 15:21:31, Evan Stade wrote:
> nit: DCHECK_EQ(cvc_textfield_, sender);

Done.

https://codereview.chromium.org/2789843004/diff/380001/components/autofill/co...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/380001/components/autofill/co...
components/autofill/core/browser/autofill_client.h:137: // contents of
|legal_message| to the user.  Display a CVC field in the bubble
On 2017/04/12 15:21:31, Evan Stade wrote:
> nit: single space after . (there is not consistency throughout the codebase
but
> there should be consistency within each file)

Done.

https://codereview.chromium.org/2789843004/diff/380001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2789843004/diff/380001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1107: #if
!defined(OS_ANDROID)
On 2017/04/12 15:21:31, Evan Stade wrote:
> still necessary?

Don't think so.  Removed, thanks.

https://codereview.chromium.org/2789843004/diff/380001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1124: #if
!defined(OS_ANDROID)
On 2017/04/12 15:21:31, Evan Stade wrote:
> still necessary?

Done.

[Jared Saul, 2017-04-12 22:49:33]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 22:50:48]

Dry run: 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, 2017-04-12 23:10:16]

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

[commit-bot: I haz the power, 2017-04-12 23:10:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Jared Saul, 2017-04-12 23:24:18]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 23:25:36]

Dry run: 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, 2017-04-12 23:45:34]

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

[commit-bot: I haz the power, 2017-04-12 23:45:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2017-04-12 23:56:22]

https://codereview.chromium.org/2789843004/diff/420001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/view_util.h (right):

https://codereview.chromium.org/2789843004/diff/420001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/view_util.h:15: static views::Textfield*
CreateCvcTextfield() {
sorry, didn't notice you inlined this in the header. Definition should be in a
cc file.

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
File components/autofill/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/BUILD.gn:127:
"ui/save_card_bubble_controller.h",
I believe this probably shouldn't be included on ios/android

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/BUILD.gn:254:
"ui/mock_save_card_bubble_controller.cc",
ditto

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
File components/autofill/core/browser/test_autofill_client.cc (right):

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.cc:6: #include
"components/autofill/core/browser/ui/mock_save_card_bubble_controller.h"
probably should only include on !android

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.cc:53: #if
defined(OS_ANDROID)
not necessary if you remove the ifdef from the header too

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
File components/autofill/core/browser/test_autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.h:103: #if
!defined(OS_ANDROID)
don't think this is necessary either

[Jared Saul, 2017-04-13 00:42:04]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[Jared Saul, 2017-04-13 00:42:35]

Thanks!  PTAL, not all things were resolved.

https://codereview.chromium.org/2789843004/diff/420001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/view_util.h (right):

https://codereview.chromium.org/2789843004/diff/420001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/view_util.h:15: static views::Textfield*
CreateCvcTextfield() {
On 2017/04/12 23:56:22, Evan Stade wrote:
> sorry, didn't notice you inlined this in the header. Definition should be in a
> cc file.

No problem; done.  PTAL: I wrapped it in a class whereas it wasn't before
because I was getting linker errors.  (Thanks for putting up with my Chromium
newness, haha; I appreciate the push for high code quality.)

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
File components/autofill/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/BUILD.gn:127:
"ui/save_card_bubble_controller.h",
On 2017/04/12 23:56:22, Evan Stade wrote:
> I believe this probably shouldn't be included on ios/android

Done.  Please let me know if the conditional block should be done differently.

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/BUILD.gn:254:
"ui/mock_save_card_bubble_controller.cc",
On 2017/04/12 23:56:22, Evan Stade wrote:
> ditto

Done.

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
File components/autofill/core/browser/test_autofill_client.cc (right):

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.cc:6: #include
"components/autofill/core/browser/ui/mock_save_card_bubble_controller.h"
On 2017/04/12 23:56:22, Evan Stade wrote:
> probably should only include on !android

Done.

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.cc:53: #if
defined(OS_ANDROID)
On 2017/04/12 23:56:22, Evan Stade wrote:
> not necessary if you remove the ifdef from the header too

Acknowledged.

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
File components/autofill/core/browser/test_autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.h:103: #if
!defined(OS_ANDROID)
On 2017/04/12 23:56:22, Evan Stade wrote:
> don't think this is necessary either

Hmm, but when I remove it, this won't build.  It looks like I'm getting errors
because <memory> and std::unique_ptr are trying to figure out how big
SaveCardBubbleController is, but of course it doesn't exist on Android.  Any
thoughts?

[commit-bot: I haz the power, 2017-04-13 00:42:53]

Dry run: 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, 2017-04-13 00:55:41]

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

[commit-bot: I haz the power, 2017-04-13 00:55:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Evan Stade, 2017-04-13 02:21:31]

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
File components/autofill/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/BUILD.gn:127:
"ui/save_card_bubble_controller.h",
On 2017/04/13 00:42:35, Jared Saul wrote:
> On 2017/04/12 23:56:22, Evan Stade wrote:
> > I believe this probably shouldn't be included on ios/android
> 
> Done.  Please let me know if the conditional block should be done differently.

seems ok

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
File components/autofill/core/browser/test_autofill_client.h (right):

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/test_autofill_client.h:103: #if
!defined(OS_ANDROID)
On 2017/04/13 00:42:35, Jared Saul wrote:
> On 2017/04/12 23:56:22, Evan Stade wrote:
> > don't think this is necessary either
> 
> Hmm, but when I remove it, this won't build.  It looks like I'm getting errors
> because <memory> and std::unique_ptr are trying to figure out how big
> SaveCardBubbleController is, but of course it doesn't exist on Android.  Any
> thoughts?

oh, lame. OK, keep it.

https://codereview.chromium.org/2789843004/diff/460001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/view_util.h (right):

https://codereview.chromium.org/2789843004/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/view_util.h:15: static views::Textfield*
CreateCvcTextfield();
what were the linker errors? You shouldn't need the class. Here's a random
example to follow:

https://cs.chromium.org/chromium/src/chrome/browser/win/browser_util.h

[Jared Saul, 2017-04-13 17:04:40]

https://codereview.chromium.org/2789843004/diff/460001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/view_util.h (right):

https://codereview.chromium.org/2789843004/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/view_util.h:15: static views::Textfield*
CreateCvcTextfield();
On 2017/04/13 02:21:31, Evan Stade wrote:
> what were the linker errors? You shouldn't need the class. Here's a random
> example to follow:
> 
> https://cs.chromium.org/chromium/src/chrome/browser/win/browser_util.h

Sorry, I misspoke there--it was a compiler error: "function
'autofill::CreateCvcTextfield' has internal linkage but is not defined". 
Following the example by removing the static modifier fixed the problem, thanks.

[Jared Saul, 2017-04-13 17:04:54]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 17:05:42]

Dry run: 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, 2017-04-13 17:17:35]

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

[commit-bot: I haz the power, 2017-04-13 17:17:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jared Saul, 2017-04-13 17:39:37]

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
File components/autofill/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2789843004/diff/420001/components/autofill/co...
components/autofill/core/browser/BUILD.gn:127:
"ui/save_card_bubble_controller.h",
On 2017/04/13 02:21:31, Evan Stade wrote:
> On 2017/04/13 00:42:35, Jared Saul wrote:
> > On 2017/04/12 23:56:22, Evan Stade wrote:
> > > I believe this probably shouldn't be included on ios/android
> > 
> > Done.  Please let me know if the conditional block should be done
differently.
> 
> seems ok

iOS still uses test_autofill_client, so changed these to !is_android only:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...

[Jared Saul, 2017-04-13 17:39:48]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 17:40:31]

Dry run: 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, 2017-04-13 18:04:00]

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

[commit-bot: I haz the power, 2017-04-13 18:04:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jared Saul, 2017-04-13 18:10:51]

The CQ bit was checked by jsaul@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 18:11:45]

Dry run: 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, 2017-04-13 19:12:52]

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

[commit-bot: I haz the power, 2017-04-13 19:12:53]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-04-13 21:47:04]

lgtm

https://codereview.chromium.org/2789843004/diff/540001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/view_util.cc (right):

https://codereview.chromium.org/2789843004/diff/540001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/view_util.cc:22: }
nit:  // namespace autofill

[Jared Saul, 2017-04-13 21:50:48]

https://codereview.chromium.org/2789843004/diff/540001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/view_util.cc (right):

https://codereview.chromium.org/2789843004/diff/540001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/view_util.cc:22: }
On 2017/04/13 21:47:04, Evan Stade wrote:
> nit:  // namespace autofill

Done, thanks.

[Jared Saul, 2017-04-13 21:51:10]

The CQ bit was checked by jsaul@google.com

[Jared Saul, 2017-04-13 21:51:11]

The patchset sent to the CQ was uploaded after l-g-t-m from csashi@google.com,
szhangcs@google.com, mathp@chromium.org, jiahuiguo@google.com,
sgurun@chromium.org, isherman@chromium.org, groby@chromium.org,
jdonnelly@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2789843004/#ps560001
(title: "Address code review comment")

[commit-bot: I haz the power, 2017-04-13 21:51:52]

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, 2017-04-13 23:21:03]

CQ is committing da patch.

Bot data: {"patchset_id": 560001, "attempt_start_ts": 1492120270826290,
"parent_rev": "7c2bb41a8275988ed20cbf8f5dbaba08a0f9f222", "commit_rev":
"49f63ad05a98b3689b5594fe6718d644cc06f937"}

[commit-bot: I haz the power, 2017-04-13 23:26:23]

CQ is committing da patch.

Bot data: {"patchset_id": 560001, "attempt_start_ts": 1492120270826290,
"parent_rev": "43069309920d7d079ec2b9417c11b64cae1d47d5", "commit_rev":
"314052191919e73f2ed0c7b8014c842103d5688c"}

[commit-bot: I haz the power, 2017-04-13 23:27:20]

Description was changed from

==========
When credit card upload to Google Payments is blocked due
to missing CVC in checkout flow, offer upload by requesting CVC
from user.

BUG=707104
==========

to

==========
When credit card upload to Google Payments is blocked due
to missing CVC in checkout flow, offer upload by requesting CVC
from user.

BUG=707104

Review-Url: https://codereview.chromium.org/2789843004
Cr-Commit-Position: refs/heads/master@{#464603}
Committed:
https://chromium.googlesource.com/chromium/src/+/314052191919e73f2ed0c7b8014c...
==========

[commit-bot: I haz the power, 2017-04-13 23:27:22]

Committed patchset #29 (id:560001) as
https://chromium.googlesource.com/chromium/src/+/314052191919e73f2ed0c7b8014c...