Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2883273005:
Use ViewStack to convert card upload request CVC experiment to 2-step flow (Closed)
|
Created:
3 years, 7 months ago by Jared Saul Modified:
3 years, 7 months 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. |
DescriptionUse 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)
jsaul@google.com changed reviewers: + csashi@google.com, mathp@chromium.org
jsaul@google.com changed required reviewers: + mathp@chromium.org
Hi Mathieu and Sashi, PTAL. Here's the initial draft of the ViewStack CL I mentioned; UX development and browser integration tests to come.
The CQ bit was checked by jsaul@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use ViewStack to convert card upload request CVC experiment to 2-step flow BUG=707104 ========== to ========== Use ViewStack to convert card upload request CVC experiment to 2-step flow BUG=707104 ==========
mathp@chromium.org changed reviewers: + anthonyvd@chromium.org
+Anthony, you have your first ViewStack user! Can you PTAL to advise? This looks great! https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:14: #include "chrome/browser/ui/views/payments/view_stack.h" no need https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:59: learn_more_link_(nullptr), feel free to initialize this and cvc_textfield_ = nullptr; in the .h instead https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:234: void SaveCardBubbleViews::CreateAndAddRequestCvcViewToViewStack() { I would have this return std::unique_ptr<views::View> and in Accept you can do view_stack_->Push(CreateRequestCvcView()); https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:235: std::unique_ptr<View> overall_view(new View()); = base::MakeUnique<views::View>(); https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:241: views::View* request_cvc_view = new views::View(); same base::MakeUnique<...>() and .release() later https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:242: request_cvc_view->set_background( need? https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:288: view_stack_->Push(CreateMainContentView(), false); Why not do this in all cases? Would simplify the code. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:288: view_stack_->Push(CreateMainContentView(), false); /*animate=*/false https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.h:92: views::Link* learn_more_link_; curious: any reason for bringing this up ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yay, awesome to see this used :) https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:103: if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) { Can this be reached if IsUploading is false? If so, view_stack_ will be null here. Note that Math's suggestion of always having the view stack exist fixes this. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:287: view_stack_ = base::MakeUnique<ViewStack>(); If you own the view, you must call set_owned_by_client on it (otherwise it'll be deleted twice). It's not immediately clear that you need to own it explicitly (and set_owned_by_client is discouraged). Provided you implement Math's change that always adds the view stack, you could also do AddChildView(new ViewStack) in the ctor, and have a ViewStack* view_stack() { return static_cast<ViewStack*>(child_at(0)); } function to get it back (or even keep a raw pointer to it). Anyways I'm not sure there's a nice solution to this, so I'll let you guys pick whichever you feel is the lesser evil as owners of this code. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:288: view_stack_->Push(CreateMainContentView(), false); On 2017/05/17 at 23:53:01, Mathieu wrote: > Why not do this in all cases? Would simplify the code. Yep, this and what's in the else block should be equivalent visually.
https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... 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 need Done. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:59: learn_more_link_(nullptr), On 2017/05/17 23:53:01, Mathieu wrote: > feel free to initialize this and cvc_textfield_ = nullptr; in the .h instead Done. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:103: if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) { On 2017/05/18 12:01:01, anthonyvd wrote: > Can this be reached if IsUploading is false? If so, view_stack_ will be null > here. Note that Math's suggestion of always having the view stack exist fixes > this. Always having the view stack exist fixes this, thanks. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:234: void SaveCardBubbleViews::CreateAndAddRequestCvcViewToViewStack() { On 2017/05/17 23:53:01, Mathieu wrote: > I would have this return std::unique_ptr<views::View> and in Accept you can do > > view_stack_->Push(CreateRequestCvcView()); Done. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:235: std::unique_ptr<View> overall_view(new View()); On 2017/05/17 23:53:01, Mathieu wrote: > = base::MakeUnique<views::View>(); Done. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:241: views::View* request_cvc_view = new views::View(); On 2017/05/17 23:53:01, Mathieu wrote: > same base::MakeUnique<...>() and .release() later Done. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:242: request_cvc_view->set_background( On 2017/05/17 23:53:01, Mathieu wrote: > need? Seems so--without it, upon switching to ViewStack the views have a cyan background and I get warnings for "Ancestor view has a non-opaque layer". https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:287: view_stack_ = base::MakeUnique<ViewStack>(); On 2017/05/18 12:01:01, anthonyvd wrote: > If you own the view, you must call set_owned_by_client on it (otherwise it'll be > deleted twice). > > It's not immediately clear that you need to own it explicitly (and > set_owned_by_client is discouraged). Provided you implement Math's change that > always adds the view stack, you could also do AddChildView(new ViewStack) in the > ctor, and have a ViewStack* view_stack() { return > static_cast<ViewStack*>(child_at(0)); } function to get it back (or even keep a > raw pointer to it). > > Anyways I'm not sure there's a nice solution to this, so I'll let you guys pick > whichever you feel is the lesser evil as owners of this code. Thanks for the heads-up. I tried the raw pointer approach, but I think the bubble was taking into account the empty ViewStack when initially deciding its size and hiding the whole thing even after adding to it. I opted for set_owned_by_client for now, but Mathieu please let me know if you have a preference. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:288: view_stack_->Push(CreateMainContentView(), false); On 2017/05/17 23:53:01, Mathieu wrote: > Why not do this in all cases? Would simplify the code. Good idea! Done. https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/save_card_bubble_views.h:92: views::Link* learn_more_link_; On 2017/05/17 23:53:01, Mathieu wrote: > curious: any reason for bringing this up ? It was purely so that it wasn't mistaken as part of the "// Upload save state" block. Since view_stack_ is going to be used in both local and upload cases, though, I reverted this.
lgtm https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... 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: > On 2017/05/17 23:53:01, Mathieu wrote: > > need? > > Seems so--without it, upon switching to ViewStack the views have a cyan background and I get warnings for "Ancestor view has a non-opaque layer". Ah yeah, this is probably because the ViewStack paints to a layer (we were using it with Harmony buttons, which paint to a layer themselves and were encountering bugs where clipping didn't happen correctly). In any case setting the background should be fine if it works for you but deciding whether to paint the stack to a layer or not should probably be changed to be the caller's decision.
https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... 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 at 17:01:23, Jared Saul wrote: > > On 2017/05/17 23:53:01, Mathieu wrote: > > > need? > > > > Seems so--without it, upon switching to ViewStack the views have a cyan > background and I get warnings for "Ancestor view has a non-opaque layer". > > Ah yeah, this is probably because the ViewStack paints to a layer (we were using > it with Harmony buttons, which paint to a layer themselves and were encountering > bugs where clipping didn't happen correctly). > > In any case setting the background should be fine if it works for you but > deciding whether to paint the stack to a layer or not should probably be changed > to be the caller's decision. You mean in ViewStack's code? Or do you have a suggested alternative for this?
https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:99: // If upload save and more info is needed, push the next view onto the stack. s/upload save and/user accepted upload but/ Move comment to inside if? https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:100: if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) { DCHECK_LE(view_stack_->size(), 1)? When can we be here if a view_stack_ size != 1? https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:106: return false; Do you need to update the documentation for Accept now? You are returning false in some cases. https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:192: std::unique_ptr<View> view(new View()); MakeUnique? https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: std::unique_ptr<views::View> SaveCardBubbleViews::CreateRequestCvcView() { Do you want to rename this function? We are returning the 'overall' view now. https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:235: views::Background::CreateSolidBackground(SK_ColorWHITE)); Do you need this? Does the stack not inherit from the background from the view on top of the stack?
Thanks! https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:99: // If upload save and more info is needed, push the next view onto the stack. On 2017/05/18 17:42:25, csashi wrote: > s/upload save and/user accepted upload but/ > > Move comment to inside if? Done. https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:100: if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) { On 2017/05/18 17:42:25, csashi wrote: > DCHECK_LE(view_stack_->size(), 1)? > When can we be here if a view_stack_ size != 1? 1) Done. 2) If missing CVC but on the second step of the bubble where CVC is being entered. The button that is clicked doesn't change. https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:106: return false; On 2017/05/18 17:42:25, csashi wrote: > Do you need to update the documentation for Accept now? You are returning false > in some cases. Hmm, I don't think so. The current documentation says "This function should return true if the window can be closed after it returns, or false if it must remain open." which seems sufficient. https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:192: std::unique_ptr<View> view(new View()); On 2017/05/18 17:42:25, csashi wrote: > MakeUnique? Done. https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: std::unique_ptr<views::View> SaveCardBubbleViews::CreateRequestCvcView() { On 2017/05/18 17:42:25, csashi wrote: > Do you want to rename this function? We are returning the 'overall' view now. I feel the name is still applicable as it's doing the same thing, but I had to wrap the horizontal view in a vertical view or it had weird effects. I'll rename the variables to make it clearer. https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:235: views::Background::CreateSolidBackground(SK_ColorWHITE)); On 2017/05/18 17:42:25, csashi wrote: > Do you need this? Does the stack not inherit from the background from the view > on top of the stack? Without it, it doesn't cover up the item below it on the stack and you can see through to the previous layer.
https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:100: if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) { On 2017/05/18 18:03:07, Jared Saul wrote: > On 2017/05/18 17:42:25, csashi wrote: > > DCHECK_LE(view_stack_->size(), 1)? > > When can we be here if a view_stack_ size != 1? > > 1) Done. > 2) If missing CVC but on the second step of the bubble where CVC is being > entered. The button that is clicked doesn't change. For (2). If stack_size() > 1, should we DCHECK(conroller_->ShouldRequestCvcFromUser()); https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:102: DCHECK_LE(view_stack_->size(), static_cast<size_t>(1)); I was recommending the DCHECK before the if. Is the DCHECK not valid there?
https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... 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 was recommending the DCHECK before the if. Is the DCHECK not valid there? BTW consider that DCHECK is only in developer builds, and this flow is very rarely triggered by developers. Would a CHECK be more useful in terms of getting reports from the wild?
https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:100: if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) { On 2017/05/18 19:29:48, csashi wrote: > On 2017/05/18 18:03:07, Jared Saul wrote: > > On 2017/05/18 17:42:25, csashi wrote: > > > DCHECK_LE(view_stack_->size(), 1)? > > > When can we be here if a view_stack_ size != 1? > > > > 1) Done. > > 2) If missing CVC but on the second step of the bubble where CVC is being > > entered. The button that is clicked doesn't change. > > For (2). If stack_size() > 1, should we > DCHECK(conroller_->ShouldRequestCvcFromUser()); Added check for <= 2 stack size at all times, as well as a check for "must be stack size 1 or ShouldRequestCvcFromUser()" https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... 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 was recommending the DCHECK before the if. Is the DCHECK not valid there? Discussed offline; I see what you mean now. Done.
https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... 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 2017/05/18 19:29:48, csashi wrote: > > I was recommending the DCHECK before the if. Is the DCHECK not valid there? > > BTW consider that DCHECK is only in developer builds, and this flow is very > rarely triggered by developers. Would a CHECK be more useful in terms of getting > reports from the wild? Mathieu: What does a CHECK do? I'm adverse to crashing browsers in the midst of checkout flows. :)
On 2017/05/18 19:52:35, Jared Saul wrote: > https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): > > https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... > 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 2017/05/18 19:29:48, csashi wrote: > > > I was recommending the DCHECK before the if. Is the DCHECK not valid there? > > > > BTW consider that DCHECK is only in developer builds, and this flow is very > > rarely triggered by developers. Would a CHECK be more useful in terms of > getting > > reports from the wild? > > Mathieu: What does a CHECK do? I'm adverse to crashing browsers in the midst of > checkout flows. :) It crashes the browser. But then you get a crash report and make the code better :) We would start seeing the crashes on Canary/Dev, which is OK as long as we are keen on fixing the bugs.
On 2017/05/18 20:20:20, Mathieu wrote: > On 2017/05/18 19:52:35, Jared Saul wrote: > > > https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... > > File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): > > > > > https://codereview.chromium.org/2883273005/diff/40001/chrome/browser/ui/views... > > 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 2017/05/18 19:29:48, csashi wrote: > > > > I was recommending the DCHECK before the if. Is the DCHECK not valid > there? > > > > > > BTW consider that DCHECK is only in developer builds, and this flow is very > > > rarely triggered by developers. Would a CHECK be more useful in terms of > > getting > > > reports from the wild? > > > > Mathieu: What does a CHECK do? I'm adverse to crashing browsers in the midst > of > > checkout flows. :) > > It crashes the browser. But then you get a crash report and make the code better > :) We would start seeing the crashes on Canary/Dev, which is OK as long as we > are keen on fixing the bugs. Good point that we'd see the crashes on Canary/Dev before it ever got to Stable; that makes me feel a little better. And we're definitely keen on fixing any bugs that pop up. :) Ok, changed the DCHECKs to CHECKs.
lgtm
https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... 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: > On 2017/05/18 17:22:59, anthonyvd wrote: > > On 2017/05/18 at 17:01:23, Jared Saul wrote: > > > On 2017/05/17 23:53:01, Mathieu wrote: > > > > need? > > > > > > Seems so--without it, upon switching to ViewStack the views have a cyan > > background and I get warnings for "Ancestor view has a non-opaque layer". > > > > Ah yeah, this is probably because the ViewStack paints to a layer (we were using > > it with Harmony buttons, which paint to a layer themselves and were encountering > > bugs where clipping didn't happen correctly). > > > > In any case setting the background should be fine if it works for you but > > deciding whether to paint the stack to a layer or not should probably be changed > > to be the caller's decision. > > You mean in ViewStack's code? Or do you have a suggested alternative for this? Sorry, yes I meant in ViewStack's code :)
lgtm, cool stuff Jared!
jsaul@google.com changed reviewers: + estade@chromium.org
jsaul@google.com changed required reviewers: + estade@chromium.org
Hi estade@, PTAL. Background info: this change takes https://codereview.chromium.org/2789843004 and uses ViewStack to convert it into a 2-step flow within the same bubble. - UX is actively working on mocks so it's not a final design yet - It's still under the same 'enable-autofill-credit-card-upload-cvc-prompt' flag - Browser integration tests are next up while I wait for UX's guidance
https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... 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... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:99: CHECK_LE(view_stack_->size(), static_cast<size_t>(2)); nit: 2U rather than casting https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:100: CHECK(view_stack_->size() == 1 || controller_->ShouldRequestCvcFromUser()); I don't think this check is super self-explanatory, consider adding a comment. https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:238: views::Background::CreateSolidBackground(SK_ColorWHITE)); Why do you need to set this color? Why aren't we getting the correct bg color by default, as per the theme? What platform are you developing on? https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:285: view_stack_->set_owned_by_client(); // Required else it'll be deleted twice. are you sure this is required? Why not make view_stack_ a weak/dumb pointer like the vast majority of views that are members?
Thanks for the quick reply! https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... 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 19:01:28, Evan Stade wrote: > nit: 2U rather than casting Done. https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... 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 19:01:29, Evan Stade wrote: > nit: make these DCHECKs Done. https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:100: CHECK(view_stack_->size() == 1 || controller_->ShouldRequestCvcFromUser()); On 2017/05/19 19:01:29, Evan Stade wrote: > I don't think this check is super self-explanatory, consider adding a comment. Done; how's this? https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:238: views::Background::CreateSolidBackground(SK_ColorWHITE)); On 2017/05/19 19:01:28, Evan Stade wrote: > Why do you need to set this color? Why aren't we getting the correct bg color by > default, as per the theme? What platform are you developing on? It's a limitation of ViewStack, see mathp@/anthonyvd@'s discussion in this CL: https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... Sounds like Anthony has an idea on how to modify ViewStack to choose what it should paint. (Also, Linux.) https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:285: view_stack_->set_owned_by_client(); // Required else it'll be deleted twice. On 2017/05/19 19:01:28, Evan Stade wrote: > are you sure this is required? Why not make view_stack_ a weak/dumb pointer like > the vast majority of views that are members? See comment thread: https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... anthonyvd@ called out that it's required. I tried making it a raw pointer, but the bubble then only accounted for an empty ViewStack when deciding its dimensions and the actual content of the bubble was hidden.
https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... 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 auto https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:238: views::Background::CreateSolidBackground(SK_ColorWHITE)); On 2017/05/19 19:20:55, Jared Saul wrote: > On 2017/05/19 19:01:28, Evan Stade wrote: > > Why do you need to set this color? Why aren't we getting the correct bg color > by > > default, as per the theme? What platform are you developing on? > > It's a limitation of ViewStack, see mathp@/anthonyvd@'s discussion in this CL: > https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... > Sounds like Anthony has an idea on how to modify ViewStack to choose what it > should paint. (Also, Linux.) What you need both here and there is CreateThemedSolidBackground(), probably with kColorId_BubbleBackground as the id, and you should probably call it on the ViewStack. Hardcoding an SkColor value is nearly always not what you want. https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:285: view_stack_->set_owned_by_client(); // Required else it'll be deleted twice. On 2017/05/19 19:20:55, Jared Saul wrote: > On 2017/05/19 19:01:28, Evan Stade wrote: > > are you sure this is required? Why not make view_stack_ a weak/dumb pointer > like > > the vast majority of views that are members? > > See comment thread: > https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... > anthonyvd@ called out that it's required. I tried making it a raw pointer, but > the bubble then only accounted for an empty ViewStack when deciding its > dimensions and the actual content of the bubble was hidden. I don't understand how making this a raw pointer not owned by client would cause that problem. The ownership/destruction sequence is almost exactly the same either way.
https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... 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 wrote: > nit: great place to use auto Done; replaced the other two "std::unique_ptr<View>" as well. https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:238: views::Background::CreateSolidBackground(SK_ColorWHITE)); On 2017/05/19 22:18:17, Evan Stade wrote: > On 2017/05/19 19:20:55, Jared Saul wrote: > > On 2017/05/19 19:01:28, Evan Stade wrote: > > > Why do you need to set this color? Why aren't we getting the correct bg > color > > by > > > default, as per the theme? What platform are you developing on? > > > > It's a limitation of ViewStack, see mathp@/anthonyvd@'s discussion in this CL: > > > https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... > > Sounds like Anthony has an idea on how to modify ViewStack to choose what it > > should paint. (Also, Linux.) > > What you need both here and there is CreateThemedSolidBackground(), probably > with kColorId_BubbleBackground as the id, and you should probably call it on the > ViewStack. Hardcoding an SkColor value is nearly always not what you want. Thanks for the tip; that worked! I also did have to set it on the 2nd view as well, otherwise when it slides in from the right without a background, both views overlap. https://codereview.chromium.org/2883273005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:285: view_stack_->set_owned_by_client(); // Required else it'll be deleted twice. On 2017/05/19 22:18:17, Evan Stade wrote: > On 2017/05/19 19:20:55, Jared Saul wrote: > > On 2017/05/19 19:01:28, Evan Stade wrote: > > > are you sure this is required? Why not make view_stack_ a weak/dumb pointer > > like > > > the vast majority of views that are members? > > > > See comment thread: > > > https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut... > > anthonyvd@ called out that it's required. I tried making it a raw pointer, > but > > the bubble then only accounted for an empty ViewStack when deciding its > > dimensions and the actual content of the bubble was hidden. > > I don't understand how making this a raw pointer not owned by client would cause > that problem. The ownership/destruction sequence is almost exactly the same > either way. Ah, you're right. I believe those problems stemmed from Anthony's suggestion of adding the ViewStack in the ctor instead of Init(). Making this a raw pointer and keeping it here works after all.
https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... 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 need this inner view? It looks like request_cvc_view only has this one child so what is its purpose? https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:255: views::ImageView* cvc_image = new views::ImageView(); this is inconsistent with the smart pointer for inner_request_cvc_view. I prefer not using smart pointers for views like this but one way or another it should be consistent.
https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... 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 wrote: > can you explain why you need this inner view? It looks like request_cvc_view > only has this one child so what is its purpose? Yep, added a comment. The reason is because if only the single child (the horizontal inner_request_cvc_view) is added to the ViewStack, the textfield stretches to fill the entire height of the bubble. Wrapping that horizontally-layed-out view in a containing view keeps the textfield at the normal size of the font. I can provide screenshots if you want them. I expect the general layout of these views to change as UX provides more direction and requirements. https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:255: views::ImageView* cvc_image = new views::ImageView(); On 2017/05/23 23:21:08, Evan Stade wrote: > this is inconsistent with the smart pointer for inner_request_cvc_view. I prefer > not using smart pointers for views like this but one way or another it should be > consistent. +1 for consistency, but can you elaborate on what should change? CreateMainContentView() also has a smart pointer for the overall view, but one views::View*, one views::ImageView*, and one views::Label*. Should these all be smart pointers as well? Alternatively, if request_cvc_view stayed a smart pointer because it's being returned, but inner_request_cvc_view and everything else was a views::View*, would that be ideal instead?
https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... 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 wrote: > On 2017/05/23 23:21:08, Evan Stade wrote: > > can you explain why you need this inner view? It looks like request_cvc_view > > only has this one child so what is its purpose? > > Yep, added a comment. The reason is because if only the single child (the > horizontal inner_request_cvc_view) is added to the ViewStack, the textfield > stretches to fill the entire height of the bubble. Wrapping that > horizontally-layed-out view in a containing view keeps the textfield at the > normal size of the font. I can provide screenshots if you want them. > > I expect the general layout of these views to change as UX provides more > direction and requirements. > It sounds like you just need to use a different BoxLayout::CrossAxisAlignment so the textfield doesn't stretch. https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:255: views::ImageView* cvc_image = new views::ImageView(); On 2017/05/23 23:57:50, Jared Saul wrote: > Alternatively, if request_cvc_view stayed a smart pointer because it's being > returned, but inner_request_cvc_view and everything else was a views::View*, > would that be ideal instead? yes, this is my preference. Thanks.
https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... 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 wrote: > On 2017/05/23 23:57:50, Jared Saul wrote: > > On 2017/05/23 23:21:08, Evan Stade wrote: > > > can you explain why you need this inner view? It looks like request_cvc_view > > > only has this one child so what is its purpose? > > > > Yep, added a comment. The reason is because if only the single child (the > > horizontal inner_request_cvc_view) is added to the ViewStack, the textfield > > stretches to fill the entire height of the bubble. Wrapping that > > horizontally-layed-out view in a containing view keeps the textfield at the > > normal size of the font. I can provide screenshots if you want them. > > > > I expect the general layout of these views to change as UX provides more > > direction and requirements. > > > > It sounds like you just need to use a different BoxLayout::CrossAxisAlignment so > the textfield doesn't stretch. Thanks! That kept it from stretching. https://codereview.chromium.org/2883273005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:255: views::ImageView* cvc_image = new views::ImageView(); On 2017/05/24 00:20:06, Evan Stade wrote: > On 2017/05/23 23:57:50, Jared Saul wrote: > > Alternatively, if request_cvc_view stayed a smart pointer because it's being > > returned, but inner_request_cvc_view and everything else was a views::View*, > > would that be ideal instead? > > yes, this is my preference. Thanks. Acknowledged (inner_request_cvc_view went away anyway).
lgtm
The CQ bit was checked by jsaul@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org, csashi@google.com, mathp@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2883273005/#ps180001 (title: "Merge forward")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495650185590940,
"parent_rev": "acd0e4ec71292ce486b7a5d9ca5ecece6712bfa3", "commit_rev":
"335471d2d07caccbd4a89d67d86f777cf85941da"}
Message was sent while issue was closed.
Description was changed from ========== Use ViewStack to convert card upload request CVC experiment to 2-step flow BUG=707104 ========== to ========== 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/+/335471d2d07caccbd4a89d67d86f... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/335471d2d07caccbd4a89d67d86f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
