Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528503002:
[WebPayments] Implement state transitions in desktop WebPayments dialog. (Closed)
Description[WebPayments] Implement state transitions in desktop WebPayments dialog.
This CL implements a crude version of the state stack and sliding
transitions in the desktop WebPayments dialog. The states do pretty much
nothing except navigating between one another but this is intended as a
skeleton to build all the screens from.
In particular, this CL adds:
1- PaymentDialogState base class that all states inherit from
2- PaymentSheetState and OrderSummaryState, 2 implementations of
PaymentDialogState
3- PushState() and PopState() functions on PaymentRequestDialog
4- Slide in/out animations when states are pushed and popped
A document outlining this design is available at go/pr-desktop-ui
BUG=667872
TEST=
1. Enable experimental javascript features
2. Visit a page that uses WebPayments and trigger the WebPayments flow
3. The dialog should display with the "Payment Sheet" open and an
"Order Summary" button
4. Clicking "Order Summary" should slide in an "Order Summary" view
from the right. It should contain a "Back" button
5. Clicking "Back" should slide the "Order Summary" view to the right
and out of view, making "Payment Sheet" available again.
Committed: https://crrev.com/4069c4e1776b8ba44d3ddca2c2b4ce59a5fcdfc2
Cr-Commit-Position: refs/heads/master@{#438398}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Change ToggleInteractable to SetInteractable #
Total comments: 26
Patch Set 3 : Address comments. #Patch Set 4 : Move views-specific files to c/b/ui/views #Patch Set 5 : Make PaymentDialogState own the view returned through GetView #
Total comments: 36
Patch Set 6 : Address feedback #Patch Set 7 : Factor out ViewStack and animations #
Total comments: 8
Patch Set 8 : Change the ViewStack interface to take std::unique_ptr<views::View> directly. #
Total comments: 12
Patch Set 9 : Address comments #
Total comments: 7
Patch Set 10 : Handle Layout while animating #
Messages
Total messages: 53 (20 generated)
anthonyvd@chromium.org changed reviewers: + krb@chromium.org, rouslan@chromium.org
Hi Kevin and Rouslan, This CL is a basic implementation of the design I proposed for the PaymentRequest dialog states. It aims to set the groundwork required to easily build the other screens without caring about the specifics of transitions between them and provides two examples in the barebones PaymentSheetState and OrderSummaryState. Can you please take an initial look before I send it to a broader review audience? Thanks!
Can't be helped, but one of us will have some merging to do. https://codereview.chromium.org/2528503002/diff/1/chrome/browser/payments/ui/... File chrome/browser/payments/ui/payment_dialog_state.h (right): https://codereview.chromium.org/2528503002/diff/1/chrome/browser/payments/ui/... chrome/browser/payments/ui/payment_dialog_state.h:27: void ToggleInteractable(bool interactable) { It doesn't actually toggle it. I'd recommend, 'SetInteractable[To](bool)'.
Yep unfortunately we'll have a non-trivial merge somewhere. I don't mind doing it, it'll mainly be about moving your view creation code in PaymentSheetState and setting up the event handling in there as well. Thanks! https://codereview.chromium.org/2528503002/diff/1/chrome/browser/payments/ui/... File chrome/browser/payments/ui/payment_dialog_state.h (right): https://codereview.chromium.org/2528503002/diff/1/chrome/browser/payments/ui/... chrome/browser/payments/ui/payment_dialog_state.h:27: void ToggleInteractable(bool interactable) { On 2016/11/22 at 20:45:33, Kevin Bailey wrote: > It doesn't actually toggle it. I'd recommend, 'SetInteractable[To](bool)'. Good catch, done.
https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/order_summary_state.cc (right): https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/order_summary_state.cc:26: views::GridLayout* layout = new views::GridLayout(view_); Who owns this object? Raw pointers are scary. If you own it throughout the lifetime of OrderSummaryState, put it in std::unique_ptr<> member variable. If you own it initially and then pass ownership to someone else, put it in std::unique_ptr<> here and then call .release() when you've passed the ownership. If you don't own an object, add a comment about it's ownership. This point does not apply to |layout|, because you own it when you created it on line 26. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/order_summary_state.cc:28: views::ColumnSet* columns = layout->AddColumnSet(0); Who owns this object? https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/order_summary_state.cc:36: views::LabelButton* back_button = Who owns this object? https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/order_summary_state.h (right): https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/order_summary_state.h:37: private: // InterfaceBeingImplemented: https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/order_summary_state.h:40: views::View* view_; std::unique_ptr<views:View> view_; You own this object, right? https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/payment_dialog_state.h (right): https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_dialog_state.h:20: virtual views::View* GetView() = 0; Specify who owns the returned view. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_dialog_state.h:34: PaymentDialogState() {} Rule of thumb is that a constructor should always be followed by a destructor. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/payment_sheet_state.cc (right): https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.cc:24: view_->SetSize(gfx::Size(300, 300)); PaymentRequestDialog is hard-coding 200x200. https://cs.chromium.org/chromium/src/chrome/browser/payments/ui/payment_reque... Magic numbers are bad. If you're using them only for now, use the same one. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.cc:26: views::GridLayout* layout = new views::GridLayout(view_); Who owns this object. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.cc:28: views::ColumnSet* columns = layout->AddColumnSet(0); Who owns this object? https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.cc:36: views::LabelButton* order_summary_button = Who owns this object? https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/payment_sheet_state.h (right): https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.h:37: private: // InterfaceBeingImplemented: https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.h:40: views::View* view_; std::unique_ptr<> if you own this object.
anthonyvd@chromium.org changed reviewers: + sky@chromium.org
Thanks a ton for the feedback, replied to all comments. Adding sky@ to reviewers as an owner. Scott, can you please take a look at this CL? Rouslan's comments raised a good point that the views ownership wasn't *super* clear. A couple of questions specific to this: - Do you think it would be preferable to have the PaymentDialogStates subclasses call set_owned_by_client() on the views they create/return in GetView() and store them in unique_ptr's? - Would those views subviews be owned by their parent implicitly (like all views by default) or does having one in the hierarchy that is owned_by_client mean that all its children have to be manually managed too? Thanks a lot! https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/order_summary_state.cc (right): https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/order_summary_state.cc:26: views::GridLayout* layout = new views::GridLayout(view_); On 2016/11/22 at 21:39:32, rouslan wrote: > Who owns this object? Raw pointers are scary. > > If you own it throughout the lifetime of OrderSummaryState, put it in std::unique_ptr<> member variable. > > If you own it initially and then pass ownership to someone else, put it in std::unique_ptr<> here and then call .release() when you've passed the ownership. > > If you don't own an object, add a comment about it's ownership. This point does not apply to |layout|, because you own it when you created it on line 26. The view owns the layout manager implicitly. From SetLayoutManage: "The LayoutManager is owned by the View". I can change it to a unique_ptr and release it in the line below, although that's pretty unidiomatic for Views code since the views owning their LayoutManager is part of the contract between the two. Looking at GridLayout uses in the codebase, it looks like the rest of Chrome does a naked-new followed by a SetLayoutManager call. I can see the argument for the unique_ptr but I can also see the argument for consistency with the rest of the codebase. WDYT? https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/order_summary_state.cc:28: views::ColumnSet* columns = layout->AddColumnSet(0); On 2016/11/22 at 21:39:32, rouslan wrote: > Who owns this object? The GridLayout does, this function returns a non-owned/weak pointer: "GridLayout takes ownership of the ColumnSet". https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/order_summary_state.cc:36: views::LabelButton* back_button = On 2016/11/22 at 21:39:32, rouslan wrote: > Who owns this object? Views are always owned by their parent unless set_owned_by_client() is called (https://cs.chromium.org/chromium/src/ui/views/view.h?rcl=0&l=169). As long as a view is added to the hierarchy, it's lifetime is managed by the views stack. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/order_summary_state.h (right): https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/order_summary_state.h:37: private: On 2016/11/22 at 21:39:32, rouslan wrote: > // InterfaceBeingImplemented: Done. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/order_summary_state.h:40: views::View* view_; On 2016/11/22 at 21:39:32, rouslan wrote: > std::unique_ptr<views:View> view_; > > You own this object, right? I don't, its parent view does. PaymentRequestDialog adds it to the hierarchy when the state is pushed on the state stack. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/payment_dialog_state.h (right): https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_dialog_state.h:20: virtual views::View* GetView() = 0; On 2016/11/22 at 21:39:33, rouslan wrote: > Specify who owns the returned view. Done. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_dialog_state.h:34: PaymentDialogState() {} On 2016/11/22 at 21:39:32, rouslan wrote: > Rule of thumb is that a constructor should always be followed by a destructor. Done. Moved to public to satisfy unique_ptr template voodoo. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/payment_sheet_state.cc (right): https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.cc:24: view_->SetSize(gfx::Size(300, 300)); On 2016/11/22 at 21:39:33, rouslan wrote: > PaymentRequestDialog is hard-coding 200x200. > > https://cs.chromium.org/chromium/src/chrome/browser/payments/ui/payment_reque... > > Magic numbers are bad. If you're using them only for now, use the same one. Yep, this CL also changes the values in PaymentRequestDialog, mainly because those were derived from a temporary label that this CL removed. They're placeholders until the actual screens are implemented. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.cc:26: views::GridLayout* layout = new views::GridLayout(view_); On 2016/11/22 at 21:39:33, rouslan wrote: > Who owns this object. See comment above about ownership. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.cc:28: views::ColumnSet* columns = layout->AddColumnSet(0); On 2016/11/22 at 21:39:33, rouslan wrote: > Who owns this object? See above. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.cc:36: views::LabelButton* order_summary_button = On 2016/11/22 at 21:39:33, rouslan wrote: > Who owns this object? See above. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... File chrome/browser/payments/ui/payment_sheet_state.h (right): https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.h:37: private: On 2016/11/22 at 21:39:33, rouslan wrote: > // InterfaceBeingImplemented: Done. https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... chrome/browser/payments/ui/payment_sheet_state.h:40: views::View* view_; On 2016/11/22 at 21:39:33, rouslan wrote: > std::unique_ptr<> if you own this object. See above comments, this object doesn't.
owned_by_client (not the default), means you own the view and have to manually delete it. If you set owned_by_client on a view that does not change ownership of children. When a view is deleted it deletes all children that are not owned_by_client. I do agree that in your patch using owned_by_client would make ownership clearer. -Scott On Tue, Nov 22, 2016 at 2:11 PM, <anthonyvd@chromium.org> wrote: > Thanks a ton for the feedback, replied to all comments. > > Adding sky@ to reviewers as an owner. Scott, can you please take a look at > this > CL? > > Rouslan's comments raised a good point that the views ownership wasn't > *super* > clear. A couple of questions specific to this: > > - Do you think it would be preferable to have the PaymentDialogStates > subclasses > call set_owned_by_client() on the views they create/return in GetView() and > store them in unique_ptr's? > - Would those views subviews be owned by their parent implicitly (like all > views > by default) or does having one in the hierarchy that is owned_by_client mean > that all its children have to be manually managed too? > > Thanks a lot! > > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > File chrome/browser/payments/ui/order_summary_state.cc (right): > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/order_summary_state.cc:26: views::GridLayout* > layout = new views::GridLayout(view_); > On 2016/11/22 at 21:39:32, rouslan wrote: >> Who owns this object? Raw pointers are scary. >> >> If you own it throughout the lifetime of OrderSummaryState, put it in > std::unique_ptr<> member variable. >> >> If you own it initially and then pass ownership to someone else, put > it in std::unique_ptr<> here and then call .release() when you've passed > the ownership. >> >> If you don't own an object, add a comment about it's ownership. This > point does not apply to |layout|, because you own it when you created it > on line 26. > > The view owns the layout manager implicitly. From SetLayoutManage: "The > LayoutManager is owned by the View". > > I can change it to a unique_ptr and release it in the line below, > although that's pretty unidiomatic for Views code since the views owning > their LayoutManager is part of the contract between the two. Looking at > GridLayout uses in the codebase, it looks like the rest of Chrome does a > naked-new followed by a SetLayoutManager call. > > I can see the argument for the unique_ptr but I can also see the > argument for consistency with the rest of the codebase. WDYT? > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/order_summary_state.cc:28: views::ColumnSet* > columns = layout->AddColumnSet(0); > On 2016/11/22 at 21:39:32, rouslan wrote: >> Who owns this object? > > The GridLayout does, this function returns a non-owned/weak pointer: > "GridLayout takes ownership of the ColumnSet". > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/order_summary_state.cc:36: > views::LabelButton* back_button = > On 2016/11/22 at 21:39:32, rouslan wrote: >> Who owns this object? > > Views are always owned by their parent unless set_owned_by_client() is > called > (https://cs.chromium.org/chromium/src/ui/views/view.h?rcl=0&l=169). As > long as a view is added to the hierarchy, it's lifetime is managed by > the views stack. > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > File chrome/browser/payments/ui/order_summary_state.h (right): > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/order_summary_state.h:37: private: > On 2016/11/22 at 21:39:32, rouslan wrote: >> // InterfaceBeingImplemented: > > Done. > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/order_summary_state.h:40: views::View* view_; > On 2016/11/22 at 21:39:32, rouslan wrote: >> std::unique_ptr<views:View> view_; >> >> You own this object, right? > > I don't, its parent view does. PaymentRequestDialog adds it to the > hierarchy when the state is pushed on the state stack. > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > File chrome/browser/payments/ui/payment_dialog_state.h (right): > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/payment_dialog_state.h:20: virtual > views::View* GetView() = 0; > On 2016/11/22 at 21:39:33, rouslan wrote: >> Specify who owns the returned view. > > Done. > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/payment_dialog_state.h:34: > PaymentDialogState() {} > On 2016/11/22 at 21:39:32, rouslan wrote: >> Rule of thumb is that a constructor should always be followed by a > destructor. > > Done. Moved to public to satisfy unique_ptr template voodoo. > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > File chrome/browser/payments/ui/payment_sheet_state.cc (right): > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/payment_sheet_state.cc:24: > view_->SetSize(gfx::Size(300, 300)); > On 2016/11/22 at 21:39:33, rouslan wrote: >> PaymentRequestDialog is hard-coding 200x200. >> >> > https://cs.chromium.org/chromium/src/chrome/browser/payments/ui/payment_reque... >> >> Magic numbers are bad. If you're using them only for now, use the same > one. > > Yep, this CL also changes the values in PaymentRequestDialog, mainly > because those were derived from a temporary label that this CL removed. > > They're placeholders until the actual screens are implemented. > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/payment_sheet_state.cc:26: views::GridLayout* > layout = new views::GridLayout(view_); > On 2016/11/22 at 21:39:33, rouslan wrote: >> Who owns this object. > > See comment above about ownership. > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/payment_sheet_state.cc:28: views::ColumnSet* > columns = layout->AddColumnSet(0); > On 2016/11/22 at 21:39:33, rouslan wrote: >> Who owns this object? > > See above. > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/payment_sheet_state.cc:36: > views::LabelButton* order_summary_button = > On 2016/11/22 at 21:39:33, rouslan wrote: >> Who owns this object? > > See above. > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > File chrome/browser/payments/ui/payment_sheet_state.h (right): > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/payment_sheet_state.h:37: private: > On 2016/11/22 at 21:39:33, rouslan wrote: >> // InterfaceBeingImplemented: > > Done. > > https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > chrome/browser/payments/ui/payment_sheet_state.h:40: views::View* view_; > On 2016/11/22 at 21:39:33, rouslan wrote: >> std::unique_ptr<> if you own this object. > > See above comments, this object doesn't. > > https://codereview.chromium.org/2528503002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Also, given your code is views specific I would expect your code to live under c/b/ui/views/payments. On Tue, Nov 22, 2016 at 3:29 PM, Scott Violet <sky@chromium.org> wrote: > owned_by_client (not the default), means you own the view and have to > manually delete it. If you set owned_by_client on a view that does not > change ownership of children. When a view is deleted it deletes all > children that are not owned_by_client. I do agree that in your patch > using owned_by_client would make ownership clearer. > > > > -Scott > > On Tue, Nov 22, 2016 at 2:11 PM, <anthonyvd@chromium.org> wrote: >> Thanks a ton for the feedback, replied to all comments. >> >> Adding sky@ to reviewers as an owner. Scott, can you please take a look at >> this >> CL? >> >> Rouslan's comments raised a good point that the views ownership wasn't >> *super* >> clear. A couple of questions specific to this: >> >> - Do you think it would be preferable to have the PaymentDialogStates >> subclasses >> call set_owned_by_client() on the views they create/return in GetView() and >> store them in unique_ptr's? >> - Would those views subviews be owned by their parent implicitly (like all >> views >> by default) or does having one in the hierarchy that is owned_by_client mean >> that all its children have to be manually managed too? >> >> Thanks a lot! >> >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> File chrome/browser/payments/ui/order_summary_state.cc (right): >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/order_summary_state.cc:26: views::GridLayout* >> layout = new views::GridLayout(view_); >> On 2016/11/22 at 21:39:32, rouslan wrote: >>> Who owns this object? Raw pointers are scary. >>> >>> If you own it throughout the lifetime of OrderSummaryState, put it in >> std::unique_ptr<> member variable. >>> >>> If you own it initially and then pass ownership to someone else, put >> it in std::unique_ptr<> here and then call .release() when you've passed >> the ownership. >>> >>> If you don't own an object, add a comment about it's ownership. This >> point does not apply to |layout|, because you own it when you created it >> on line 26. >> >> The view owns the layout manager implicitly. From SetLayoutManage: "The >> LayoutManager is owned by the View". >> >> I can change it to a unique_ptr and release it in the line below, >> although that's pretty unidiomatic for Views code since the views owning >> their LayoutManager is part of the contract between the two. Looking at >> GridLayout uses in the codebase, it looks like the rest of Chrome does a >> naked-new followed by a SetLayoutManager call. >> >> I can see the argument for the unique_ptr but I can also see the >> argument for consistency with the rest of the codebase. WDYT? >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/order_summary_state.cc:28: views::ColumnSet* >> columns = layout->AddColumnSet(0); >> On 2016/11/22 at 21:39:32, rouslan wrote: >>> Who owns this object? >> >> The GridLayout does, this function returns a non-owned/weak pointer: >> "GridLayout takes ownership of the ColumnSet". >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/order_summary_state.cc:36: >> views::LabelButton* back_button = >> On 2016/11/22 at 21:39:32, rouslan wrote: >>> Who owns this object? >> >> Views are always owned by their parent unless set_owned_by_client() is >> called >> (https://cs.chromium.org/chromium/src/ui/views/view.h?rcl=0&l=169). As >> long as a view is added to the hierarchy, it's lifetime is managed by >> the views stack. >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> File chrome/browser/payments/ui/order_summary_state.h (right): >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/order_summary_state.h:37: private: >> On 2016/11/22 at 21:39:32, rouslan wrote: >>> // InterfaceBeingImplemented: >> >> Done. >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/order_summary_state.h:40: views::View* view_; >> On 2016/11/22 at 21:39:32, rouslan wrote: >>> std::unique_ptr<views:View> view_; >>> >>> You own this object, right? >> >> I don't, its parent view does. PaymentRequestDialog adds it to the >> hierarchy when the state is pushed on the state stack. >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> File chrome/browser/payments/ui/payment_dialog_state.h (right): >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/payment_dialog_state.h:20: virtual >> views::View* GetView() = 0; >> On 2016/11/22 at 21:39:33, rouslan wrote: >>> Specify who owns the returned view. >> >> Done. >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/payment_dialog_state.h:34: >> PaymentDialogState() {} >> On 2016/11/22 at 21:39:32, rouslan wrote: >>> Rule of thumb is that a constructor should always be followed by a >> destructor. >> >> Done. Moved to public to satisfy unique_ptr template voodoo. >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> File chrome/browser/payments/ui/payment_sheet_state.cc (right): >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/payment_sheet_state.cc:24: >> view_->SetSize(gfx::Size(300, 300)); >> On 2016/11/22 at 21:39:33, rouslan wrote: >>> PaymentRequestDialog is hard-coding 200x200. >>> >>> >> https://cs.chromium.org/chromium/src/chrome/browser/payments/ui/payment_reque... >>> >>> Magic numbers are bad. If you're using them only for now, use the same >> one. >> >> Yep, this CL also changes the values in PaymentRequestDialog, mainly >> because those were derived from a temporary label that this CL removed. >> >> They're placeholders until the actual screens are implemented. >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/payment_sheet_state.cc:26: views::GridLayout* >> layout = new views::GridLayout(view_); >> On 2016/11/22 at 21:39:33, rouslan wrote: >>> Who owns this object. >> >> See comment above about ownership. >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/payment_sheet_state.cc:28: views::ColumnSet* >> columns = layout->AddColumnSet(0); >> On 2016/11/22 at 21:39:33, rouslan wrote: >>> Who owns this object? >> >> See above. >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/payment_sheet_state.cc:36: >> views::LabelButton* order_summary_button = >> On 2016/11/22 at 21:39:33, rouslan wrote: >>> Who owns this object? >> >> See above. >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> File chrome/browser/payments/ui/payment_sheet_state.h (right): >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/payment_sheet_state.h:37: private: >> On 2016/11/22 at 21:39:33, rouslan wrote: >>> // InterfaceBeingImplemented: >> >> Done. >> >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... >> chrome/browser/payments/ui/payment_sheet_state.h:40: views::View* view_; >> On 2016/11/22 at 21:39:33, rouslan wrote: >>> std::unique_ptr<> if you own this object. >> >> See above comments, this object doesn't. >> >> https://codereview.chromium.org/2528503002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/22 at 23:30:31, sky wrote: > Also, given your code is views specific I would expect your code to > live under c/b/ui/views/payments. > > On Tue, Nov 22, 2016 at 3:29 PM, Scott Violet <sky@chromium.org> wrote: > > owned_by_client (not the default), means you own the view and have to > > manually delete it. If you set owned_by_client on a view that does not > > change ownership of children. When a view is deleted it deletes all > > children that are not owned_by_client. I do agree that in your patch > > using owned_by_client would make ownership clearer. > > > > > > > > -Scott > > > > On Tue, Nov 22, 2016 at 2:11 PM, <anthonyvd@chromium.org> wrote: > >> Thanks a ton for the feedback, replied to all comments. > >> > >> Adding sky@ to reviewers as an owner. Scott, can you please take a look at > >> this > >> CL? > >> > >> Rouslan's comments raised a good point that the views ownership wasn't > >> *super* > >> clear. A couple of questions specific to this: > >> > >> - Do you think it would be preferable to have the PaymentDialogStates > >> subclasses > >> call set_owned_by_client() on the views they create/return in GetView() and > >> store them in unique_ptr's? > >> - Would those views subviews be owned by their parent implicitly (like all > >> views > >> by default) or does having one in the hierarchy that is owned_by_client mean > >> that all its children have to be manually managed too? > >> > >> Thanks a lot! > >> > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> File chrome/browser/payments/ui/order_summary_state.cc (right): > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/order_summary_state.cc:26: views::GridLayout* > >> layout = new views::GridLayout(view_); > >> On 2016/11/22 at 21:39:32, rouslan wrote: > >>> Who owns this object? Raw pointers are scary. > >>> > >>> If you own it throughout the lifetime of OrderSummaryState, put it in > >> std::unique_ptr<> member variable. > >>> > >>> If you own it initially and then pass ownership to someone else, put > >> it in std::unique_ptr<> here and then call .release() when you've passed > >> the ownership. > >>> > >>> If you don't own an object, add a comment about it's ownership. This > >> point does not apply to |layout|, because you own it when you created it > >> on line 26. > >> > >> The view owns the layout manager implicitly. From SetLayoutManage: "The > >> LayoutManager is owned by the View". > >> > >> I can change it to a unique_ptr and release it in the line below, > >> although that's pretty unidiomatic for Views code since the views owning > >> their LayoutManager is part of the contract between the two. Looking at > >> GridLayout uses in the codebase, it looks like the rest of Chrome does a > >> naked-new followed by a SetLayoutManager call. > >> > >> I can see the argument for the unique_ptr but I can also see the > >> argument for consistency with the rest of the codebase. WDYT? > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/order_summary_state.cc:28: views::ColumnSet* > >> columns = layout->AddColumnSet(0); > >> On 2016/11/22 at 21:39:32, rouslan wrote: > >>> Who owns this object? > >> > >> The GridLayout does, this function returns a non-owned/weak pointer: > >> "GridLayout takes ownership of the ColumnSet". > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/order_summary_state.cc:36: > >> views::LabelButton* back_button = > >> On 2016/11/22 at 21:39:32, rouslan wrote: > >>> Who owns this object? > >> > >> Views are always owned by their parent unless set_owned_by_client() is > >> called > >> (https://cs.chromium.org/chromium/src/ui/views/view.h?rcl=0&l=169). As > >> long as a view is added to the hierarchy, it's lifetime is managed by > >> the views stack. > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> File chrome/browser/payments/ui/order_summary_state.h (right): > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/order_summary_state.h:37: private: > >> On 2016/11/22 at 21:39:32, rouslan wrote: > >>> // InterfaceBeingImplemented: > >> > >> Done. > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/order_summary_state.h:40: views::View* view_; > >> On 2016/11/22 at 21:39:32, rouslan wrote: > >>> std::unique_ptr<views:View> view_; > >>> > >>> You own this object, right? > >> > >> I don't, its parent view does. PaymentRequestDialog adds it to the > >> hierarchy when the state is pushed on the state stack. > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> File chrome/browser/payments/ui/payment_dialog_state.h (right): > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/payment_dialog_state.h:20: virtual > >> views::View* GetView() = 0; > >> On 2016/11/22 at 21:39:33, rouslan wrote: > >>> Specify who owns the returned view. > >> > >> Done. > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/payment_dialog_state.h:34: > >> PaymentDialogState() {} > >> On 2016/11/22 at 21:39:32, rouslan wrote: > >>> Rule of thumb is that a constructor should always be followed by a > >> destructor. > >> > >> Done. Moved to public to satisfy unique_ptr template voodoo. > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> File chrome/browser/payments/ui/payment_sheet_state.cc (right): > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/payment_sheet_state.cc:24: > >> view_->SetSize(gfx::Size(300, 300)); > >> On 2016/11/22 at 21:39:33, rouslan wrote: > >>> PaymentRequestDialog is hard-coding 200x200. > >>> > >>> > >> https://cs.chromium.org/chromium/src/chrome/browser/payments/ui/payment_reque... > >>> > >>> Magic numbers are bad. If you're using them only for now, use the same > >> one. > >> > >> Yep, this CL also changes the values in PaymentRequestDialog, mainly > >> because those were derived from a temporary label that this CL removed. > >> > >> They're placeholders until the actual screens are implemented. > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/payment_sheet_state.cc:26: views::GridLayout* > >> layout = new views::GridLayout(view_); > >> On 2016/11/22 at 21:39:33, rouslan wrote: > >>> Who owns this object. > >> > >> See comment above about ownership. > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/payment_sheet_state.cc:28: views::ColumnSet* > >> columns = layout->AddColumnSet(0); > >> On 2016/11/22 at 21:39:33, rouslan wrote: > >>> Who owns this object? > >> > >> See above. > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/payment_sheet_state.cc:36: > >> views::LabelButton* order_summary_button = > >> On 2016/11/22 at 21:39:33, rouslan wrote: > >>> Who owns this object? > >> > >> See above. > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> File chrome/browser/payments/ui/payment_sheet_state.h (right): > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/payment_sheet_state.h:37: private: > >> On 2016/11/22 at 21:39:33, rouslan wrote: > >>> // InterfaceBeingImplemented: > >> > >> Done. > >> > >> https://codereview.chromium.org/2528503002/diff/20001/chrome/browser/payments... > >> chrome/browser/payments/ui/payment_sheet_state.h:40: views::View* view_; > >> On 2016/11/22 at 21:39:33, rouslan wrote: > >>> std::unique_ptr<> if you own this object. > >> > >> See above comments, this object doesn't. > >> > >> https://codereview.chromium.org/2528503002/ > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. > > Thanks a ton for the advice. I'll modify this code to use set_owned_by_client. I'll also move it to c/b/ui/views/payments (this folder already contained a DialogDelegateView and a dependency on view so I was under the impression that this could live here, good to know!).
The CQ bit was checked by anthonyvd@chromium.org 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...
Hello everyone. The latest 2 patchsets address the feedback regarding: - views-specific code living in c/b/ui/views - making the ownership of the views involved clearer by having PaymentDialogState own the view in a unique_ptr after calling set_owned_by_client() on them Can you please take another look? Thanks a ton for your feedback!
lgtm % comment https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_dialog_state.cc (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_dialog_state.cc:11: PaymentDialogState::PaymentDialogState() Please initialize interactiable_ boolean here. Otherwise it's random.
Description was changed from ========== [WebPayments] Implement state transitions in desktop WebPayments dialog. This CL implements a crude version of the state stack and sliding transitions in the desktop WebPayments dialog. The states do pretty much nothing except navigating between one another but this is intended as a skeleton to build all the screens from. In particular, this CL adds: 1- PaymentDialogState base class that all states inherit from 2- PaymentSheetState and OrderSummaryState, 2 implementations of PaymentDialogState 3- PushState() and PopState() functions on PaymentRequestDialog 4- Slide in/out animations when states are pushed and popped A document outlining this design is available at go/pr-desktop-ui BUG=667872 TEST= 1. Enable experimental javascript features 2. Visit a page that uses WebPayments and trigger the WebPayments flow 3. The dialog should display with the "Payment Sheet" open and an "Order Summary" button 4. Clicking "Order Summary" should slide in an "Order Summary" view from the right. It should contain a "Back" button 5. Clicking "Back" should slide the "Order Summary" view to the right and out of view, making "Payment Sheet" available again. ========== to ========== [WebPayments] Implement state transitions in desktop WebPayments dialog. This CL implements a crude version of the state stack and sliding transitions in the desktop WebPayments dialog. The states do pretty much nothing except navigating between one another but this is intended as a skeleton to build all the screens from. In particular, this CL adds: 1- PaymentDialogState base class that all states inherit from 2- PaymentSheetState and OrderSummaryState, 2 implementations of PaymentDialogState 3- PushState() and PopState() functions on PaymentRequestDialog 4- Slide in/out animations when states are pushed and popped A document outlining this design is available at go/pr-desktop-ui BUG=667872 TEST= 1. Enable experimental javascript features 2. Visit a page that uses WebPayments and trigger the WebPayments flow 3. The dialog should display with the "Payment Sheet" open and an "Order Summary" button 4. Clicking "Order Summary" should slide in an "Order Summary" view from the right. It should contain a "Back" button 5. Clicking "Back" should slide the "Order Summary" view to the right and out of view, making "Payment Sheet" available again. ==========
sky@chromium.org changed reviewers: + ellyjones@chromium.org
+elly as she is making layout consistent and it's likely better to change it now rather than later. I think you should move the logic of the stack and animations into a separate class (that isn't payment specific). It's easier to test that way. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_state.cc (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_state.cc:15: const int kBackButtonTag = 0; constexpr, also move into anonymous namespace and add description. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_state.cc:23: view->SetSize(gfx::Size(300, 300)); Why are you setting the size? The size should come from the layout manager, or the parent. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_state.cc:32: layout->AddView(new views::Label(base::ASCIIToUTF16("Order Summary"))); Shouldn't you be looking these names up from resources? https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_state.cc:57: if (sender->tag() == kBackButtonTag) { DCHECK_EQ as there is only one button. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_state.h (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_state.h:23: class Observer { Move to it's own header (see chromium style guide). https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_dialog_state.cc (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_dialog_state.cc:12: : view_(nullptr) {} No need to initialize view_ here (default value is fine). https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_dialog_state.h (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_dialog_state.h:28: // Marks the state as interactable or not depending on |interactable|. States This seems error prone, and a very weird interface that you allow the user to interact with ui elements but have them do nothing. Did you consider changing the enabled state of the views? Assuming you don't want to do that, having each view know to correctly deal with this is error prone. I recommend having a parent view that you can toggle CanProcessEventsWithinSubtree() when you don't want the user to interact with it. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:47: parent_view_->RemoveChildView(state_animating_out_->GetView()); Is this necessary? https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:53: }; DISALLOW... https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:73: slide_in_animator_->Cancel(); Are you sure cancel resluts in OnBoundsAnimatorDone? https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:83: state->GetView()->SetPosition(gfx::Point(x() + width(), y())); I would expect this to SetBounds, by which I mean explicitly setting the size too. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:87: slide_in_animator_->AnimateViewTo(state->GetView(), bounds()); bounds() is relative to the parent, where as the view is a child of this, so it should have an origin of 0,0. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:98: gfx::Rect destination = bounds(); Similar comment about bounds here, you want a origin of 0x0, (well, really you want origin of width(), 0). https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:118: return gfx::Size(300, 300); How do you know 300x300 is going to be enough? https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.h (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:54: std::stack<std::unique_ptr<PaymentDialogState> > state_stack_; You shouldn't need the space between '>' and '>'. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_state.cc (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_state.cc:32: layout->AddView(new views::Label(base::ASCIIToUTF16("Payments Sheet"))); Same comments about names. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_state.h (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_state.h:23: class Observer { Similar comment about moving to standalone file.
lgtm
Thanks for the feedback! Addressed all comments except the one about factoring out the stack and animations out of this and into a generic location. Do you think it should happen here or in a different CL? I'm fine with either. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_state.cc (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_state.cc:15: const int kBackButtonTag = 0; On 2016/12/08 at 03:58:16, sky wrote: > constexpr, also move into anonymous namespace and add description. Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_state.cc:23: view->SetSize(gfx::Size(300, 300)); On 2016/12/08 at 03:58:17, sky wrote: > Why are you setting the size? The size should come from the layout manager, or the parent. Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_state.cc:32: layout->AddView(new views::Label(base::ASCIIToUTF16("Order Summary"))); On 2016/12/08 at 03:58:16, sky wrote: > Shouldn't you be looking these names up from resources? Those controls are there temporarily to have something in the view (since the layout itself isn't implemented at all yet). I changed the label strings to pull their value from resources as those are here to stay. As for the buttons, those are going to be replaced (by icons in some cases and clickable sections in some others) so I left the temporary strings in. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_state.cc:57: if (sender->tag() == kBackButtonTag) { On 2016/12/08 at 03:58:17, sky wrote: > DCHECK_EQ as there is only one button. Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_state.h (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_state.h:23: class Observer { On 2016/12/08 at 03:58:17, sky wrote: > Move to it's own header (see chromium style guide). Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_dialog_state.cc (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_dialog_state.cc:11: PaymentDialogState::PaymentDialogState() On 2016/12/07 at 23:29:19, rouslan wrote: > Please initialize interactiable_ boolean here. Otherwise it's random. Ack. interactable_ doesn't exist anymore as of patchset 6. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_dialog_state.cc:12: : view_(nullptr) {} On 2016/12/08 at 03:58:17, sky wrote: > No need to initialize view_ here (default value is fine). Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_dialog_state.h (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_dialog_state.h:28: // Marks the state as interactable or not depending on |interactable|. States On 2016/12/08 at 03:58:17, sky wrote: > This seems error prone, and a very weird interface that you allow the user to interact with ui elements but have them do nothing. Did you consider changing the enabled state of the views? Assuming you don't want to do that, having each view know to correctly deal with this is error prone. I recommend having a parent view that you can toggle CanProcessEventsWithinSubtree() when you don't want the user to interact with it. I didn't know about CanProcessEventsWithinSubtree(), this is much better! Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:47: parent_view_->RemoveChildView(state_animating_out_->GetView()); On 2016/12/08 at 03:58:17, sky wrote: > Is this necessary? Ahh, it's not! Because the state being deleted also deletes the View and it removes itself from its parent in its destructor. Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:53: }; On 2016/12/08 at 03:58:17, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:73: slide_in_animator_->Cancel(); On 2016/12/08 at 03:58:17, sky wrote: > Are you sure cancel resluts in OnBoundsAnimatorDone? Yes, Cancel() ends up calling BoundsAnimator::AnimationEndedOrCanceled (through Animation::Stop()), which removes the view from data_. Because of this, the following BoundsAnimator::AnimationContainerProgressed() call will enter the !IsAnimating() branch and call OnBoundsAnimatorDone(). Also tested it manually to make sure this assumption was correct. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:83: state->GetView()->SetPosition(gfx::Point(x() + width(), y())); On 2016/12/08 at 03:58:17, sky wrote: > I would expect this to SetBounds, by which I mean explicitly setting the size too. Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:87: slide_in_animator_->AnimateViewTo(state->GetView(), bounds()); On 2016/12/08 at 03:58:17, sky wrote: > bounds() is relative to the parent, where as the view is a child of this, so it should have an origin of 0,0. Ah yes, so this only worked because the parent was at 0, 0 in this case. Done, thanks. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:98: gfx::Rect destination = bounds(); On 2016/12/08 at 03:58:17, sky wrote: > Similar comment about bounds here, you want a origin of 0x0, (well, really you want origin of width(), 0). Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:118: return gfx::Size(300, 300); On 2016/12/08 at 03:58:17, sky wrote: > How do you know 300x300 is going to be enough? I don't, this is a temporary value until we figure out the correct sizing behavior. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.h (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:54: std::stack<std::unique_ptr<PaymentDialogState> > state_stack_; On 2016/12/08 at 03:58:17, sky wrote: > You shouldn't need the space between '>' and '>'. Done. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_state.cc (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_state.cc:32: layout->AddView(new views::Label(base::ASCIIToUTF16("Payments Sheet"))); On 2016/12/08 at 03:58:17, sky wrote: > Same comments about names. Ack, replied to above comment. https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_state.h (right): https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_state.h:23: class Observer { On 2016/12/08 at 03:58:17, sky wrote: > Similar comment about moving to standalone file. Done.
I think you should do the refactoring here because that'll allow you to more easily test this code. To be clear I think the animation/stack code should remain in the directory you have it. If we start needing it else where we can move to ui/views, or another place then. Also, doing animations at the layer level is going to be more performant, but I'm not sure you'll really notice it for these animations, so I leave it to you to decide if you want to use animations at the layer level. -Scott On Thu, Dec 8, 2016 at 12:31 PM, <anthonyvd@chromium.org> wrote: > Thanks for the feedback! > > Addressed all comments except the one about factoring out the stack and > animations out of this and into a generic location. Do you think it should > happen here or in a different CL? I'm fine with either. > > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/order_summary_state.cc (right): > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/order_summary_state.cc:15: const int > kBackButtonTag = 0; > On 2016/12/08 at 03:58:16, sky wrote: >> constexpr, also move into anonymous namespace and add description. > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/order_summary_state.cc:23: > view->SetSize(gfx::Size(300, 300)); > On 2016/12/08 at 03:58:17, sky wrote: >> Why are you setting the size? The size should come from the layout > manager, or the parent. > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/order_summary_state.cc:32: > layout->AddView(new views::Label(base::ASCIIToUTF16("Order Summary"))); > On 2016/12/08 at 03:58:16, sky wrote: >> Shouldn't you be looking these names up from resources? > > Those controls are there temporarily to have something in the view > (since the layout itself isn't implemented at all yet). I changed the > label strings to pull their value from resources as those are here to > stay. As for the buttons, those are going to be replaced (by icons in > some cases and clickable sections in some others) so I left the > temporary strings in. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/order_summary_state.cc:57: if > (sender->tag() == kBackButtonTag) { > On 2016/12/08 at 03:58:17, sky wrote: >> DCHECK_EQ as there is only one button. > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/order_summary_state.h (right): > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/order_summary_state.h:23: class > Observer { > On 2016/12/08 at 03:58:17, sky wrote: >> Move to it's own header (see chromium style guide). > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/payment_dialog_state.cc (right): > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_dialog_state.cc:11: > PaymentDialogState::PaymentDialogState() > On 2016/12/07 at 23:29:19, rouslan wrote: >> Please initialize interactiable_ boolean here. Otherwise it's random. > > Ack. interactable_ doesn't exist anymore as of patchset 6. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_dialog_state.cc:12: : > view_(nullptr) {} > On 2016/12/08 at 03:58:17, sky wrote: >> No need to initialize view_ here (default value is fine). > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/payment_dialog_state.h (right): > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_dialog_state.h:28: // Marks the > state as interactable or not depending on |interactable|. States > On 2016/12/08 at 03:58:17, sky wrote: >> This seems error prone, and a very weird interface that you allow the > user to interact with ui elements but have them do nothing. Did you > consider changing the enabled state of the views? Assuming you don't > want to do that, having each view know to correctly deal with this is > error prone. I recommend having a parent view that you can toggle > CanProcessEventsWithinSubtree() when you don't want the user to interact > with it. > > I didn't know about CanProcessEventsWithinSubtree(), this is much > better! > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_dialog.cc:47: > parent_view_->RemoveChildView(state_animating_out_->GetView()); > On 2016/12/08 at 03:58:17, sky wrote: >> Is this necessary? > > Ahh, it's not! Because the state being deleted also deletes the View and > it removes itself from its parent in its destructor. > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_dialog.cc:53: }; > On 2016/12/08 at 03:58:17, sky wrote: >> DISALLOW... > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_dialog.cc:73: > slide_in_animator_->Cancel(); > On 2016/12/08 at 03:58:17, sky wrote: >> Are you sure cancel resluts in OnBoundsAnimatorDone? > > Yes, Cancel() ends up calling BoundsAnimator::AnimationEndedOrCanceled > (through Animation::Stop()), which removes the view from data_. Because > of this, the following BoundsAnimator::AnimationContainerProgressed() > call will enter the !IsAnimating() branch and call > OnBoundsAnimatorDone(). > > Also tested it manually to make sure this assumption was correct. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_dialog.cc:83: > state->GetView()->SetPosition(gfx::Point(x() + width(), y())); > On 2016/12/08 at 03:58:17, sky wrote: >> I would expect this to SetBounds, by which I mean explicitly setting > the size too. > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_dialog.cc:87: > slide_in_animator_->AnimateViewTo(state->GetView(), bounds()); > On 2016/12/08 at 03:58:17, sky wrote: >> bounds() is relative to the parent, where as the view is a child of > this, so it should have an origin of 0,0. > > Ah yes, so this only worked because the parent was at 0, 0 in this case. > > Done, thanks. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_dialog.cc:98: gfx::Rect > destination = bounds(); > On 2016/12/08 at 03:58:17, sky wrote: >> Similar comment about bounds here, you want a origin of 0x0, (well, > really you want origin of width(), 0). > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_dialog.cc:118: return > gfx::Size(300, 300); > On 2016/12/08 at 03:58:17, sky wrote: >> How do you know 300x300 is going to be enough? > > I don't, this is a temporary value until we figure out the correct > sizing behavior. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/payment_request_dialog.h (right): > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_dialog.h:54: > std::stack<std::unique_ptr<PaymentDialogState> > state_stack_; > On 2016/12/08 at 03:58:17, sky wrote: >> You shouldn't need the space between '>' and '>'. > > Done. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/payment_sheet_state.cc (right): > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_sheet_state.cc:32: > layout->AddView(new views::Label(base::ASCIIToUTF16("Payments Sheet"))); > On 2016/12/08 at 03:58:17, sky wrote: >> Same comments about names. > > Ack, replied to above comment. > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/payment_sheet_state.h (right): > > https://codereview.chromium.org/2528503002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_sheet_state.h:23: class > Observer { > On 2016/12/08 at 03:58:17, sky wrote: >> Similar comment about moving to standalone file. > > Done. > > https://codereview.chromium.org/2528503002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Makes a lot of sense. I refactored the stack/animation logic into the ViewStack and ViewStackState classes and added some tests. As for layers, I've never used them so I'm more comfortable with this method. Since as you mention the performance difference should be very minimal here, I'd land this as-is and make the move to layers if better performance is required down the line. Please take a look. Thanks again for your time and valuable feedback!
The CQ bit was checked by anthonyvd@chromium.org 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...
I won't be able to get to this again until Monday. If this is urgent you'll need to find another reviewer (maybe msw@). If not, I'll review first thing Monday (possibly Sunday evening, but no guarantees). -Scott On Fri, Dec 9, 2016 at 1:26 PM, <anthonyvd@chromium.org> wrote: > Makes a lot of sense. I refactored the stack/animation logic into the > ViewStack > and ViewStackState classes and added some tests. > > As for layers, I've never used them so I'm more comfortable with this > method. > Since as you mention the performance difference should be very minimal here, > I'd > land this as-is and make the move to layers if better performance is > required > down the line. > > Please take a look. Thanks again for your time and valuable feedback! > > https://codereview.chromium.org/2528503002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/09 at 21:45:17, sky wrote: > I won't be able to get to this again until Monday. If this is urgent > you'll need to find another reviewer (maybe msw@). If not, I'll review > first thing Monday (possibly Sunday evening, but no guarantees). > > -Scott > > On Fri, Dec 9, 2016 at 1:26 PM, <anthonyvd@chromium.org> wrote: > > Makes a lot of sense. I refactored the stack/animation logic into the > > ViewStack > > and ViewStackState classes and added some tests. > > > > As for layers, I've never used them so I'm more comfortable with this > > method. > > Since as you mention the performance difference should be very minimal here, > > I'd > > land this as-is and make the move to layers if better performance is > > required > > down the line. > > > > Please take a look. Thanks again for your time and valuable feedback! > > > > https://codereview.chromium.org/2528503002/ > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. > > Thanks for the heads up! This isn't urgent, no rush :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_dialog.cc:35: view_stack_.reset(new ViewStack(std::move(initial_state))); Can you elaborate on why ViewStack can't operate solely on View? It seems like ViewStack could contain a stack of Views, and not a stack of ViewStackState. If you need to associate extra data with the Views can't they be done here and not in ViewStack? I'm suggesting the API for ViewStack is: PushState(std::unique_ptr<View> view); PopState(). https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack.cc (right): https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:13: class SlideOutObserver : public views::BoundsAnimatorObserver { Move to anonymous namespace? https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:61: state->GetView()->SetBounds(width(), 0, width(), height()); You should call Layout() on the view after changing the bounds. https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack.h (right): https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.h:34: // The children of this view must not be able to process events when the views do you really want to disable all events when animating, or just events to the view being animated out?
Thanks for the timely review, comments addressed. https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_dialog.cc:35: view_stack_.reset(new ViewStack(std::move(initial_state))); On 2016/12/12 at 01:54:32, sky wrote: > Can you elaborate on why ViewStack can't operate solely on View? It seems like ViewStack could contain a stack of Views, and not a stack of ViewStackState. If you need to associate extra data with the Views can't they be done here and not in ViewStack? I'm suggesting the API for ViewStack is: > PushState(std::unique_ptr<View> view); > PopState(). I felt like encapsulating event handling in the states would make the dialog code cleaner (for example, it avoids huge functions that switch on button->tag() to figure out what to do). However now that ViewStack is independent of payments, the interface you suggest probably makes more sense. The added complexity + forcing callers to subclass ViewStackState isn't worth the slightly more separated code. The latest patchset implements that interface, let me know what you think! https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack.cc (right): https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:13: class SlideOutObserver : public views::BoundsAnimatorObserver { On 2016/12/12 at 01:54:32, sky wrote: > Move to anonymous namespace? Done. https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:61: state->GetView()->SetBounds(width(), 0, width(), height()); On 2016/12/12 at 01:54:32, sky wrote: > You should call Layout() on the view after changing the bounds. Done. https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack.h (right): https://codereview.chromium.org/2528503002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.h:34: // The children of this view must not be able to process events when the views On 2016/12/12 at 01:54:32, sky wrote: > do you really want to disable all events when animating, or just events to the view being animated out? I think we want all events disabled when animating in and out. Considering the speed of the animation, it's unlikely that the user will try to perform an event on the view that's not being animated anyways. It'll also prevent accidental events in a partially obscured view. Since this attempts to implement some Material Design style behavior, I can run it past UXers once this lands to see what they think.
https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack.cc (right): https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:14: class SlideOutObserver : public views::BoundsAnimatorObserver { Is this class needed? If so, what for? It seems its like the only thing it does is to wait for the animation to be done and then delete itself. https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:40: : slide_in_animator_(new views::BoundsAnimator(this)), MakeUnique where possible. https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:58: You should override Layout to resize the views. This is important for the initial view. You'll have to decide how to deal with a layout during animation (more specifically a resize). https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack.h (right): https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.h:19: explicit ViewStack(std::unique_ptr<views::View> initial_view); Can you make the constructor take no args and instead consumers call Push when ready? I think it makes for a simpler API, with less code duplication (for example, you forgot the set_owned_by_client for the view passed to the constructor). https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.h:53: bool can_process_events_within_subtree_; Does the value of this always match that of whether an animation is running? Can it be removed and instead check the animations (less duplication means smaller possibility of getting things out of sync).
Patchset #9 (id:160001) has been deleted
https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack.cc (right): https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:14: class SlideOutObserver : public views::BoundsAnimatorObserver { On 2016/12/12 at 17:38:07, sky wrote: > Is this class needed? If so, what for? It seems its like the only thing it does is to wait for the animation to be done and then delete itself. Its initial purpose was to remove the view from the hierarchy and properly delete it after the animation was over. The ViewStack can do that itself though, so moved the cleanup to ViewStack::OnBoundsAnimatorDone() below. https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:40: : slide_in_animator_(new views::BoundsAnimator(this)), On 2016/12/12 at 17:38:07, sky wrote: > MakeUnique where possible. Done. https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:58: On 2016/12/12 at 17:38:07, sky wrote: > You should override Layout to resize the views. This is important for the initial view. You'll have to decide how to deal with a layout during animation (more specifically a resize). I was under the impression that propagating Layout to children was handled by the LayoutManager. Is that not the case? What should this override do? Is this still relevant now that the constructor doesn't take an initial view anymore? Sorry for the avalanche of questions, I'd like to understand the Layout mechanics a little better :-) https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack.h (right): https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.h:19: explicit ViewStack(std::unique_ptr<views::View> initial_view); On 2016/12/12 at 17:38:07, sky wrote: > Can you make the constructor take no args and instead consumers call Push when ready? I think it makes for a simpler API, with less code duplication (for example, you forgot the set_owned_by_client for the view passed to the constructor). Done, good point. To avoid the situation where the first view animates on top of an empty space, I added the |animate| param to Push(). This allows callers to push a view without the animation. https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.h:53: bool can_process_events_within_subtree_; On 2016/12/12 at 17:38:08, sky wrote: > Does the value of this always match that of whether an animation is running? Can it be removed and instead check the animations (less duplication means smaller possibility of getting things out of sync). It does, good catch! Done.
https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack.cc (right): https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:58: On 2016/12/12 17:38:07, sky wrote: > You should override Layout to resize the views. This is important for the > initial view. You'll have to decide how to deal with a layout during animation > (more specifically a resize). You are absolutely right. The only wrinkle here is if a layout happens during animation everything is snapped to the origin, which is why I was suggesting overriding Layout. https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack_unittest.cc (right): https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack_unittest.cc:47: view_stack_.reset(new ViewStack()); Use MakeUnique, or just make ViewStack a value mamember (not wrapped in a unique_ptr_). https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack_unittest.cc:63: } Please add assertions around bounds and visibility. https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack_unittest.cc:80: base::RunLoop().Run(); Do these tests set the animation time to 0? If they don't, they should.
https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack.cc (right): https://codereview.chromium.org/2528503002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack.cc:58: On 2016/12/12 at 20:30:56, sky wrote: > On 2016/12/12 17:38:07, sky wrote: > > You should override Layout to resize the views. This is important for the > > initial view. You'll have to decide how to deal with a layout during animation > > (more specifically a resize). > > You are absolutely right. The only wrinkle here is if a layout happens during animation everything is snapped to the origin, which is why I was suggesting overriding Layout. A yeah, makes sense. I added code that re-targets the animations if a Layout happens while views are being animated (and a unit test for it). To achieve that, I had to change the std::stack member of ViewStack to an std::vector (to be able to iterate on the views and find which ones are being animated). I'd be interested in knowing if there's an easier way of doing that but it doesn't seem like it from the BoundsAnimator interface. https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack_unittest.cc (right): https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack_unittest.cc:47: view_stack_.reset(new ViewStack()); On 2016/12/12 at 20:30:56, sky wrote: > Use MakeUnique, or just make ViewStack a value mamember (not wrapped in a unique_ptr_). Used MakeUnique here to be able to delete the view manually at a specific time in tests. Made it a value in the dialog itself. https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack_unittest.cc:63: } On 2016/12/12 at 20:30:56, sky wrote: > Please add assertions around bounds and visibility. Added some around bounds, let me know if you had anything else in mind. I was trying to avoid testing BoundsAnimator itself here but some of it is probably a good idea. When you say visibility, are there some functions related to that or is it just the bounds stuff? https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack_unittest.cc:80: base::RunLoop().Run(); On 2016/12/12 at 20:30:56, sky wrote: > Do these tests set the animation time to 0? If they don't, they should. It looks like this makes the base::RunLoop().Run() call hang for some reason (even though IsAnimating() returns true). Am I doing something wrong? I set it to 1 in this patchset to illustrate.
https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/view_stack_unittest.cc (right): https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/view_stack_unittest.cc:80: base::RunLoop().Run(); On 2016/12/12 22:20:48, anthonyvd wrote: > On 2016/12/12 at 20:30:56, sky wrote: > > Do these tests set the animation time to 0? If they don't, they should. > > It looks like this makes the base::RunLoop().Run() call hang for some reason > (even though IsAnimating() returns true). Am I doing something wrong? I set it > to 1 in this patchset to illustrate. That would seem to indicate your delegate isn't being called to quit the loop right? I'm not sure why that would be. You'll have to investigate.
On 2016/12/12 at 23:57:00, sky wrote: > https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... > File chrome/browser/ui/views/payments/view_stack_unittest.cc (right): > > https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/payments/view_stack_unittest.cc:80: base::RunLoop().Run(); > On 2016/12/12 22:20:48, anthonyvd wrote: > > On 2016/12/12 at 20:30:56, sky wrote: > > > Do these tests set the animation time to 0? If they don't, they should. > > > > It looks like this makes the base::RunLoop().Run() call hang for some reason > > (even though IsAnimating() returns true). Am I doing something wrong? I set it > > to 1 in this patchset to illustrate. > > That would seem to indicate your delegate isn't being called to quit the loop right? I'm not sure why that would be. You'll have to investigate. Looking into this a bit more: the reason this happens is that calling AnimateViewTo when duration == 0 will end up directly calling delegate->AnimationEnded(). It's kind of a catch-22 because SetAnimationDelegate DCHECKs IsAnimating(), so AnimateViewTo has to be called first, but it's too late once it returns to call SetAnimationDelegate. I also tried overriding BoundsAnimator::CreateAnimation so that the TestAnimationDelegate was present from the beginning, which also failed (its AnimationEnded was still called before my base::RunLoop.Run(), which I imagine is the reason why). Ultimately, there aren't many places I can find in Chrome where tests deal with animations like this and none of the existing ones set the animation duration to 0. Even looking through the 4 calls to SetAnimationDuration in the codebase, none pass 0 and one callsite even passes 1 to "disable" the BoundsAnimator (https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_view.cc?rcl=14816...) which makes me think that passing 0 might not be very well supported for this type of use-case. Since I need to assert things between the animation's beginning and end, I suggest leaving the SetAnimationDuration(1) calls as-is. My understanding is that this would only require 1 "tick" before finishing the animation, making it functionally equivalent to what we'd want to achieve with SetAnimationDuration(0) while still allowing for assertions mid-animation. I confirmed that behavior locally with some debug logging. Sorry for the wall of text, let me know what you think!
On 2016/12/13 at 16:20:54, anthonyvd wrote: > On 2016/12/12 at 23:57:00, sky wrote: > > https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... > > File chrome/browser/ui/views/payments/view_stack_unittest.cc (right): > > > > https://codereview.chromium.org/2528503002/diff/180001/chrome/browser/ui/view... > > chrome/browser/ui/views/payments/view_stack_unittest.cc:80: base::RunLoop().Run(); > > On 2016/12/12 22:20:48, anthonyvd wrote: > > > On 2016/12/12 at 20:30:56, sky wrote: > > > > Do these tests set the animation time to 0? If they don't, they should. > > > > > > It looks like this makes the base::RunLoop().Run() call hang for some reason > > > (even though IsAnimating() returns true). Am I doing something wrong? I set it > > > to 1 in this patchset to illustrate. > > > > That would seem to indicate your delegate isn't being called to quit the loop right? I'm not sure why that would be. You'll have to investigate. > > Looking into this a bit more: the reason this happens is that calling AnimateViewTo when duration == 0 will end up directly calling delegate->AnimationEnded(). It's kind of a catch-22 because SetAnimationDelegate DCHECKs IsAnimating(), so AnimateViewTo has to be called first, but it's too late once it returns to call SetAnimationDelegate. > > I also tried overriding BoundsAnimator::CreateAnimation so that the TestAnimationDelegate was present from the beginning, which also failed (its AnimationEnded was still called before my base::RunLoop.Run(), which I imagine is the reason why). > Correction (dug a lot more): it's not that AnimationEnded is called to early, it's that it's never called. In fact, SlideAnimation::Start() is just never called when duration == 0 because of this early return in SlideAnimation::Show(): https://cs.chromium.org/chromium/src/ui/gfx/animation/slide_animation.cc?rcl=.... Because of this, the animation will skip all the way to the end state without going through the Start, Step, Stop steps and therefore not raise the associated AnimationProgressed and AnimationEnded events. The rest of this comment should still be valid and I stand by my suggestion to use SetAnimationDuration(1) to assert things in the intermediary state between "animation started" and "animation finished". > Ultimately, there aren't many places I can find in Chrome where tests deal with animations like this and none of the existing ones set the animation duration to 0. Even looking through the 4 calls to SetAnimationDuration in the codebase, none pass 0 and one callsite even passes 1 to "disable" the BoundsAnimator (https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_view.cc?rcl=14816...) which makes me think that passing 0 might not be very well supported for this type of use-case. > > Since I need to assert things between the animation's beginning and end, I suggest leaving the SetAnimationDuration(1) calls as-is. My understanding is that this would only require 1 "tick" before finishing the animation, making it functionally equivalent to what we'd want to achieve with SetAnimationDuration(0) while still allowing for assertions mid-animation. I confirmed that behavior locally with some debug logging. > > Sorry for the wall of text, let me know what you think!
A delay of 1 is fine by me. LGTM
On 2016/12/13 at 18:09:27, sky wrote: > A delay of 1 is fine by me. LGTM Thanks a ton for the thorough review! Elly is OOO, should I loop in someone else?
You might try kylixrd@. That said, I'm fine with you landing this now. Adjustments for harmony can be done later. -Scott On Tue, Dec 13, 2016 at 12:34 PM, <anthonyvd@chromium.org> wrote: > On 2016/12/13 at 18:09:27, sky wrote: >> A delay of 1 is fine by me. LGTM > > Thanks a ton for the thorough review! Elly is OOO, should I loop in someone > else? > > https://codereview.chromium.org/2528503002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/13 at 21:34:12, sky wrote: > You might try kylixrd@. That said, I'm fine with you landing this now. > Adjustments for harmony can be done later. > > -Scott > > On Tue, Dec 13, 2016 at 12:34 PM, <anthonyvd@chromium.org> wrote: > > On 2016/12/13 at 18:09:27, sky wrote: > >> A delay of 1 is fine by me. LGTM > > > > Thanks a ton for the thorough review! Elly is OOO, should I loop in someone > > else? > > > > https://codereview.chromium.org/2528503002/ > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. > > Excellent, this doesn't implement much actual UI anyways as the current views are placeholders. I'll include kylixrd@ or ellyjones@ as reviewers on subsequent CLs.
The CQ bit was checked by anthonyvd@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from krb@chromium.org, rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2528503002/#ps200001 (title: "Handle Layout while animating")
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": 200001, "attempt_start_ts": 1481682651134700,
"parent_rev": "8c6e2e7d2927888f42af2919023cc9da09792e5f", "commit_rev":
"6bfb6be36344d0b40cf480eb232512a2a594b468"}
Message was sent while issue was closed.
Description was changed from ========== [WebPayments] Implement state transitions in desktop WebPayments dialog. This CL implements a crude version of the state stack and sliding transitions in the desktop WebPayments dialog. The states do pretty much nothing except navigating between one another but this is intended as a skeleton to build all the screens from. In particular, this CL adds: 1- PaymentDialogState base class that all states inherit from 2- PaymentSheetState and OrderSummaryState, 2 implementations of PaymentDialogState 3- PushState() and PopState() functions on PaymentRequestDialog 4- Slide in/out animations when states are pushed and popped A document outlining this design is available at go/pr-desktop-ui BUG=667872 TEST= 1. Enable experimental javascript features 2. Visit a page that uses WebPayments and trigger the WebPayments flow 3. The dialog should display with the "Payment Sheet" open and an "Order Summary" button 4. Clicking "Order Summary" should slide in an "Order Summary" view from the right. It should contain a "Back" button 5. Clicking "Back" should slide the "Order Summary" view to the right and out of view, making "Payment Sheet" available again. ========== to ========== [WebPayments] Implement state transitions in desktop WebPayments dialog. This CL implements a crude version of the state stack and sliding transitions in the desktop WebPayments dialog. The states do pretty much nothing except navigating between one another but this is intended as a skeleton to build all the screens from. In particular, this CL adds: 1- PaymentDialogState base class that all states inherit from 2- PaymentSheetState and OrderSummaryState, 2 implementations of PaymentDialogState 3- PushState() and PopState() functions on PaymentRequestDialog 4- Slide in/out animations when states are pushed and popped A document outlining this design is available at go/pr-desktop-ui BUG=667872 TEST= 1. Enable experimental javascript features 2. Visit a page that uses WebPayments and trigger the WebPayments flow 3. The dialog should display with the "Payment Sheet" open and an "Order Summary" button 4. Clicking "Order Summary" should slide in an "Order Summary" view from the right. It should contain a "Back" button 5. Clicking "Back" should slide the "Order Summary" view to the right and out of view, making "Payment Sheet" available again. Review-Url: https://codereview.chromium.org/2528503002 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [WebPayments] Implement state transitions in desktop WebPayments dialog. This CL implements a crude version of the state stack and sliding transitions in the desktop WebPayments dialog. The states do pretty much nothing except navigating between one another but this is intended as a skeleton to build all the screens from. In particular, this CL adds: 1- PaymentDialogState base class that all states inherit from 2- PaymentSheetState and OrderSummaryState, 2 implementations of PaymentDialogState 3- PushState() and PopState() functions on PaymentRequestDialog 4- Slide in/out animations when states are pushed and popped A document outlining this design is available at go/pr-desktop-ui BUG=667872 TEST= 1. Enable experimental javascript features 2. Visit a page that uses WebPayments and trigger the WebPayments flow 3. The dialog should display with the "Payment Sheet" open and an "Order Summary" button 4. Clicking "Order Summary" should slide in an "Order Summary" view from the right. It should contain a "Back" button 5. Clicking "Back" should slide the "Order Summary" view to the right and out of view, making "Payment Sheet" available again. Review-Url: https://codereview.chromium.org/2528503002 ========== to ========== [WebPayments] Implement state transitions in desktop WebPayments dialog. This CL implements a crude version of the state stack and sliding transitions in the desktop WebPayments dialog. The states do pretty much nothing except navigating between one another but this is intended as a skeleton to build all the screens from. In particular, this CL adds: 1- PaymentDialogState base class that all states inherit from 2- PaymentSheetState and OrderSummaryState, 2 implementations of PaymentDialogState 3- PushState() and PopState() functions on PaymentRequestDialog 4- Slide in/out animations when states are pushed and popped A document outlining this design is available at go/pr-desktop-ui BUG=667872 TEST= 1. Enable experimental javascript features 2. Visit a page that uses WebPayments and trigger the WebPayments flow 3. The dialog should display with the "Payment Sheet" open and an "Order Summary" button 4. Clicking "Order Summary" should slide in an "Order Summary" view from the right. It should contain a "Back" button 5. Clicking "Back" should slide the "Order Summary" view to the right and out of view, making "Payment Sheet" available again. Committed: https://crrev.com/4069c4e1776b8ba44d3ddca2c2b4ce59a5fcdfc2 Cr-Commit-Position: refs/heads/master@{#438398} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/4069c4e1776b8ba44d3ddca2c2b4ce59a5fcdfc2 Cr-Commit-Position: refs/heads/master@{#438398} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
