[Web Payments] Refactor sheet display to allow updating view content

[Web Payments] Refactor sheet display to allow updating view content

BUG=703302
Review-Url: https://codereview.chromium.org/2768133002
Cr-Commit-Position: refs/heads/master@{#459438}
Committed: https://chromium.googlesource.com/chromium/src/+/5807cbec8e12dc0181f62c69221751486f8176ff

[anthonyvd, 2017-03-22 21:21:15]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 21:22:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[anthonyvd, 2017-03-22 21:22:53]

anthonyvd@chromium.org changed reviewers:
+ mathp@chromium.org

[anthonyvd, 2017-03-22 21:22:53]

Hi Math,

Can you please take a look at this refactor that we discussed yesterday? I still
need to rebase when mad@'s change lands but I wanted to get the ball rolling in
case you had some bigger picture comments.

Thanks!

[commit-bot: I haz the power, 2017-03-22 22:12:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 22:12:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mathieu, 2017-03-23 11:36:46]

I really like it! It's neat because the view controllers don't even need access
to their views once they are created! Just call Update and voila ;)

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/order_summary_view_controller.h (right):

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/order_summary_view_controller.h:39:
views::Button* pay_button_;
nit: do we need to fwd declare this?

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.cc
(right):

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:39: void
PaymentRequestSheetController::UpdateContentView() {
very neat!

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.h
(right):

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:46: void
UpdateContentView();
let's add a comment, here and below

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (left):

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:306: void
PaymentSheetViewController::UpdateContentView() {
yisss

[anthonyvd, 2017-03-23 16:14:12]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 16:14:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[anthonyvd, 2017-03-23 16:18:11]

Addressed comments and rebased to include the new shipping address editor in the
CL. It was a hairy rebase but this should be no functional change.

+mad@ as an FYI: This basically moves the mechanism you implemented in the
editors to the base sheet class (since we realized other sheets needed to update
as well sometimes) :)

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/order_summary_view_controller.h (right):

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/order_summary_view_controller.h:39:
views::Button* pay_button_;
On 2017/03/23 at 11:36:45, Mathieu Perreault wrote:
> nit: do we need to fwd declare this?

Yep, done.

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.cc
(right):

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:39: void
PaymentRequestSheetController::UpdateContentView() {
On 2017/03/23 at 11:36:45, Mathieu Perreault wrote:
> very neat!

Thanks!

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.h
(right):

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:46: void
UpdateContentView();
On 2017/03/23 at 11:36:45, Mathieu Perreault wrote:
> let's add a comment, here and below

Done.

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (left):

https://codereview.chromium.org/2768133002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:306: void
PaymentSheetViewController::UpdateContentView() {
On 2017/03/23 at 11:36:45, Mathieu Perreault wrote:
> yisss

Yeah, this is much cleaner for all sheets!

[commit-bot: I haz the power, 2017-03-23 16:31:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 16:31:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2017-03-23 16:40:24]

lgtm, didn't fully review the rebase but I'm confident nothing got lost :)

[anthonyvd, 2017-03-23 18:33:20]

Patchset #4 (id:60001) has been deleted

[anthonyvd, 2017-03-23 19:26:42]

Patchset #4 (id:80001) has been deleted

[anthonyvd, 2017-03-23 20:03:34]

Patchset #4 (id:100001) has been deleted

[anthonyvd, 2017-03-23 20:42:54]

Patchset #4 (id:120001) has been deleted

[Mathieu, 2017-03-24 12:48:21]

On 2017/03/23 16:40:24, Mathieu (slow - travel) wrote:
> lgtm, didn't fully review the rebase but I'm confident nothing got lost :)

I looked at the 5 failing tests, they all wait for the dialog to close
(DialogEvent::DIALOG_CLOSED)

This suggests that the way the dialog closes is wrong. Perhaps now that the
Controller holds on to the |content_view_|, the Controller gets deleted (and
destroys the View) before other views are? I would look into this code...

[anthonyvd, 2017-03-24 14:48:02]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 14:48:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 15:44:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 15:44:54]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-03-24 15:46:52]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-03-24 15:46:52]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2768133002/#ps190001
(title: "Add comments.")

[commit-bot: I haz the power, 2017-03-24 15:47:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 15:56:18]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1490370412482300,
"parent_rev": "9aac56e95a24dcdeec343f906922f06cddfdf5a5", "commit_rev":
"5807cbec8e12dc0181f62c69221751486f8176ff"}

[commit-bot: I haz the power, 2017-03-24 15:57:02]

Description was changed from

==========
[Web Payments] Refactor sheet display to allow updating view content

BUG=703302
==========

to

==========
[Web Payments] Refactor sheet display to allow updating view content

BUG=703302

Review-Url: https://codereview.chromium.org/2768133002
Cr-Commit-Position: refs/heads/master@{#459438}
Committed:
https://chromium.googlesource.com/chromium/src/+/5807cbec8e12dc0181f62c692217...
==========

[commit-bot: I haz the power, 2017-03-24 15:57:03]

Committed patchset #7 (id:190001) as
https://chromium.googlesource.com/chromium/src/+/5807cbec8e12dc0181f62c692217...