Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(20)

Issue 2883273005: Use ViewStack to convert card upload request CVC experiment to 2-step flow (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by Jared Saul
Modified:
1 month, 4 weeks ago
CC:
chromium-reviews, estade+watch_chromium.org, mathp+autofillwatch_chromium.org, rogerm+autofillwatch_chromium.org, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, tfarina, vabr+watchlistautofill_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use ViewStack to convert card upload request CVC experiment to 2-step flow BUG=707104 Review-Url: https://codereview.chromium.org/2883273005 Cr-Commit-Position: refs/heads/master@{#474386} Committed: https://chromium.googlesource.com/chromium/src/+/335471d2d07caccbd4a89d67d86f777cf85941da

Patch Set 1 #

Total comments: 25

Patch Set 2 : Addressing code review comments #

Total comments: 14

Patch Set 3 : Addressing code review comments #

Total comments: 4

Patch Set 4 : Addressing code review comments #

Patch Set 5 : Addressing code review comments #

Total comments: 16

Patch Set 6 : Addressing code review comments #

Patch Set 7 : Addressing code review comments #

Total comments: 8

Patch Set 8 : Added comment explaining inner view's purpose #

Patch Set 9 : Change cross_axis_alignment of CVC view #

Patch Set 10 : Merge forward #

Messages

Total messages: 43 (14 generated)
Jared Saul
Hi Mathieu and Sashi, PTAL. Here's the initial draft of the ViewStack CL I mentioned; ...
2 months ago (2017-05-17 22:09:10 UTC) #3
Mathieu
+Anthony, you have your first ViewStack user! Can you PTAL to advise? This looks great! ...
2 months ago (2017-05-17 23:53:01 UTC) #8
anthonyvd
Yay, awesome to see this used :) https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode103 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:103: if (controller_->ShouldRequestCvcFromUser() ...
2 months ago (2017-05-18 12:01:01 UTC) #11
Jared Saul
https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode14 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:14: #include "chrome/browser/ui/views/payments/view_stack.h" On 2017/05/17 23:53:01, Mathieu wrote: > no ...
2 months ago (2017-05-18 17:01:25 UTC) #12
anthonyvd
lgtm https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode242 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:242: request_cvc_view->set_background( On 2017/05/18 at 17:01:23, Jared Saul wrote: ...
2 months ago (2017-05-18 17:22:59 UTC) #13
Jared Saul
https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode242 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:242: request_cvc_view->set_background( On 2017/05/18 17:22:59, anthonyvd wrote: > On 2017/05/18 ...
2 months ago (2017-05-18 17:34:34 UTC) #14
csashi
https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode99 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:99: // If upload save and more info is needed, ...
2 months ago (2017-05-18 17:42:25 UTC) #15
Jared Saul
Thanks! https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode99 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:99: // If upload save and more info is ...
2 months ago (2017-05-18 18:03:08 UTC) #16
csashi
https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode100 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:100: if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) { On 2017/05/18 ...
2 months ago (2017-05-18 19:29:48 UTC) #17
Mathieu
https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode102 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:102: DCHECK_LE(view_stack_->size(), static_cast<size_t>(1)); On 2017/05/18 19:29:48, csashi wrote: > I ...
2 months ago (2017-05-18 19:48:11 UTC) #18
Jared Saul
https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode100 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:100: if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) { On 2017/05/18 ...
2 months ago (2017-05-18 19:51:37 UTC) #19
Jared Saul
https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode102 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:102: DCHECK_LE(view_stack_->size(), static_cast<size_t>(1)); On 2017/05/18 19:48:11, Mathieu wrote: > On ...
2 months ago (2017-05-18 19:52:35 UTC) #20
Mathieu
On 2017/05/18 19:52:35, Jared Saul wrote: > https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc > File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): > > https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode102 ...
2 months ago (2017-05-18 20:20:20 UTC) #21
Jared Saul
On 2017/05/18 20:20:20, Mathieu wrote: > On 2017/05/18 19:52:35, Jared Saul wrote: > > > ...
2 months ago (2017-05-18 20:50:17 UTC) #22
csashi
lgtm
2 months ago (2017-05-18 20:53:56 UTC) #23
anthonyvd
https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode242 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:242: request_cvc_view->set_background( On 2017/05/18 at 17:34:34, Jared Saul wrote: > ...
2 months ago (2017-05-18 21:08:42 UTC) #24
Mathieu
lgtm, cool stuff Jared!
2 months ago (2017-05-19 01:15:28 UTC) #25
Jared Saul
Hi estade@, PTAL. Background info: this change takes https://codereview.chromium.org/2789843004 and uses ViewStack to convert it ...
2 months ago (2017-05-19 18:53:48 UTC) #28
Evan Stade
https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode99 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:99: CHECK_LE(view_stack_->size(), static_cast<size_t>(2)); nit: make these DCHECKs https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode99 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:99: CHECK_LE(view_stack_->size(), ...
2 months ago (2017-05-19 19:01:29 UTC) #29
Jared Saul
Thanks for the quick reply! https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode99 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:99: CHECK_LE(view_stack_->size(), static_cast<size_t>(2)); On 2017/05/19 ...
2 months ago (2017-05-19 19:20:55 UTC) #30
Evan Stade
https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode195 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:195: std::unique_ptr<View> view = base::MakeUnique<views::View>(); nit: great place to use ...
2 months ago (2017-05-19 22:18:17 UTC) #31
Jared Saul
https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode195 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:195: std::unique_ptr<View> view = base::MakeUnique<views::View>(); On 2017/05/19 22:18:16, Evan Stade ...
1 month, 4 weeks ago (2017-05-23 18:53:03 UTC) #32
Evan Stade
https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode246 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:246: auto inner_request_cvc_view = base::MakeUnique<views::View>(); can you explain why you ...
1 month, 4 weeks ago (2017-05-23 23:21:09 UTC) #33
Jared Saul
https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode246 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:246: auto inner_request_cvc_view = base::MakeUnique<views::View>(); On 2017/05/23 23:21:08, Evan Stade ...
1 month, 4 weeks ago (2017-05-23 23:57:50 UTC) #34
Evan Stade
https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode246 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:246: auto inner_request_cvc_view = base::MakeUnique<views::View>(); On 2017/05/23 23:57:50, Jared Saul ...
1 month, 4 weeks ago (2017-05-24 00:20:06 UTC) #35
Jared Saul
https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/views/autofill/save_card_bubble_views.cc#newcode246 chrome/browser/ui/views/autofill/save_card_bubble_views.cc:246: auto inner_request_cvc_view = base::MakeUnique<views::View>(); On 2017/05/24 00:20:06, Evan Stade ...
1 month, 4 weeks ago (2017-05-24 00:45:24 UTC) #36
Evan Stade
lgtm
1 month, 4 weeks ago (2017-05-24 01:48:10 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2883273005/180001
1 month, 4 weeks ago (2017-05-24 18:23:56 UTC) #40
commit-bot: I haz the power
1 month, 4 weeks ago (2017-05-24 19:24:46 UTC) #43
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/335471d2d07caccbd4a89d67d86f...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973