Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2872623002:
[Web Payments] Add "pencil" edit button to lists. (Closed)
Description[Web Payments] Add "pencil" edit button to lists.
BUG=719478
Review-Url: https://codereview.chromium.org/2872623002
Cr-Commit-Position: refs/heads/master@{#471890}
Committed: https://chromium.googlesource.com/chromium/src/+/1e0e8bda501273ef5b6669370bed2999efdf0f85
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments. #
Total comments: 2
Patch Set 3 : Add comment #
Total comments: 2
Patch Set 4 : Move icon #Patch Set 5 : Add license #Patch Set 6 : Rebase and fix unit tests #Patch Set 7 : Address comments, icon changes #
Total comments: 9
Patch Set 8 : Move and rename icon. #
Total comments: 1
Patch Set 9 : Rebase #Patch Set 10 : Don't own the edit button #
Total comments: 10
Patch Set 11 : Address comments. #
Total comments: 2
Patch Set 12 : Rebase #Messages
Total messages: 51 (25 generated)
anthonyvd@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, PTAL at this CL that adds edit buttons!
Cool stuff, some comments! https://codereview.chromium.org/2872623002/diff/1/chrome/browser/ui/views/pay... File chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc (right): https://codereview.chromium.org/2872623002/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:110: AddCreditCard(card); nit: you can inline autofill::test::GetCreditCard() https://codereview.chromium.org/2872623002/diff/1/chrome/browser/ui/views/pay... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/payment_request_item_list.cc:39: constexpr int kBackArrowSize = 16; fix https://codereview.chromium.org/2872623002/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/payment_request_item_list.cc:119: I would find it easier to reason if we had CreateEditButton() called here? Why do it in the constructor? https://codereview.chromium.org/2872623002/diff/1/ui/vector_icons/BUILD.gn File ui/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2872623002/diff/1/ui/vector_icons/BUILD.gn#ne... ui/vector_icons/BUILD.gn:18: "edit_pen.icon", it's technically a pencil? :) Using a pen would mean you can't erase your changes haha
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...
anthonyvd@chromium.org changed reviewers: + estade@chromium.org
Thanks, comments addressed. +Evan, can you please do an OWNERs review of ui/vector_icons/*? Thanks! https://codereview.chromium.org/2872623002/diff/1/chrome/browser/ui/views/pay... File chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc (right): https://codereview.chromium.org/2872623002/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:110: AddCreditCard(card); On 2017/05/08 at 14:08:02, Mathieu wrote: > nit: you can inline autofill::test::GetCreditCard() Done. https://codereview.chromium.org/2872623002/diff/1/chrome/browser/ui/views/pay... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/payment_request_item_list.cc:39: constexpr int kBackArrowSize = 16; On 2017/05/08 at 14:08:02, Mathieu wrote: > fix Done. https://codereview.chromium.org/2872623002/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/payment_request_item_list.cc:119: On 2017/05/08 at 14:08:02, Mathieu wrote: > I would find it easier to reason if we had CreateEditButton() called here? Why do it in the constructor? This way avoided having to store the show_edit_button bool. Changed it since it's probably better to delay creating the button anyways. https://codereview.chromium.org/2872623002/diff/1/ui/vector_icons/BUILD.gn File ui/vector_icons/BUILD.gn (right): https://codereview.chromium.org/2872623002/diff/1/ui/vector_icons/BUILD.gn#ne... ui/vector_icons/BUILD.gn:18: "edit_pen.icon", On 2017/05/08 at 14:08:02, Mathieu wrote: > it's technically a pencil? :) > > Using a pen would mean you can't erase your changes haha Good point, done.
mathp@google.com changed reviewers: + mathp@google.com
lgtm thanks https://codereview.chromium.org/2872623002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_item_list.cc:105: constexpr int kExtraViewSpacing = 10; Nit: I would add a comment such as // This is the space between <view> and <view>. This way it's easier to read and eventually tweak
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2872623002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_item_list.cc:105: constexpr int kExtraViewSpacing = 10; On 2017/05/08 at 14:50:28, mathp wrote: > Nit: I would add a comment such as > > // This is the space between <view> and <view>. > > This way it's easier to read and eventually tweak Done.
Description was changed from ========== [Web Payments] Add "pencil" edit button to lists. BUG=719478 ========== to ========== [Web Payments] Add "pencil" edit button to lists. BUG=719478 ==========
mathp@chromium.org changed reviewers: - mathp@google.com
lgtm from the right account
what is the source for this pencil? https://codereview.chromium.org/2872623002/diff/40001/ui/vector_icons/edit_pe... File ui/vector_icons/edit_pencil.icon (right): https://codereview.chromium.org/2872623002/diff/40001/ui/vector_icons/edit_pe... ui/vector_icons/edit_pencil.icon:1: CANVAS_DIMENSIONS, 20, this file needs a license header and should be in chrome/app/vector_icons/
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...
anthonyvd@chromium.org changed reviewers: + mathp@google.com
Comments addressed. PTAL! Our UXer bbergher@, created this icon. https://codereview.chromium.org/2872623002/diff/40001/ui/vector_icons/edit_pe... File ui/vector_icons/edit_pencil.icon (right): https://codereview.chromium.org/2872623002/diff/40001/ui/vector_icons/edit_pe... ui/vector_icons/edit_pencil.icon:1: CANVAS_DIMENSIONS, 20, On 2017/05/08 at 19:32:54, Evan Stade wrote: > this file needs a license header and should be in chrome/app/vector_icons/ Both done. For my curiosity: what's the rule for icon location?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
On 2017/05/08 21:10:54, anthonyvd wrote: > Comments addressed. PTAL! > > Our UXer bbergher@, created this icon. I ask because if it's from go/icons it should use the name that go/icons gives it. And normally (but not always), for icons of this size we have a separate 1x and >1x definition to make sure pixel alignment is good, so I wonder if bbergher considered that but decided it wasn't necessary. > > https://codereview.chromium.org/2872623002/diff/40001/ui/vector_icons/edit_pe... > File ui/vector_icons/edit_pencil.icon (right): > > https://codereview.chromium.org/2872623002/diff/40001/ui/vector_icons/edit_pe... > ui/vector_icons/edit_pencil.icon:1: CANVAS_DIMENSIONS, 20, > On 2017/05/08 at 19:32:54, Evan Stade wrote: > > this file needs a license header and should be in chrome/app/vector_icons/ > > Both done. For my curiosity: what's the rule for icon location? Like raster assets, they belong in the most specific location they can live in. ui/vector_icons would generally only be for icons that are used from many different directories. This one is only used in chrome/browser so it goes in chrome/app/vector_icons (I'm not sure why it's chrome/app but that's where browser-specific resources live).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
On 2017/05/08 22:36:24, Evan Stade wrote: > On 2017/05/08 21:10:54, anthonyvd wrote: > > Comments addressed. PTAL! > > > > Our UXer bbergher@, created this icon. > > I ask because if it's from go/icons it should use the name that go/icons gives > it. And normally (but not always), for icons of this size we have a separate 1x > and >1x definition to make sure pixel alignment is good, so I wonder if bbergher > considered that but decided it wasn't necessary. > > > > > > https://codereview.chromium.org/2872623002/diff/40001/ui/vector_icons/edit_pe... > > File ui/vector_icons/edit_pencil.icon (right): > > > > > https://codereview.chromium.org/2872623002/diff/40001/ui/vector_icons/edit_pe... > > ui/vector_icons/edit_pencil.icon:1: CANVAS_DIMENSIONS, 20, > > On 2017/05/08 at 19:32:54, Evan Stade wrote: > > > this file needs a license header and should be in chrome/app/vector_icons/ > > > > Both done. For my curiosity: what's the rule for icon location? > > Like raster assets, they belong in the most specific location they can live in. > ui/vector_icons would generally only be for icons that are used from many > different directories. This one is only used in chrome/browser so it goes in > chrome/app/vector_icons (I'm not sure why it's chrome/app but that's where > browser-specific resources live). I talked to bettes@ and his take is that we don't need another icon for this. Anthony will update the CL with a new icon with a different size, though.
Updated the icon file and changed its name. estade@, can you PTAL? Thanks!
https://codereview.chromium.org/2872623002/diff/120001/chrome/app/vector_icon... File chrome/app/vector_icons/ic_edit.icon (right): https://codereview.chromium.org/2872623002/diff/120001/chrome/app/vector_icon... chrome/app/vector_icons/ic_edit.icon:2: // Use of this source code is governed by a BSD-style license that can be nit: omit "ic_" from the name, and it's probably better off in ui/vector_icons/ since it's a generic icon from go/icons (we don't always follow this rule but it's the one I prefer. https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_method_view_controller.cc (right): https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_method_view_controller.cc:75: /*on_edited=*/base::BindOnce( nit: is this the correct format for documenting parameters? I've only seen this style in Chrome: https://cs.chromium.org/chromium/src/ash/system/network/network_icon.cc?rcl=d... https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_item_list.cc:69: checkmark->set_owned_by_client(); why is this and the edit button owned by the client? https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_item_list.cc:120: edit_button_->SetSize(gfx::Size(kEditIconSize, kEditIconSize)); You should only call View::SetSize during layout. You can make the column below have a fixed size and pass 16 there.
Comments addressed. PTAL! Thanks :) https://codereview.chromium.org/2872623002/diff/120001/chrome/app/vector_icon... File chrome/app/vector_icons/ic_edit.icon (right): https://codereview.chromium.org/2872623002/diff/120001/chrome/app/vector_icon... chrome/app/vector_icons/ic_edit.icon:2: // Use of this source code is governed by a BSD-style license that can be On 2017/05/10 at 16:06:25, Evan Stade wrote: > nit: omit "ic_" from the name, and it's probably better off in ui/vector_icons/ since it's a generic icon from go/icons (we don't always follow this rule but it's the one I prefer. Done. https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_method_view_controller.cc (right): https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_method_view_controller.cc:75: /*on_edited=*/base::BindOnce( On 2017/05/10 at 16:06:25, Evan Stade wrote: > nit: is this the correct format for documenting parameters? I've only seen this style in Chrome: > > https://cs.chromium.org/chromium/src/ash/system/network/network_icon.cc?rcl=d... The style guide doesn't specify and there's other instances of this form in the code, like in metrics and net (https://cs.chromium.org/search/?q=%22%3D*/%22&type=cs). We use this form in payments code because it keeps the comment and the value as one blob (for tools) IIRC. https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_item_list.cc:69: checkmark->set_owned_by_client(); On 2017/05/10 at 16:06:25, Evan Stade wrote: > why is this and the edit button owned by the client? The relationship between views and their controllers in payments code was making views lifetime non-obvious. Because of this, our convention is to keep owned_by_client views in unique_ptr members when the controllers keep accessing the views after their creation. It helps us avoid use-after-free errors and makes lifetime clearer where it matters. In this case, the checkmark is made visible/not visible on user interaction and the edit button is used in a button pressed handler that handles presses from more than one button. https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_item_list.cc:120: edit_button_->SetSize(gfx::Size(kEditIconSize, kEditIconSize)); On 2017/05/10 at 16:06:25, Evan Stade wrote: > You should only call View::SetSize during layout. > > You can make the column below have a fixed size and pass 16 there. Good point, done.
estade@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_item_list.cc:69: checkmark->set_owned_by_client(); On 2017/05/10 16:39:26, anthonyvd wrote: > On 2017/05/10 at 16:06:25, Evan Stade wrote: > > why is this and the edit button owned by the client? > > The relationship between views and their controllers in payments code was making > views lifetime non-obvious. Because of this, our convention is to keep > owned_by_client views in unique_ptr members when the controllers keep accessing > the views after their creation. It helps us avoid use-after-free errors and > makes lifetime clearer where it matters. > > In this case, the checkmark is made visible/not visible on user interaction and > the edit button is used in a button pressed handler that handles presses from > more than one button. The convention is different for every other piece of UI code in Chrome. set_owned_by_client should be a rare exception. +sky to weigh in
https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_item_list.h (right): https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_item_list.h:110: std::unique_ptr<views::ImageButton> edit_button_; I think a better approach here is to not have the member at all. Instead use a tag or id on the button to identify in the listener.
On 2017/05/12 01:28:49, sky wrote: > https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... > File chrome/browser/ui/views/payments/payment_request_item_list.h (right): > > https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... > chrome/browser/ui/views/payments/payment_request_item_list.h:110: > std::unique_ptr<views::ImageButton> edit_button_; > I think a better approach here is to not have the member at all. Instead use a > tag or id on the button to identify in the listener. Anthony, under what conditions do you have views outliving the controller? It's pretty rare to do that as generally the controller is tied to the life of the view.
On 2017/05/12 at 01:36:14, sky wrote: > On 2017/05/12 01:28:49, sky wrote: > > https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... > > File chrome/browser/ui/views/payments/payment_request_item_list.h (right): > > > > https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... > > chrome/browser/ui/views/payments/payment_request_item_list.h:110: > > std::unique_ptr<views::ImageButton> edit_button_; > > I think a better approach here is to not have the member at all. Instead use a > > tag or id on the button to identify in the listener. > > Anthony, under what conditions do you have views outliving the controller? It's pretty rare to do that as generally the controller is tied to the life of the view. Most of the time, the view barely outlives the controller because we destroy the controller when the view it's associated with leaves the hierarchy (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/payment...). Those views are owned by ViewStack (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/view_st...). In the case of things like the edit_button_ here, it's created at the same time as the view that goes on the view stack but it might be replaced at some point when the view updates its contents (because something in PaymentRequest or from a User action triggered it). In this case, we only compare this button to the ButtonPressed sender but there are other places where we use this pattern to hold on to certain pieces of UI and update them after certain user/website actions and triggers. To achieve this, we saw 3 choices: 1/ Keep raw pointers to the views we were interested in keeping a handle on. While this would be fine, we saw the potential for memory errors if a pointer wasn't properly kept up to date. We also realized it made it really hard to reason about views/controllers lifetime, especially for some teammates who weren't familiar with view but still had to write/review this code. 2/ Set IDs on views we wanted to find later to update them. We felt that traversing the hierarchy to find a view every time we needed to update it was unnecessarily wasteful. This would also litter the code with View* v = GetViewById(id); if (v) { /* do something */ }, which is minor but we avoid in (3/). 3/ Keep the views we're interested in as owned_by_client std::unique_ptr<views::View>. We felt this eliminated potential error cases while making lifetime way clearer and avoiding full lookup every time a view was needed. Obviously we weren't aware that set_owned_by_client was discouraged (maybe we should add this to its comment in view.h). Sorry for the wall of text, hopefully this gives you some more context. If you still fell like this should be changed throughout payments UI code, I'll file a bug for it since this goes way beyond the scope of this CL.
On 2017/05/12 02:03:12, anthonyvd wrote: > On 2017/05/12 at 01:36:14, sky wrote: > > On 2017/05/12 01:28:49, sky wrote: > > > > https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... > > > File chrome/browser/ui/views/payments/payment_request_item_list.h (right): > > > > > > > https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... > > > chrome/browser/ui/views/payments/payment_request_item_list.h:110: > > > std::unique_ptr<views::ImageButton> edit_button_; > > > I think a better approach here is to not have the member at all. Instead use > a > > > tag or id on the button to identify in the listener. > > > > Anthony, under what conditions do you have views outliving the controller? > It's pretty rare to do that as generally the controller is tied to the life of > the view. > > Most of the time, the view barely outlives the controller because we destroy the > controller when the view it's associated with leaves the hierarchy > (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/payment...). > Those views are owned by ViewStack > (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/view_st...). > > In the case of things like the edit_button_ here, it's created at the same time > as the view that goes on the view stack but it might be replaced at some point > when the view updates its contents (because something in PaymentRequest or from > a User action triggered it). In this case, we only compare this button to the > ButtonPressed sender but there are other places where we use this pattern to > hold on to certain pieces of UI and update them after certain user/website > actions and triggers. To achieve this, we saw 3 choices: It seems unusual to me that the object which is creating and tracking views (PaymentRequestItemList) is not itself a View, yet also not a controller (controllers don't involve themselves with whatever view toolkit implements the presentation). > > 1/ Keep raw pointers to the views we were interested in keeping a handle on. > While this would be fine, we saw the potential for memory errors if a pointer > wasn't properly kept up to date. This sounds to me like it still has the potential to cause errors, just not crashing ones. Instead of reference after free, you'll be referencing (updating) something that's no longer in the view hierarchy. Isn't that a bug as well? > Sorry for the wall of text, hopefully this gives you some more context. If you > still fell like this should be changed throughout payments UI code, I'll file a > bug for it since this goes way beyond the scope of this CL. I brought this up because you were adding a new instance. It's indeed a bridge too far to change the rest of the payments UI code in this CL.
On 2017/05/12 16:39:50, Evan Stade wrote: > On 2017/05/12 02:03:12, anthonyvd wrote: > > On 2017/05/12 at 01:36:14, sky wrote: > > > On 2017/05/12 01:28:49, sky wrote: > > > > > > > https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... > > > > File chrome/browser/ui/views/payments/payment_request_item_list.h (right): > > > > > > > > > > > https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... > > > > chrome/browser/ui/views/payments/payment_request_item_list.h:110: > > > > std::unique_ptr<views::ImageButton> edit_button_; > > > > I think a better approach here is to not have the member at all. Instead > use > > a > > > > tag or id on the button to identify in the listener. > > > > > > Anthony, under what conditions do you have views outliving the controller? > > It's pretty rare to do that as generally the controller is tied to the life of > > the view. > > > > Most of the time, the view barely outlives the controller because we destroy > the > > controller when the view it's associated with leaves the hierarchy > > > (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/payment...). > > Those views are owned by ViewStack > > > (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/view_st...). > > > > In the case of things like the edit_button_ here, it's created at the same > time > > as the view that goes on the view stack but it might be replaced at some point > > when the view updates its contents (because something in PaymentRequest or > from > > a User action triggered it). In this case, we only compare this button to the > > ButtonPressed sender but there are other places where we use this pattern to > > hold on to certain pieces of UI and update them after certain user/website > > actions and triggers. To achieve this, we saw 3 choices: > > It seems unusual to me that the object which is creating and tracking views > (PaymentRequestItemList) is not itself a View, yet also not a controller > (controllers don't involve themselves with whatever view toolkit implements the > presentation). Isn't the core of the problem that PaymentRequestItemList needs to update state in views at some indeterminate point in the future? That problem can happen regardless of whether PaymentRequestItemList. Seems like a better solution is to have the lifetime of the UI and the object responsible for updating the UI be the same. Is that possible Anthony? I don't think this discussion should hold up this review though. Anthony, how about removing the button as a member and using a tag and moving this discussion to email? > > > > > 1/ Keep raw pointers to the views we were interested in keeping a handle on. > > While this would be fine, we saw the potential for memory errors if a pointer > > wasn't properly kept up to date. > > This sounds to me like it still has the potential to cause errors, just not > crashing ones. Instead of reference after free, you'll be referencing (updating) > something that's no longer in the view hierarchy. Isn't that a bug as well? > > > Sorry for the wall of text, hopefully this gives you some more context. If you > > still fell like this should be changed throughout payments UI code, I'll file > a > > bug for it since this goes way beyond the scope of this CL. > > I brought this up because you were adding a new instance. It's indeed a bridge > too far to change the rest of the payments UI code in this CL.
The latest patch removes the edit button ownership. PTAL. On 2017/05/12 at 17:25:05, sky wrote: > On 2017/05/12 16:39:50, Evan Stade wrote: > > On 2017/05/12 02:03:12, anthonyvd wrote: > > > On 2017/05/12 at 01:36:14, sky wrote: > > > > On 2017/05/12 01:28:49, sky wrote: > > > > > > > > > > https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... > > > > > File chrome/browser/ui/views/payments/payment_request_item_list.h (right): > > > > > > > > > > > > > > > https://codereview.chromium.org/2872623002/diff/140001/chrome/browser/ui/view... > > > > > chrome/browser/ui/views/payments/payment_request_item_list.h:110: > > > > > std::unique_ptr<views::ImageButton> edit_button_; > > > > > I think a better approach here is to not have the member at all. Instead > > use > > > a > > > > > tag or id on the button to identify in the listener. > > > > > > > > Anthony, under what conditions do you have views outliving the controller? > > > It's pretty rare to do that as generally the controller is tied to the life of > > > the view. > > > > > > Most of the time, the view barely outlives the controller because we destroy > > the > > > controller when the view it's associated with leaves the hierarchy > > > > > (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/payment...). > > > Those views are owned by ViewStack > > > > > (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/view_st...). > > > > > > In the case of things like the edit_button_ here, it's created at the same > > time > > > as the view that goes on the view stack but it might be replaced at some point > > > when the view updates its contents (because something in PaymentRequest or > > from > > > a User action triggered it). In this case, we only compare this button to the > > > ButtonPressed sender but there are other places where we use this pattern to > > > hold on to certain pieces of UI and update them after certain user/website > > > actions and triggers. To achieve this, we saw 3 choices: > > > > It seems unusual to me that the object which is creating and tracking views > > (PaymentRequestItemList) is not itself a View, yet also not a controller > > (controllers don't involve themselves with whatever view toolkit implements the > > presentation). > > Isn't the core of the problem that PaymentRequestItemList needs to update state in views at some indeterminate point in the future? That problem can happen regardless of whether PaymentRequestItemList. Seems like a better solution is to have the lifetime of the UI and the object responsible for updating the UI be the same. Is that possible Anthony? > > I don't think this discussion should hold up this review though. Anthony, how about removing the button as a member and using a tag and moving this discussion to email? SGTM. Let me take your and Evan's questions/comments to an email to get that going. > > > > > > > > > 1/ Keep raw pointers to the views we were interested in keeping a handle on. > > > While this would be fine, we saw the potential for memory errors if a pointer > > > wasn't properly kept up to date. > > > > This sounds to me like it still has the potential to cause errors, just not > > crashing ones. Instead of reference after free, you'll be referencing (updating) > > something that's no longer in the view hierarchy. Isn't that a bug as well? > > > > > Sorry for the wall of text, hopefully this gives you some more context. If you > > > still fell like this should be changed throughout payments UI code, I'll file > > a > > > bug for it since this goes way beyond the scope of this CL. > > > > I brought this up because you were adding a new instance. It's indeed a bridge > > too far to change the rest of the payments UI code in this CL.
https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_item_list.cc:148: if (sender->id() == static_cast<int>(DialogViewID::EDIT_ITEM_BUTTON)) { nit: are both of these casts necessary? (here and L139) https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/profile_list_view_controller.cc (right): https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/profile_list_view_controller.cc:43: class ProfileItem : public PaymentRequestItemList::Item { while I'm poking around (and this doesn't have to be addressed right now)... It looks like this class is just a thunk that passes through almost all overrides of PaymentRequestItemList::Item methods to ShippingProfileViewController. It seems like what you want is PaymentRequestItemList::Item to define a delegate interface that ShippingProfileViewController implements, rather than having this subclass that redirects the calls. https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/profile_list_view_controller.cc:100: ProfileListViewController* parent_view_; nit: confusing that the controller is named parent_view_ https://codereview.chromium.org/2872623002/diff/180001/ui/vector_icons/edit.icon File ui/vector_icons/edit.icon (right): https://codereview.chromium.org/2872623002/diff/180001/ui/vector_icons/edit.i... ui/vector_icons/edit.icon:5: CANVAS_DIMENSIONS, 16, sorry, forgot to mention this. Generic icons from go/icons are added at a default size of 48x48. Individual UI surfaces that want to scale to a certain size (like this one) then provide the icon size.
Comments addressed, icon file updated. PTAL. https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_item_list.cc:148: if (sender->id() == static_cast<int>(DialogViewID::EDIT_ITEM_BUTTON)) { On 2017/05/12 at 18:48:07, Evan Stade wrote: > nit: are both of these casts necessary? (here and L139) Yeah, since DialogViewID is an enum class not casting results in error: invalid operands to binary expression ('int' and 'payments::DialogViewID') https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/profile_list_view_controller.cc (right): https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/profile_list_view_controller.cc:43: class ProfileItem : public PaymentRequestItemList::Item { On 2017/05/12 at 18:48:07, Evan Stade wrote: > while I'm poking around (and this doesn't have to be addressed right now)... > > It looks like this class is just a thunk that passes through almost all overrides of PaymentRequestItemList::Item methods to ShippingProfileViewController. It seems like what you want is PaymentRequestItemList::Item to define a delegate interface that ShippingProfileViewController implements, rather than having this subclass that redirects the calls. Ack. https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/profile_list_view_controller.cc:100: ProfileListViewController* parent_view_; On 2017/05/12 at 18:48:07, Evan Stade wrote: > nit: confusing that the controller is named parent_view_ Done. https://codereview.chromium.org/2872623002/diff/180001/ui/vector_icons/edit.icon File ui/vector_icons/edit.icon (right): https://codereview.chromium.org/2872623002/diff/180001/ui/vector_icons/edit.i... ui/vector_icons/edit.icon:5: CANVAS_DIMENSIONS, 16, On 2017/05/12 at 18:48:07, Evan Stade wrote: > sorry, forgot to mention this. Generic icons from go/icons are added at a default size of 48x48. Individual UI surfaces that want to scale to a certain size (like this one) then provide the icon size. Done.
vector icon lgtm https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_item_list.cc:148: if (sender->id() == static_cast<int>(DialogViewID::EDIT_ITEM_BUTTON)) { On 2017/05/12 20:16:48, anthonyvd wrote: > On 2017/05/12 at 18:48:07, Evan Stade wrote: > > nit: are both of these casts necessary? (here and L139) > > Yeah, since DialogViewID is an enum class not casting results in > > error: invalid operands to binary expression ('int' and > 'payments::DialogViewID') what error do you get for the other one? Because clang doesn't complain when I set_id with an enum value. https://codereview.chromium.org/2872623002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_method_view_controller.cc (right): https://codereview.chromium.org/2872623002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_method_view_controller.cc:74: case PaymentInstrument::Type::AUTOFILL: are you going to add other types soon? You might just leave this as a DCHECK_EQ for now to reduce indentation and improve readability
https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_item_list.cc (right): https://codereview.chromium.org/2872623002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_item_list.cc:148: if (sender->id() == static_cast<int>(DialogViewID::EDIT_ITEM_BUTTON)) { On 2017/05/15 at 17:14:55, Evan Stade wrote: > On 2017/05/12 20:16:48, anthonyvd wrote: > > On 2017/05/12 at 18:48:07, Evan Stade wrote: > > > nit: are both of these casts necessary? (here and L139) > > > > Yeah, since DialogViewID is an enum class not casting results in > > > > error: invalid operands to binary expression ('int' and > > 'payments::DialogViewID') > > what error do you get for the other one? error: cannot initialize a parameter of type 'int' with an rvalue of type 'payments::DialogViewID' > Because clang doesn't complain when I set_id with an enum value. Yeah, enum class conversion rules are stricter than plain enum's. https://codereview.chromium.org/2872623002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_method_view_controller.cc (right): https://codereview.chromium.org/2872623002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_method_view_controller.cc:74: case PaymentInstrument::Type::AUTOFILL: On 2017/05/15 at 17:14:55, Evan Stade wrote: > are you going to add other types soon? You might just leave this as a DCHECK_EQ for now to reduce indentation and improve readability Yes, web payment apps are in the pipeline and there's some other exploration going on as well :)
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@google.com, mathp@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2872623002/#ps220001 (title: "Rebase")
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": 220001, "attempt_start_ts": 1494869268213460,
"parent_rev": "92abf957cbc29e3d166b20a1f0b7d66b0e86a5dc", "commit_rev":
"1e0e8bda501273ef5b6669370bed2999efdf0f85"}
Message was sent while issue was closed.
Description was changed from ========== [Web Payments] Add "pencil" edit button to lists. BUG=719478 ========== to ========== [Web Payments] Add "pencil" edit button to lists. BUG=719478 Review-Url: https://codereview.chromium.org/2872623002 Cr-Commit-Position: refs/heads/master@{#471890} Committed: https://chromium.googlesource.com/chromium/src/+/1e0e8bda501273ef5b6669370bed... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/1e0e8bda501273ef5b6669370bed... |
