Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2579513002:
[WebPayments] Factor out sheet-specific logic in Controllers. (Closed)
Description[WebPayments] Factor out sheet-specific logic in Controllers.
This CL adds a mechanism to isolate sheet-specific code previously in
PaymentRequestDialog to Controller classes. This will allow for clearer
view creation and event handling as the different sheets in the dialog
are implemented.
BUG=667872
Committed: https://crrev.com/9380084b03aa5c232c96c32ff66e5b4317a36d54
Cr-Commit-Position: refs/heads/master@{#439141}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address comments. #
Total comments: 7
Patch Set 3 : Address comments. #
Total comments: 86
Patch Set 4 : Address comments. #
Total comments: 20
Patch Set 5 : Addressed Rouslan's comments. #Patch Set 6 : Add missing include and semicolon. #Messages
Total messages: 36 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
anthonyvd@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, This CL attempts to better separate the event handling and view creation for different PaymentRequest dialog states. I think it will allow us to avoid a monolithic PaymentRequestDialog class, while still not being different from everything in views:: like the ViewStackStates of my previous CL were. I'd really appreciate if you could take a look (especially since you have our back-and-forth in the previous CL for context) but please don't hesitate to suggest a different OWNER if I'm sending too much stuff your way. Thank you very much!
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...
https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:28: OrderSummaryViewController* controller); As this function takes an OrderSummaryViewController why not make it a member function and not static? https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:21: template<typename TController> optional: I don't think 'T' adds much here, Controller? https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:26: Remove newline? https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:29: std::unique_ptr<views::View> view = TController::CreateView(controller.get()); See comment in other file, I would change this to controller->CreateView() as it feels more natural than having a static function take a parameter of the type. https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:58: PaymentRequestDialog::~PaymentRequestDialog() {} If this is hit and controller_map_ is non-empty then the controllers are destroyed before the views. Not sure if it matters, but figure it's worth mentioning. https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:25: std::unique_ptr<views::View> PaymentSheetViewController::CreateView( Make declaration/definition order match.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:28: OrderSummaryViewController* controller); On 2016/12/14 at 23:44:25, sky wrote: > As this function takes an OrderSummaryViewController why not make it a member function and not static? Not sure what I was thinking! Done. https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:21: template<typename TController> On 2016/12/14 at 23:44:25, sky wrote: > optional: I don't think 'T' adds much here, Controller? Hold habit, done. https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:26: On 2016/12/14 at 23:44:25, sky wrote: > Remove newline? Done. https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:29: std::unique_ptr<views::View> view = TController::CreateView(controller.get()); On 2016/12/14 at 23:44:25, sky wrote: > See comment in other file, I would change this to controller->CreateView() as it feels more natural than having a static function take a parameter of the type. Done. https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:58: PaymentRequestDialog::~PaymentRequestDialog() {} On 2016/12/14 at 23:44:25, sky wrote: > If this is hit and controller_map_ is non-empty then the controllers are destroyed before the views. Not sure if it matters, but figure it's worth mentioning. Yep, that's on purpose for a couple reasons: 1- The opposite destruction order would make it so that the map is destroyed before the views remove themselves from the hierarchy, making ViewHierarchyChanged access a destroyed controller_map_ without some extra code. 2- As everything is getting teared down, there should be no events to handle from the controllers at this point whether the views still live or not. https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2579513002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:25: std::unique_ptr<views::View> PaymentSheetViewController::CreateView( On 2016/12/14 at 23:44:25, sky wrote: > Make declaration/definition order match. Done (here and in OrderSummaryViewController).
LGTM https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:25: ~OrderSummaryViewController() override {} Don't inline the destructor (I'm surprised you didn't get a warning). https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:27: std::unique_ptr<views::View> CreateView() override; Prefix with where override is from (like you have on line 30). https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_sheet_controller.h (right): https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:31: virtual std::unique_ptr<views::View> CreateView() = 0; Technically you don't need CreateView() defined in this class, but it does make things clearer. https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.h:27: std::unique_ptr<views::View> CreateView() override; Same comment about adding comment for where override is from.
anthonyvd@chromium.org changed reviewers: + rouslan@chromium.org, tmartino@chromium.org
Thanks, comments addressed! +Rouslan and Tommy, can you guys please take a look? Thanks! https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:25: ~OrderSummaryViewController() override {} On 2016/12/15 at 16:49:44, sky wrote: > Don't inline the destructor (I'm surprised you didn't get a warning). Done. https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:27: std::unique_ptr<views::View> CreateView() override; On 2016/12/15 at 16:49:44, sky wrote: > Prefix with where override is from (like you have on line 30). Done. https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.h:27: std::unique_ptr<views::View> CreateView() override; On 2016/12/15 at 16:49:44, sky wrote: > Same comment about adding comment for where override is from. Done.
Overall a great patch. Don't despair about the long list of comments from me. These are mostly the same comments over and over again, so we don't accidentally miss anything. TL;DR: IWYU and be clear about raw pointer ownership. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:31: views::View* content_view = new views::View; #include "ui/views/view.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:40: views::LabelButton* back_button = #include "ui/views/controls/button/label_button.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:42: this, base::ASCIIToUTF16("Back")); Don't forget to properly i18n this eventually. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:47: l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_ORDER_SUMMARY_TITLE), Drive-by: 1) Place this string in components/autofill_strings.grdp 2) Let Android use it via formatter_data="android_java" 3) Remove IDS_PAYMENTS_ORDER_SUMMARY_LABEL from android_chrome_strings.grdp. 4) Use R.string.payment_request_order_summary_title instead of R.string.payments_order_summary_label in PaymentRequestUI.java. 5) Optional: Use IDS_PAYMENT_REQUEST_ORDER_SUMMARY_TITLE instead of IDS_IOS_PAYMENT_REQUEST_PAYMENT_ITEMS_TITLE. It's OK to make these changes in a follow up patch. You don't need an Android build setup, because the change would be mechanical. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:53: DCHECK_EQ(kBackButtonTag, sender->tag()); #include "base/logging.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:9: no newline here necessary. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:13: class View; No need to forward-declare or #include in the header the types that are used only to override parent methods. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:23: OrderSummaryViewController(PaymentRequestImpl* impl, Forward declare PaymentRequestImpl. Add a comment about who owns the impl and dialog params. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:24: PaymentRequestDialog* dialog); Forward declare PaymentRequestDialog. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:34: DISALLOW_COPY_AND_ASSIGN(OrderSummaryViewController); #include "base/macros.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:7: #include "base/strings/utf_string_conversions.h" No longer used. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:10: #include "chrome/browser/ui/views/payments/payment_request_views_util.h" CreatePaymentView() is not used in here. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:17: #include "ui/views/layout/grid_layout.h" No longer used. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:25: payments::PaymentRequestDialog* dialog) { Add a comment about who owns the map, impl, and dialog params. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:27: base::MakeUnique<Controller>(impl, dialog); #include "base/memory/ptr_util.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:83: void PaymentRequestDialog::GoBack() { Please reorder methods in .cc to match their order in .h. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:97: DCHECK(!details.move_view); #include "base/logging.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:11: #include "ui/views/controls/button/button.h" no longer used. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:14: class ViewStack; Redundant with view_stack.h include. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:21: using ControllerMap = Who owns these View* pointers? https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:22: std::map<views::View*, std::unique_ptr<PaymentRequestSheetController>>; #include <memory> for unique_ptr. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:57: DISALLOW_COPY_AND_ASSIGN(PaymentRequestDialog); #include "base/macros.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_sheet_controller.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:25: PaymentRequestSheetController(PaymentRequestImpl* impl, Add a comment about ownership of impl and dialog params. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:33: // The PaymentRequestImpl object associated with this instance of the dialog. Pointer ownership comment. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:35: // The dialog that contains and owns this object. Pointer ownership comment. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:39: PaymentRequestImpl* impl_; Add comments about ownership of impl_ and dialog_ parameters. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_views_util.cc (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:7: #include "base/strings/utf_string_conversions.h" Not used. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:9: #include "ui/views/controls/button/md_text_button.h" Not used. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:11: #include "ui/views/layout/fill_layout.h" Not used. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:18: std::unique_ptr<views::View> view = base::MakeUnique<views::View>(); #include "base/memory/ptr_util.h" #inclued "ui/views/view.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:19: view->set_background(views::Background::CreateSolidBackground(SK_ColorWHITE)); #inclued "third_party/skia/include/core/SkColor.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_views_util.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.h:22: const base::string16& title, views::View* content_view); Who owns the content_view? https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:20: } You've placed "// namespace" comment in the other file. Please be consistent. I'm OK either way here. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:31: views::View* content_view = new views::View; #include "ui/views/view.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:40: views::LabelButton* order_summary_button = #include "ui/views/controls/button/label_button.h" https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:42: this, base::ASCIIToUTF16("Order Summary")); Don't forget to i18n this eventually. This is a button label, so the dialog section title should not be reused, as I understand. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:47: l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_PAYMENT_SHEET_TITLE), Drive-by: move this to autofill_strings.grdp. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.h:9: No need for a newline. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.h:13: class View; No need to forward-declare or #include in the header the types that are used only to override parent methods. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.h:23: PaymentSheetViewController(PaymentRequestImpl* impl, Forward declare PaymentRequestImpl and PaymentRequestDialog. Add a comment about ownership of impl and dialog parameters. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.h:34: DISALLOW_COPY_AND_ASSIGN(PaymentSheetViewController); #include "base/macros.h"
No despair here; I like thorough reviews :-) Comments addressed, thanks a ton! https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:31: views::View* content_view = new views::View; On 2016/12/15 at 19:11:14, rouslan wrote: > #include "ui/views/view.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:40: views::LabelButton* back_button = On 2016/12/15 at 19:11:14, rouslan wrote: > #include "ui/views/controls/button/label_button.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:42: this, base::ASCIIToUTF16("Back")); On 2016/12/15 at 19:11:14, rouslan wrote: > Don't forget to properly i18n this eventually. Absolutely, this is going to be an arrow icon ultimately. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:47: l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_ORDER_SUMMARY_TITLE), On 2016/12/15 at 19:11:14, rouslan wrote: > Drive-by: > > 1) Place this string in components/autofill_strings.grdp > > 2) Let Android use it via formatter_data="android_java" > > 3) Remove IDS_PAYMENTS_ORDER_SUMMARY_LABEL from android_chrome_strings.grdp. > > 4) Use R.string.payment_request_order_summary_title instead of R.string.payments_order_summary_label in PaymentRequestUI.java. > > 5) Optional: Use IDS_PAYMENT_REQUEST_ORDER_SUMMARY_TITLE instead of IDS_IOS_PAYMENT_REQUEST_PAYMENT_ITEMS_TITLE. > > It's OK to make these changes in a follow up patch. You don't need an Android build setup, because the change would be mechanical. Will do! https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:53: DCHECK_EQ(kBackButtonTag, sender->tag()); On 2016/12/15 at 19:11:14, rouslan wrote: > #include "base/logging.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:9: On 2016/12/15 at 19:11:14, rouslan wrote: > no newline here necessary. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:13: class View; On 2016/12/15 at 19:11:14, rouslan wrote: > No need to forward-declare or #include in the header the types that are used only to override parent methods. Remnants from a previous patch, good catch! Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:23: OrderSummaryViewController(PaymentRequestImpl* impl, On 2016/12/15 at 19:11:14, rouslan wrote: > Forward declare PaymentRequestImpl. > Add a comment about who owns the impl and dialog params. This is a similar situation than the previous comment about forward-declares for overrides. Should those two be forward-declared only because ctors aren't actually overrides? In any case, done. As for the ownership comment, added it to the base class. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:24: PaymentRequestDialog* dialog); On 2016/12/15 at 19:11:14, rouslan wrote: > Forward declare PaymentRequestDialog. See previous comment, done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:34: DISALLOW_COPY_AND_ASSIGN(OrderSummaryViewController); On 2016/12/15 at 19:11:14, rouslan wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:7: #include "base/strings/utf_string_conversions.h" On 2016/12/15 at 19:11:14, rouslan wrote: > No longer used. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:10: #include "chrome/browser/ui/views/payments/payment_request_views_util.h" On 2016/12/15 at 19:11:14, rouslan wrote: > CreatePaymentView() is not used in here. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:17: #include "ui/views/layout/grid_layout.h" On 2016/12/15 at 19:11:15, rouslan wrote: > No longer used. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:25: payments::PaymentRequestDialog* dialog) { On 2016/12/15 at 19:11:15, rouslan wrote: > Add a comment about who owns the map, impl, and dialog params. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:27: base::MakeUnique<Controller>(impl, dialog); On 2016/12/15 at 19:11:15, rouslan wrote: > #include "base/memory/ptr_util.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:83: void PaymentRequestDialog::GoBack() { On 2016/12/15 at 19:11:14, rouslan wrote: > Please reorder methods in .cc to match their order in .h. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:97: DCHECK(!details.move_view); On 2016/12/15 at 19:11:14, rouslan wrote: > #include "base/logging.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:11: #include "ui/views/controls/button/button.h" On 2016/12/15 at 19:11:15, rouslan wrote: > no longer used. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:21: using ControllerMap = On 2016/12/15 at 19:11:15, rouslan wrote: > Who owns these View* pointers? The ViewStack does, added a comment. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:22: std::map<views::View*, std::unique_ptr<PaymentRequestSheetController>>; On 2016/12/15 at 19:11:15, rouslan wrote: > #include <memory> for unique_ptr. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:57: DISALLOW_COPY_AND_ASSIGN(PaymentRequestDialog); On 2016/12/15 at 19:11:15, rouslan wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_sheet_controller.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:25: PaymentRequestSheetController(PaymentRequestImpl* impl, On 2016/12/15 at 19:11:15, rouslan wrote: > Add a comment about ownership of impl and dialog params. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:33: // The PaymentRequestImpl object associated with this instance of the dialog. On 2016/12/15 at 19:11:15, rouslan wrote: > Pointer ownership comment. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:35: // The dialog that contains and owns this object. On 2016/12/15 at 19:11:15, rouslan wrote: > Pointer ownership comment. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:39: PaymentRequestImpl* impl_; On 2016/12/15 at 19:11:15, rouslan wrote: > Add comments about ownership of impl_ and dialog_ parameters. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_views_util.cc (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:7: #include "base/strings/utf_string_conversions.h" On 2016/12/15 at 19:11:15, rouslan wrote: > Not used. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:9: #include "ui/views/controls/button/md_text_button.h" On 2016/12/15 at 19:11:15, rouslan wrote: > Not used. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:11: #include "ui/views/layout/fill_layout.h" On 2016/12/15 at 19:11:15, rouslan wrote: > Not used. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:18: std::unique_ptr<views::View> view = base::MakeUnique<views::View>(); On 2016/12/15 at 19:11:15, rouslan wrote: > #include "base/memory/ptr_util.h" > #inclued "ui/views/view.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:19: view->set_background(views::Background::CreateSolidBackground(SK_ColorWHITE)); On 2016/12/15 at 19:11:15, rouslan wrote: > #inclued "third_party/skia/include/core/SkColor.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_views_util.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.h:22: const base::string16& title, views::View* content_view); On 2016/12/15 at 19:11:15, rouslan wrote: > Who owns the content_view? The returned view does. Made that clearer with a comment and by taking |content_view| as a unique_ptr. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:20: } On 2016/12/15 at 19:11:15, rouslan wrote: > You've placed "// namespace" comment in the other file. Please be consistent. I'm OK either way here. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:31: views::View* content_view = new views::View; On 2016/12/15 at 19:11:15, rouslan wrote: > #include "ui/views/view.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:40: views::LabelButton* order_summary_button = On 2016/12/15 at 19:11:15, rouslan wrote: > #include "ui/views/controls/button/label_button.h" Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:42: this, base::ASCIIToUTF16("Order Summary")); On 2016/12/15 at 19:11:15, rouslan wrote: > Don't forget to i18n this eventually. This is a button label, so the dialog section title should not be reused, as I understand. Yep, this will eventually be replaced by the entire "Order Summary" section of the Payment Sheet. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:47: l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_PAYMENT_SHEET_TITLE), On 2016/12/15 at 19:11:15, rouslan wrote: > Drive-by: move this to autofill_strings.grdp. Will do. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.h:9: On 2016/12/15 at 19:11:15, rouslan wrote: > No need for a newline. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.h:13: class View; On 2016/12/15 at 19:11:16, rouslan wrote: > No need to forward-declare or #include in the header the types that are used only to override parent methods. Done. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.h:23: PaymentSheetViewController(PaymentRequestImpl* impl, On 2016/12/15 at 19:11:15, rouslan wrote: > Forward declare PaymentRequestImpl and PaymentRequestDialog. Add a comment about ownership of impl and dialog parameters. Forward declare: done. Added ownership comment to base class (which this ctor only forwards to). https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.h:34: DISALLOW_COPY_AND_ASSIGN(PaymentSheetViewController); On 2016/12/15 at 19:11:16, rouslan wrote: > #include "base/macros.h" Done.
Meta comment: If you're passing ownership of data, then use std::unique_ptr<>.
Memory management comments are not necessary when using that.
Raw pointers require careful management. Here are three rules of thumb for
comments:
I.
// Does not take ownership of parameters.
// The parameters should outlive this object.
// Parameters should not be null.
SomeObject(DataType* a, DataType* b);
Meaning: ~SomeObject() does not delete |a| and |b|. These pointers are always
non-null and valid. This makes your peer's life easier, because they don't have
to constantly check |a| and |b| for nullptr or fear that they have been deleted
from under their feet.
If you say "should not be null" for some parameters, make that more concrete by
asserting that in code as well. For example:
SomeObject::SomeObject(DataType* a, DataType* b) : a_(a), b_(b) {
DCHECK(a_);
DCHECK(b_);
}
Note that I'm dchecking my own copies instead of the original pointers. This is
to be consistent with constructors that take ownership of their parameters.
After a_(std::move(a)) has been executed, |a| is no longer a valid pointer, so
|a_| should be DCHECK-ed instead.
II.
// Caller should not take ownership of the result.
DataType* a() { return a_; }
Meaning: The caller can do some_object->a()->SomeOperation(), but should not do
std::unique_ptr<DataType> this_is_mine_now_mwahaha(some_object->a()).
III.
// Not owned. Outlives this object.
DataType* a_;
Same meaning as I.
https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/order_summary_view_controller.h (right):
https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/order_summary_view_controller.h:23:
OrderSummaryViewController(PaymentRequestImpl* impl,
On 2016/12/15 19:55:06, anthonyvd wrote:
> On 2016/12/15 at 19:11:14, rouslan wrote:
> > Forward declare PaymentRequestImpl.
> > Add a comment about who owns the impl and dialog params.
>
> This is a similar situation than the previous comment about forward-declares
for
> overrides. Should those two be forward-declared only because ctors aren't
> actually overrides?
Exactly.
> In any case, done.
Thx.
> As for the ownership comment, added it to the base class.
Would be nice to have it in the child class as well. When I'm reading this file
on its own, I would have to first look into .cc and then into
payment_request_sheet_controller.h to figure out what happens to the two
pointers. The comment does not need to be fancy or long. This is usually
sufficient:
// Does not take ownership of the arguments, which should outlive this object.
https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog.h (right):
https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog.h:14: class ViewStack;
On 2016/12/15 19:11:15, rouslan wrote:
> Redundant with view_stack.h include.
Missed one.
https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.h (right):
https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.h:22: const
base::string16& title, views::View* content_view);
On 2016/12/15 19:55:06, anthonyvd wrote:
> On 2016/12/15 at 19:11:15, rouslan wrote:
> > Who owns the content_view?
>
> The returned view does. Made that clearer with a comment and by taking
> |content_view| as a unique_ptr.
This is great, thank you.
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right):
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/order_summary_view_controller.cc:34:
std::unique_ptr<views::View> content_view = base::MakeUnique<views::View>();
#include <memory>
(for std::unique_ptr<>)
#include "base/memory/ptr_util.h"
(for base::MakeUnique<>)
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/order_summary_view_controller.cc:51:
std::move(content_view));
#include <utility>
(for std::move())
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_dialog.cc (right):
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_dialog.cc:32:
(*map)[view.get()] = std::move(controller);
#inclued <utility>
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.h
(right):
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:25: //
Objects of this class are owned by |dialog|, so it's a weak pointer that
s/weak pointer/non-owned pointer/
This avoids confusion with
https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:37: // The
pointer is owned by PaymentRequestFactory and will outlive this object.
All ownership comments should be from the point of the caller of this method.
The caller of this method has a reference to PaymentRequestSheetController and
may not know anything about PaymentRequestFactory. What the caller needs to know
is whether they should be taking ownership of this pointer. Here the answer is
"no". So the best comment style is something like this:
// Caller should not take ownership of the result.
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:41: // The
return value is owned by its parent view.
Ditto
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:45: // Owned
by PaymentRequestFactory and will outlive this object.
Let's restrict the private member variable comments to the scope of this file.
No need to mention the factory.
// Not owned. Will outlive this.
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:48: // Owned
by its parent view.
// Not owned. Will outlive this.
Is that correct to say?
Given this comment, your peers working on .cc file will assume that dialog_ is
never null and always valid. This will make their lives easier.
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:33:
std::unique_ptr<views::View> content_view = base::MakeUnique<views::View>();
<memory>
ptr_util.h
https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:50:
std::move(content_view));
<utility>
Thanks again, addressed all comments! Meta discussion regarding ownership comments below. The style guide is pretty clear about pointer semantics (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...), in a nutshell: - Raw pointers are not owned. - Ownership, along with its transfer, is expressed through std::unique_ptr. In that context, comments like "// Not owned" or "// Callers should not take ownership" are IMHO a bit redundant, especially since they do nothing to enforce any sort of ownership semantics. My opinion is that comments about null-ness (like you describe in I.) are useful and I added one in the latest patchset. As for the others, I would propose using ownership-related comments wherever semantics aren't obviously in line with what the style-guide covers and omit them where they do. Obviously that's just a suggestion and consistency across our team's code will trump anyone's personal preference. WDYT? https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.h:23: OrderSummaryViewController(PaymentRequestImpl* impl, On 2016/12/15 at 20:29:33, rouslan wrote: > On 2016/12/15 19:55:06, anthonyvd wrote: > > On 2016/12/15 at 19:11:14, rouslan wrote: > > > Forward declare PaymentRequestImpl. > > > Add a comment about who owns the impl and dialog params. > > > > This is a similar situation than the previous comment about forward-declares for > > overrides. Should those two be forward-declared only because ctors aren't > > actually overrides? > > Exactly. > > > In any case, done. > > Thx. > > > As for the ownership comment, added it to the base class. > > Would be nice to have it in the child class as well. When I'm reading this file on its own, I would have to first look into .cc and then into payment_request_sheet_controller.h to figure out what happens to the two pointers. The comment does not need to be fancy or long. This is usually sufficient: > > // Does not take ownership of the arguments, which should outlive this object. Fair point, added the shorter-form comment to the concrete implementations. https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.h (right): https://codereview.chromium.org/2579513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.h:14: class ViewStack; On 2016/12/15 at 20:29:33, rouslan wrote: > On 2016/12/15 19:11:15, rouslan wrote: > > Redundant with view_stack.h include. > > Missed one. For some reason codereview didn't show this comment (had to unfold with "show context"). Thanks, done. https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/order_summary_view_controller.cc:34: std::unique_ptr<views::View> content_view = base::MakeUnique<views::View>(); On 2016/12/15 at 20:29:33, rouslan wrote: > #include <memory> > > (for std::unique_ptr<>) > > > #include "base/memory/ptr_util.h" > > (for base::MakeUnique<>) Done. https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/order_summary_view_controller.cc:51: std::move(content_view)); On 2016/12/15 at 20:29:33, rouslan wrote: > #include <utility> > > (for std::move()) Done. https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_dialog.cc:32: (*map)[view.get()] = std::move(controller); On 2016/12/15 at 20:29:33, rouslan wrote: > #inclued <utility> Done. https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_sheet_controller.h (right): https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:25: // Objects of this class are owned by |dialog|, so it's a weak pointer that On 2016/12/15 at 20:29:33, rouslan wrote: > s/weak pointer/non-owned pointer/ > > This avoids confusion with https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h Good point, done. https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:37: // The pointer is owned by PaymentRequestFactory and will outlive this object. On 2016/12/15 at 20:29:33, rouslan wrote: > All ownership comments should be from the point of the caller of this method. The caller of this method has a reference to PaymentRequestSheetController and may not know anything about PaymentRequestFactory. What the caller needs to know is whether they should be taking ownership of this pointer. Here the answer is "no". So the best comment style is something like this: > > // Caller should not take ownership of the result. Done, see meta comment for some discussion. https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:41: // The return value is owned by its parent view. On 2016/12/15 at 20:29:33, rouslan wrote: > Ditto Done. https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:45: // Owned by PaymentRequestFactory and will outlive this object. On 2016/12/15 at 20:29:33, rouslan wrote: > Let's restrict the private member variable comments to the scope of this file. No need to mention the factory. > > // Not owned. Will outlive this. Done. https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_sheet_controller.h:48: // Owned by its parent view. On 2016/12/15 at 20:29:33, rouslan wrote: > // Not owned. Will outlive this. > > Is that correct to say? > > Given this comment, your peers working on .cc file will assume that dialog_ is never null and always valid. This will make their lives easier. It is. Done. https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:33: std::unique_ptr<views::View> content_view = base::MakeUnique<views::View>(); On 2016/12/15 at 20:29:33, rouslan wrote: > <memory> > ptr_util.h Done. https://codereview.chromium.org/2579513002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:50: std::move(content_view)); On 2016/12/15 at 20:29:33, rouslan wrote: > <utility> Done.
LGTM The updated style guide makes sense. Thank you for pointing me to it. I have not seen that part before. Given the guide, feel free to use your own judgement for whether to be explicit in comments about raw pointer ownership.
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2579513002/#ps120001 (title: "Addressed Rouslan's comments.")
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 anthonyvd@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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 rouslan@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2579513002/#ps140001 (title: "Add missing include and semicolon.")
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": 140001, "attempt_start_ts": 1481909237908110,
"parent_rev": "87d3683bb4a18da8144df6276c7b7cba7f3cd77a", "commit_rev":
"1539e5820efa613ed726eb0842b1499af3fef547"}
Message was sent while issue was closed.
Description was changed from ========== [WebPayments] Factor out sheet-specific logic in Controllers. This CL adds a mechanism to isolate sheet-specific code previously in PaymentRequestDialog to Controller classes. This will allow for clearer view creation and event handling as the different sheets in the dialog are implemented. BUG=667872 ========== to ========== [WebPayments] Factor out sheet-specific logic in Controllers. This CL adds a mechanism to isolate sheet-specific code previously in PaymentRequestDialog to Controller classes. This will allow for clearer view creation and event handling as the different sheets in the dialog are implemented. BUG=667872 Review-Url: https://codereview.chromium.org/2579513002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [WebPayments] Factor out sheet-specific logic in Controllers. This CL adds a mechanism to isolate sheet-specific code previously in PaymentRequestDialog to Controller classes. This will allow for clearer view creation and event handling as the different sheets in the dialog are implemented. BUG=667872 Review-Url: https://codereview.chromium.org/2579513002 ========== to ========== [WebPayments] Factor out sheet-specific logic in Controllers. This CL adds a mechanism to isolate sheet-specific code previously in PaymentRequestDialog to Controller classes. This will allow for clearer view creation and event handling as the different sheets in the dialog are implemented. BUG=667872 Committed: https://crrev.com/9380084b03aa5c232c96c32ff66e5b4317a36d54 Cr-Commit-Position: refs/heads/master@{#439141} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9380084b03aa5c232c96c32ff66e5b4317a36d54 Cr-Commit-Position: refs/heads/master@{#439141} |
