[Payment Request] Error message screen

[Payment Request] Error message screen

Displays an error message if:
1. PaymentResponse.complete() gets called with 'fail'.
2. The promise returned by PaymentRequestUpdateEvent.updateWith()
doesn't get settled in a reasonable amount of time.

Payment Request will cancel once the user dismisses the error screen.

TBR=rouslan@,sdefresne@,

BUG=602666
Review-Url: https://codereview.chromium.org/2701923003
Cr-Commit-Position: refs/heads/master@{#453208}
Committed: https://chromium.googlesource.com/chromium/src/+/fac86e79522c21f6683ab4e973443733d3dfbacd

[Moe, 2017-02-17 21:09:52]

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

[commit-bot: I haz the power, 2017-02-17 21:10:38]

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

[Moe, 2017-02-17 21:51:09]

Description was changed from

==========
[Payment Request] Error message screen

Displays an error message if:
1. PaymentResponse.complete() gets called with 'fail'.
2. The promise returned by PaymentRequestUpdateEvent.updateWith()
doesn't get settled in a reasonable amount of time.

Payment Request will cancel once the user dismisses the error screen.

BUG=602666
==========

to

==========
[Payment Request] Error message screen

Displays an error message if:
1. PaymentResponse.complete() gets called with 'fail'.
2. The promise returned by PaymentRequestUpdateEvent.updateWith()
doesn't get settled in a reasonable amount of time.

Payment Request will cancel once the user dismisses the error screen.

TBR=rouslan@,sdefresne@,

BUG=602666
==========

[Moe, 2017-02-17 21:51:09]

mahmadi@chromium.org changed reviewers:
+ eugenebut@chromium.org, lpromero@chromium.org, rouslan@chromium.org,
sdefresne@chromium.org

[Moe, 2017-02-17 22:00:26]

Hi reviewers, Please review this CL.

lpromero@ ios/c/b/payments/
eugenebut@ 	ios/web/
TBR rouslan@, sdefrense@ for component strings.

eugene@, we managed to come up with an example
(https://rsolomakhin.github.io/pr/ko/fail/) that exceeds the parsing depth of 8
by 1. Therefore, settings it to 10 to be slightly safer.

[Eugene But (OOO till 7-30), 2017-02-17 22:52:34]

ios/web lgtm

[commit-bot: I haz the power, 2017-02-17 23:09:28]

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

[commit-bot: I haz the power, 2017-02-17 23:09:28]

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

[lpromero, 2017-02-20 17:14:07]

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_coordinator.mm (right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_coordinator.mm:281:
_errorViewController.reset([[PaymentRequestErrorViewController alloc] init]);
This should be created and presented via a coordinator.

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_error_view_controller.h
(right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_error_view_controller.h:12: extern
NSString* const kPaymentRequestErrorCollectionViewId;
s/Id/ID

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_error_view_controller.mm
(right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_error_view_controller.mm:56: // Set
up right (ok) button.
Nit: s/right/trailing

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_error_view_controller.mm:62:
action:@selector(onOk)]);
You should create a protocol for this method. Have a look at
SuggestionsItemActions for an example.

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:113: // as if
PaymentResponse.complete() was invoked with the default "unkown"
*unknown

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:386: [self dismissUI];
There seems to be a retain cycle. self retains _paymentRequestCoordinator, which
retains the error view controller, which retains the block, which retains self.

You need to use the weak self dance as in line 234.

[Moe, 2017-02-21 04:58:59]

Patchset #2 (id:20001) has been deleted

[Moe, 2017-02-21 05:07:06]

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_coordinator.mm (right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_coordinator.mm:281:
_errorViewController.reset([[PaymentRequestErrorViewController alloc] init]);
On 2017/02/20 17:14:06, lpromero wrote:
> This should be created and presented via a coordinator.

Done. So only one view controller per coordinator?

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_error_view_controller.h
(right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_error_view_controller.h:12: extern
NSString* const kPaymentRequestErrorCollectionViewId;
On 2017/02/20 17:14:06, lpromero wrote:
> s/Id/ID

Done.

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_error_view_controller.mm
(right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_error_view_controller.mm:56: // Set
up right (ok) button.
On 2017/02/20 17:14:07, lpromero wrote:
> Nit: s/right/trailing

Done.

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_error_view_controller.mm:62:
action:@selector(onOk)]);
On 2017/02/20 17:14:07, lpromero wrote:
> You should create a protocol for this method. Have a look at
> SuggestionsItemActions for an example.

I took a look at SuggestionsItemActions. It seems like an "actions protocol"
seems to be more appropriate for a reusable item rather than a button in a view
controller. What benefits does such a protocol have? for testing perhaps?

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:113: // as if
PaymentResponse.complete() was invoked with the default "unkown"
On 2017/02/20 17:14:07, lpromero wrote:
> *unknown

Done.

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:386: [self dismissUI];
On 2017/02/20 17:14:07, lpromero wrote:
> There seems to be a retain cycle. self retains _paymentRequestCoordinator,
which
> retains the error view controller, which retains the block, which retains
self.
> 
> You need to use the weak self dance as in line 234.

Done.

https://codereview.chromium.org/2701923003/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_coordinator.mm (right):

https://codereview.chromium.org/2701923003/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator.mm:44:
[_viewController dismissViewControllerAnimated:YES completion:nil];
I'm not sure I'm understanding the docs about
dismissViewControllerAnimated:completion:
https://developer.apple.com/reference/uikit/uiviewcontroller/1621505-dismissv...

"Dismisses the view controller that was presented modally by the view
controller. ... The presenting view controller is responsible for dismissing the
view controller it presented. ... If you call this method on the presented view
controller itself, UIKit asks the presenting view controller to handle the
dismissal."

according to the docs, it's more appropriate to call this method on
self.baseViewController (in this case is a navigation controller) which
presented the _viewController. However, doing so, dismisses the navigation
controller itself from its presenting view controller. So we shouldn't call this
method on a view controller which itself is being presented?

[lpromero, 2017-02-21 12:26:22]

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_coordinator.mm (right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_coordinator.mm:281:
_errorViewController.reset([[PaymentRequestErrorViewController alloc] init]);
On 2017/02/21 05:07:06, moe wrote:
> On 2017/02/20 17:14:06, lpromero wrote:
> > This should be created and presented via a coordinator.
> 
> Done. So only one view controller per coordinator?

As a rule of thumb yes. This instance is pretty good case for a simple
coordinator.

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_error_view_controller.mm
(right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_error_view_controller.mm:62:
action:@selector(onOk)]);
On 2017/02/21 05:07:06, moe wrote:
> On 2017/02/20 17:14:07, lpromero wrote:
> > You should create a protocol for this method. Have a look at
> > SuggestionsItemActions for an example.
> 
> I took a look at SuggestionsItemActions. It seems like an "actions protocol"
> seems to be more appropriate for a reusable item rather than a button in a
view
> controller. What benefits does such a protocol have? for testing perhaps? 

It also make explicit the ways in for this controller. It can be used for
example as a gesture recognizer action, as a keyboard command trigger.
Testing is questionable (jury is still out on this question from the other
review with protocol conformance in the public interface!).

https://codereview.chromium.org/2701923003/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_coordinator.mm (right):

https://codereview.chromium.org/2701923003/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator.mm:44:
[_viewController dismissViewControllerAnimated:YES completion:nil];
On 2017/02/21 05:07:06, moe wrote:
> I'm not sure I'm understanding the docs about
> dismissViewControllerAnimated:completion:
>
https://developer.apple.com/reference/uikit/uiviewcontroller/1621505-dismissv...
> 
> "Dismisses the view controller that was presented modally by the view
> controller. ... The presenting view controller is responsible for dismissing
the
> view controller it presented. ... If you call this method on the presented
view
> controller itself, UIKit asks the presenting view controller to handle the
> dismissal."
> 
> according to the docs, it's more appropriate to call this method on
> self.baseViewController (in this case is a navigation controller) which
> presented the _viewController. However, doing so, dismisses the navigation
> controller itself from its presenting view controller. So we shouldn't call
this
> method on a view controller which itself is being presented?

Yes, this one is a bit weird.

Historically, -dismissViewController:animated:completion: was to be called on
the "presented". But it was a bad pattern, as it should be on to the presenter
to dismiss (to balance presentation/dismissal). So now (since maybe 5 releases
of iOS maybe?), the best practice is to call on the "presenter".
The problem is that they kept supporting the old behavior that calling it on the
"presented" could dismiss it.

I think that your issue is that _viewController has already been dismissed
somehow (bug) by the time you call stop, and if -stop uses
[self.baseViewController dismiss…], then it dismisses baseViewController itself
instead of its own presented. Can you confirm that when reproducing with
[self.baseViewController dismiss…] it shows that the nav has no presented
controller? (And thus gets dismissed?)

And the bug works the other way too!

If you keep [_viewController dismiss…] but something has been presented on
_viewController, you don't end up dismissing _viewController, but the one
presented on top only! Leaving one view controller too many!

That's why the way we recommend implementing -stop is:

[[_viewController presentingViewController] dismiss…];

That way, if _viewController has already been dismissed, its
presentingViewController is nil, so no-op. And if _viewController has stuff
presented on top of it, the entire stack including _viewController is dismissed.

[Moe, 2017-02-21 15:37:41]

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_error_view_controller.mm
(right):

https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_error_view_controller.mm:62:
action:@selector(onOk)]);
On 2017/02/21 12:26:22, lpromero wrote:
> On 2017/02/21 05:07:06, moe wrote:
> > On 2017/02/20 17:14:07, lpromero wrote:
> > > You should create a protocol for this method. Have a look at
> > > SuggestionsItemActions for an example.
> > 
> > I took a look at SuggestionsItemActions. It seems like an "actions protocol"
> > seems to be more appropriate for a reusable item rather than a button in a
> view
> > controller. What benefits does such a protocol have? for testing perhaps? 
> 
> It also make explicit the ways in for this controller. It can be used for
> example as a gesture recognizer action, as a keyboard command trigger.
> Testing is questionable (jury is still out on this question from the other
> review with protocol conformance in the public interface!).

Makes sense. Done.

https://codereview.chromium.org/2701923003/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_coordinator.mm (right):

https://codereview.chromium.org/2701923003/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator.mm:44:
[_viewController dismissViewControllerAnimated:YES completion:nil];
On 2017/02/21 12:26:22, lpromero wrote:
> On 2017/02/21 05:07:06, moe wrote:
> > I'm not sure I'm understanding the docs about
> > dismissViewControllerAnimated:completion:
> >
>
https://developer.apple.com/reference/uikit/uiviewcontroller/1621505-dismissv...
> > 
> > "Dismisses the view controller that was presented modally by the view
> > controller. ... The presenting view controller is responsible for dismissing
> the
> > view controller it presented. ... If you call this method on the presented
> view
> > controller itself, UIKit asks the presenting view controller to handle the
> > dismissal."
> > 
> > according to the docs, it's more appropriate to call this method on
> > self.baseViewController (in this case is a navigation controller) which
> > presented the _viewController. However, doing so, dismisses the navigation
> > controller itself from its presenting view controller. So we shouldn't call
> this
> > method on a view controller which itself is being presented?
> 
> Yes, this one is a bit weird.
> 
> Historically, -dismissViewController:animated:completion: was to be called on
> the "presented". But it was a bad pattern, as it should be on to the presenter
> to dismiss (to balance presentation/dismissal). So now (since maybe 5 releases
> of iOS maybe?), the best practice is to call on the "presenter".
> The problem is that they kept supporting the old behavior that calling it on
the
> "presented" could dismiss it.
> 
> I think that your issue is that _viewController has already been dismissed
> somehow (bug) by the time you call stop, and if -stop uses
> [self.baseViewController dismiss…], then it dismisses baseViewController
itself
> instead of its own presented. Can you confirm that when reproducing with
> [self.baseViewController dismiss…] it shows that the nav has no presented
> controller? (And thus gets dismissed?)
> 
> And the bug works the other way too!
> 
> If you keep [_viewController dismiss…] but something has been presented on
> _viewController, you don't end up dismissing _viewController, but the one
> presented on top only! Leaving one view controller too many!
> 
> That's why the way we recommend implementing -stop is:
> 
> [[_viewController presentingViewController] dismiss…];
> 
> That way, if _viewController has already been dismissed, its
> presentingViewController is nil, so no-op. And if _viewController has stuff
> presented on top of it, the entire stack including _viewController is
dismissed.

Thank you for the explanation Louis! Your are right about the problem (bug).
-stop is being called twice for every coordinator b/c ChromeCoordinator calls
[self stop] in its -dealloc. So the first time, the _viewController was being
dismissed and the second time the navigationController itself since it was no
longer presenting. Changing the implementation to
[[_viewController presentingViewController] dismiss…];
solves the issue, but do you think ChromeCoordinator should be calling stop in
-dealloc?

[please use gerrit instead, 2017-02-21 16:37:08]

LGTM % comments

https://codereview.chromium.org/2701923003/diff/60001/components/components_s...
File components/components_strings.grd (right):

https://codereview.chromium.org/2701923003/diff/60001/components/components_s...
components/components_strings.grd:283: Ok
Can you use IDS_OK instead?

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/app/strings/...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:940: Ok
Can you use IDS_OK instead?

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_view_controller.mm
(right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_view_controller.mm:1: //
Copyright 2017 The Chromium Authors. All rights reserved.
This can be used to get Objective C readability. Is it time?

[lpromero, 2017-02-21 16:38:32]

On 2017/02/21 15:37:41, moe wrote:
>
https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
> File ios/chrome/browser/payments/payment_request_error_view_controller.mm
> (right):
> 
>
https://codereview.chromium.org/2701923003/diff/1/ios/chrome/browser/payments...
> ios/chrome/browser/payments/payment_request_error_view_controller.mm:62:
> action:@selector(onOk)]);
> On 2017/02/21 12:26:22, lpromero wrote:
> > On 2017/02/21 05:07:06, moe wrote:
> > > On 2017/02/20 17:14:07, lpromero wrote:
> > > > You should create a protocol for this method. Have a look at
> > > > SuggestionsItemActions for an example.
> > > 
> > > I took a look at SuggestionsItemActions. It seems like an "actions
protocol"
> > > seems to be more appropriate for a reusable item rather than a button in a
> > view
> > > controller. What benefits does such a protocol have? for testing perhaps? 
> > 
> > It also make explicit the ways in for this controller. It can be used for
> > example as a gesture recognizer action, as a keyboard command trigger.
> > Testing is questionable (jury is still out on this question from the other
> > review with protocol conformance in the public interface!).
> 
> Makes sense. Done.
> 
>
https://codereview.chromium.org/2701923003/diff/40001/ios/chrome/browser/paym...
> File ios/chrome/browser/payments/payment_request_error_coordinator.mm (right):
> 
>
https://codereview.chromium.org/2701923003/diff/40001/ios/chrome/browser/paym...
> ios/chrome/browser/payments/payment_request_error_coordinator.mm:44:
> [_viewController dismissViewControllerAnimated:YES completion:nil];
> On 2017/02/21 12:26:22, lpromero wrote:
> > On 2017/02/21 05:07:06, moe wrote:
> > > I'm not sure I'm understanding the docs about
> > > dismissViewControllerAnimated:completion:
> > >
> >
>
https://developer.apple.com/reference/uikit/uiviewcontroller/1621505-dismissv...
> > > 
> > > "Dismisses the view controller that was presented modally by the view
> > > controller. ... The presenting view controller is responsible for
dismissing
> > the
> > > view controller it presented. ... If you call this method on the presented
> > view
> > > controller itself, UIKit asks the presenting view controller to handle the
> > > dismissal."
> > > 
> > > according to the docs, it's more appropriate to call this method on
> > > self.baseViewController (in this case is a navigation controller) which
> > > presented the _viewController. However, doing so, dismisses the navigation
> > > controller itself from its presenting view controller. So we shouldn't
call
> > this
> > > method on a view controller which itself is being presented?
> > 
> > Yes, this one is a bit weird.
> > 
> > Historically, -dismissViewController:animated:completion: was to be called
on
> > the "presented". But it was a bad pattern, as it should be on to the
presenter
> > to dismiss (to balance presentation/dismissal). So now (since maybe 5
releases
> > of iOS maybe?), the best practice is to call on the "presenter".
> > The problem is that they kept supporting the old behavior that calling it on
> the
> > "presented" could dismiss it.
> > 
> > I think that your issue is that _viewController has already been dismissed
> > somehow (bug) by the time you call stop, and if -stop uses
> > [self.baseViewController dismiss…], then it dismisses baseViewController
> itself
> > instead of its own presented. Can you confirm that when reproducing with
> > [self.baseViewController dismiss…] it shows that the nav has no presented
> > controller? (And thus gets dismissed?)
> > 
> > And the bug works the other way too!
> > 
> > If you keep [_viewController dismiss…] but something has been presented on
> > _viewController, you don't end up dismissing _viewController, but the one
> > presented on top only! Leaving one view controller too many!
> > 
> > That's why the way we recommend implementing -stop is:
> > 
> > [[_viewController presentingViewController] dismiss…];
> > 
> > That way, if _viewController has already been dismissed, its
> > presentingViewController is nil, so no-op. And if _viewController has stuff
> > presented on top of it, the entire stack including _viewController is
> dismissed.
> 
> Thank you for the explanation Louis! Your are right about the problem (bug).
> -stop is being called twice for every coordinator b/c ChromeCoordinator calls
> [self stop] in its -dealloc. So the first time, the _viewController was being
> dismissed and the second time the navigationController itself since it was no
> longer presenting. Changing the implementation to
> [[_viewController presentingViewController] dismiss…];
> solves the issue, but do you think ChromeCoordinator should be calling stop in
> -dealloc?

BrowserCoordinator, the future replacement of ChromeCoordinator, removed the
call to stop in dealloc:
https://chromium.googlesource.com/chromium/src/+/b367bcc4494b1b21126852955719...

I pinged marq about the more robust solution.

[lpromero, 2017-02-21 16:51:08]

LGTM with nits/

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_coordinator.mm (right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_coordinator.mm:347: void
(^callback)() = coordinator.callback;
Nit: Use ProceduralBlock.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_coordinator.h (right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator.h:30:
@property(nonatomic, copy) void (^callback)();
Nit: Use ProceduralBlock.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator.h:30:
@property(nonatomic, copy) void (^callback)();
This is leaked as you don't use ARC.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator.h:33:
@property(nonatomic, weak) id<PaymentRequestErrorCoordinatorDelegate> delegate;
s/weak/assign under non-ARC.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_coordinator_unittest.mm
(right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator_unittest.mm:33: id
persented_view_controller =
*presented

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_view_controller.h
(right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_view_controller.h:29:
@property(nonatomic, assign) NSString* errorMessage;
s/assign/copy

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_view_controller.h:32:
@property(nonatomic, weak) id<PaymentRequestErrorViewControllerDelegate>
s/weak/assign under non-ARC

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:94: // page does not call
settle the pending update promise in a timely fashion.
Should "call" be removed?

[Moe, 2017-02-21 22:56:15]

Thank you all.

https://codereview.chromium.org/2701923003/diff/60001/components/components_s...
File components/components_strings.grd (right):

https://codereview.chromium.org/2701923003/diff/60001/components/components_s...
components/components_strings.grd:283: Ok
On 2017/02/21 16:37:08, rouslan wrote:
> Can you use IDS_OK instead?

Since this is an accessibility label, I would rather add another one for OK to
be consistent with the others.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/app/strings/...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:940: Ok
On 2017/02/21 16:37:08, rouslan wrote:
> Can you use IDS_OK instead?

Done.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_coordinator.mm (right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_coordinator.mm:347: void
(^callback)() = coordinator.callback;
On 2017/02/21 16:51:07, lpromero wrote:
> Nit: Use ProceduralBlock.

Done.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_coordinator.mm:347: void
(^callback)() = coordinator.callback;
On 2017/02/21 16:51:07, lpromero wrote:
> Nit: Use ProceduralBlock.

Just to make sure, since the callback gets deallocated once
_errorCoordinator.reset() is called, does it make sense to do something like the
following here:

ProceduralBlock callback = [[coordinator.callback copy] autorelease];

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_coordinator.h (right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator.h:30:
@property(nonatomic, copy) void (^callback)();
On 2017/02/21 16:51:07, lpromero wrote:
> Nit: Use ProceduralBlock.

Done.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator.h:30:
@property(nonatomic, copy) void (^callback)();
On 2017/02/21 16:51:07, lpromero wrote:
> This is leaked as you don't use ARC.

Done. Started using base::mac::ReleaseProperties(self) in -dealloc.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator.h:33:
@property(nonatomic, weak) id<PaymentRequestErrorCoordinatorDelegate> delegate;
On 2017/02/21 16:51:07, lpromero wrote:
> s/weak/assign under non-ARC.

Done.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_coordinator_unittest.mm
(right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_coordinator_unittest.mm:33: id
persented_view_controller =
On 2017/02/21 16:51:07, lpromero wrote:
> *presented

Done.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_view_controller.h
(right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_view_controller.h:29:
@property(nonatomic, assign) NSString* errorMessage;
On 2017/02/21 16:51:08, lpromero wrote:
> s/assign/copy

Done! also started using base::mac::ReleaseProperties in -dealloc. Looks like
base::mac::ObjCPropertyReleaser is deprecated.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_view_controller.h:32:
@property(nonatomic, weak) id<PaymentRequestErrorViewControllerDelegate>
On 2017/02/21 16:51:08, lpromero wrote:
> s/weak/assign under non-ARC

Done.

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_view_controller.mm
(right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_view_controller.mm:1: //
Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/02/21 16:37:08, rouslan wrote:
> This can be used to get Objective C readability. Is it time?

I'd wait a bit longer until I'd stop making obvious memory management mistakes.
I have quite a few CLs with components written from scratch to use for that
purpose :)

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:94: // page does not call
settle the pending update promise in a timely fashion.
On 2017/02/21 16:51:08, lpromero wrote:
> Should "call" be removed?

Done.

[lpromero, 2017-02-22 13:31:44]

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_coordinator.mm (right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_coordinator.mm:347: void
(^callback)() = coordinator.callback;
On 2017/02/21 22:56:14, moe wrote:
> On 2017/02/21 16:51:07, lpromero wrote:
> > Nit: Use ProceduralBlock.
> 
> Just to make sure, since the callback gets deallocated once
> _errorCoordinator.reset() is called,
>  does it make sense to do something like the
> following here:
> 
> ProceduralBlock callback = [[coordinator.callback copy] autorelease]; 

Interesting question. If you were using ARC, line 347 would be a strong ref to
the block so it wouldn't get dealloced on line 350.
Since this is non-ARC, though, you are correct that the block can be deallocated
as is. Bug!

Use a base::mac::ScopedBlock for the local var. Good catch!

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_error_view_controller.h
(right):

https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_error_view_controller.h:29:
@property(nonatomic, assign) NSString* errorMessage;
On 2017/02/21 22:56:15, moe wrote:
> On 2017/02/21 16:51:08, lpromero wrote:
> > s/assign/copy
> 
> Done! also started using base::mac::ReleaseProperties in -dealloc. Looks like
> base::mac::ObjCPropertyReleaser is deprecated.

Good to know. Thanks for the heads up!

[Moe, 2017-02-22 13:56:13]

On 2017/02/22 13:31:44, lpromero wrote:
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> File ios/chrome/browser/payments/payment_request_coordinator.mm (right):
> 
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> ios/chrome/browser/payments/payment_request_coordinator.mm:347: void
> (^callback)() = coordinator.callback;
> On 2017/02/21 22:56:14, moe wrote:
> > On 2017/02/21 16:51:07, lpromero wrote:
> > > Nit: Use ProceduralBlock.
> > 
> > Just to make sure, since the callback gets deallocated once
> > _errorCoordinator.reset() is called,
> >  does it make sense to do something like the
> > following here:
> > 
> > ProceduralBlock callback = [[coordinator.callback copy] autorelease]; 
> 
> Interesting question. If you were using ARC, line 347 would be a strong ref to
> the block so it wouldn't get dealloced on line 350.
> Since this is non-ARC, though, you are correct that the block can be
deallocated
> as is. Bug!
> 
> Use a base::mac::ScopedBlock for the local var. Good catch!
> 
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> File ios/chrome/browser/payments/payment_request_error_view_controller.h
> (right):
> 
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> ios/chrome/browser/payments/payment_request_error_view_controller.h:29:
> @property(nonatomic, assign) NSString* errorMessage;
> On 2017/02/21 22:56:15, moe wrote:
> > On 2017/02/21 16:51:08, lpromero wrote:
> > > s/assign/copy
> > 
> > Done! also started using base::mac::ReleaseProperties in -dealloc. Looks
like
> > base::mac::ObjCPropertyReleaser is deprecated.
> 
> Good to know. Thanks for the heads up!

Speaking of ARC, I'm going to wait for this land and rebase accordingly:
https://codereview.chromium.org/2710493006/

[lpromero, 2017-02-22 13:58:58]

On 2017/02/22 13:56:13, moe wrote:
> On 2017/02/22 13:31:44, lpromero wrote:
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > File ios/chrome/browser/payments/payment_request_coordinator.mm (right):
> > 
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > ios/chrome/browser/payments/payment_request_coordinator.mm:347: void
> > (^callback)() = coordinator.callback;
> > On 2017/02/21 22:56:14, moe wrote:
> > > On 2017/02/21 16:51:07, lpromero wrote:
> > > > Nit: Use ProceduralBlock.
> > > 
> > > Just to make sure, since the callback gets deallocated once
> > > _errorCoordinator.reset() is called,
> > >  does it make sense to do something like the
> > > following here:
> > > 
> > > ProceduralBlock callback = [[coordinator.callback copy] autorelease]; 
> > 
> > Interesting question. If you were using ARC, line 347 would be a strong ref
to
> > the block so it wouldn't get dealloced on line 350.
> > Since this is non-ARC, though, you are correct that the block can be
> deallocated
> > as is. Bug!
> > 
> > Use a base::mac::ScopedBlock for the local var. Good catch!
> > 
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > File ios/chrome/browser/payments/payment_request_error_view_controller.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > ios/chrome/browser/payments/payment_request_error_view_controller.h:29:
> > @property(nonatomic, assign) NSString* errorMessage;
> > On 2017/02/21 22:56:15, moe wrote:
> > > On 2017/02/21 16:51:08, lpromero wrote:
> > > > s/assign/copy
> > > 
> > > Done! also started using base::mac::ReleaseProperties in -dealloc. Looks
> like
> > > base::mac::ObjCPropertyReleaser is deprecated.
> > 
> > Good to know. Thanks for the heads up!
> 
> Speaking of ARC, I'm going to wait for this land and rebase accordingly:
> https://codereview.chromium.org/2710493006/

Then this changes a couple things. Let me know, I'll make a new pass when
ARCified.

[lpromero, 2017-02-22 13:59:00]

On 2017/02/22 13:56:13, moe wrote:
> On 2017/02/22 13:31:44, lpromero wrote:
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > File ios/chrome/browser/payments/payment_request_coordinator.mm (right):
> > 
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > ios/chrome/browser/payments/payment_request_coordinator.mm:347: void
> > (^callback)() = coordinator.callback;
> > On 2017/02/21 22:56:14, moe wrote:
> > > On 2017/02/21 16:51:07, lpromero wrote:
> > > > Nit: Use ProceduralBlock.
> > > 
> > > Just to make sure, since the callback gets deallocated once
> > > _errorCoordinator.reset() is called,
> > >  does it make sense to do something like the
> > > following here:
> > > 
> > > ProceduralBlock callback = [[coordinator.callback copy] autorelease]; 
> > 
> > Interesting question. If you were using ARC, line 347 would be a strong ref
to
> > the block so it wouldn't get dealloced on line 350.
> > Since this is non-ARC, though, you are correct that the block can be
> deallocated
> > as is. Bug!
> > 
> > Use a base::mac::ScopedBlock for the local var. Good catch!
> > 
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > File ios/chrome/browser/payments/payment_request_error_view_controller.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > ios/chrome/browser/payments/payment_request_error_view_controller.h:29:
> > @property(nonatomic, assign) NSString* errorMessage;
> > On 2017/02/21 22:56:15, moe wrote:
> > > On 2017/02/21 16:51:08, lpromero wrote:
> > > > s/assign/copy
> > > 
> > > Done! also started using base::mac::ReleaseProperties in -dealloc. Looks
> like
> > > base::mac::ObjCPropertyReleaser is deprecated.
> > 
> > Good to know. Thanks for the heads up!
> 
> Speaking of ARC, I'm going to wait for this land and rebase accordingly:
> https://codereview.chromium.org/2710493006/

Then this changes a couple things. Let me know, I'll make a new pass when
ARCified.

[Moe, 2017-02-22 18:21:37]

On 2017/02/22 13:59:00, lpromero wrote:
> On 2017/02/22 13:56:13, moe wrote:
> > On 2017/02/22 13:31:44, lpromero wrote:
> > >
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > > File ios/chrome/browser/payments/payment_request_coordinator.mm (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > > ios/chrome/browser/payments/payment_request_coordinator.mm:347: void
> > > (^callback)() = coordinator.callback;
> > > On 2017/02/21 22:56:14, moe wrote:
> > > > On 2017/02/21 16:51:07, lpromero wrote:
> > > > > Nit: Use ProceduralBlock.
> > > > 
> > > > Just to make sure, since the callback gets deallocated once
> > > > _errorCoordinator.reset() is called,
> > > >  does it make sense to do something like the
> > > > following here:
> > > > 
> > > > ProceduralBlock callback = [[coordinator.callback copy] autorelease]; 
> > > 
> > > Interesting question. If you were using ARC, line 347 would be a strong
ref
> to
> > > the block so it wouldn't get dealloced on line 350.
> > > Since this is non-ARC, though, you are correct that the block can be
> > deallocated
> > > as is. Bug!
> > > 
> > > Use a base::mac::ScopedBlock for the local var. Good catch!
> > > 
> > >
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > > File ios/chrome/browser/payments/payment_request_error_view_controller.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2701923003/diff/60001/ios/chrome/browser/paym...
> > > ios/chrome/browser/payments/payment_request_error_view_controller.h:29:
> > > @property(nonatomic, assign) NSString* errorMessage;
> > > On 2017/02/21 22:56:15, moe wrote:
> > > > On 2017/02/21 16:51:08, lpromero wrote:
> > > > > s/assign/copy
> > > > 
> > > > Done! also started using base::mac::ReleaseProperties in -dealloc. Looks
> > like
> > > > base::mac::ObjCPropertyReleaser is deprecated.
> > > 
> > > Good to know. Thanks for the heads up!
> > 
> > Speaking of ARC, I'm going to wait for this land and rebase accordingly:
> > https://codereview.chromium.org/2710493006/
> 
> Then this changes a couple things. Let me know, I'll make a new pass when
> ARCified.

Thanks Louis, ARCified. Please have another pass at it.

[lpromero, 2017-02-23 09:17:41]

lgtm.

Thanks for having created the other actions protocols!

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
File ios/chrome/browser/payments/payment_request_error_coordinator_unittest.mm
(right):

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request_error_coordinator_unittest.mm:22:
[[[UIViewController alloc] init] autorelease];
The test target is not yet ARC? If not, nothing to do.

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request_manager.mm:359: [strongSelf
cancelRequestWithErrorMessage:@"Request canceled."];
Add early return if strongSelf == nil. The behavior will technically be the say
(nil-messaging will no-op here), but it's more maintainable. For example if
tomorrow this code is changed in:

PaymentRequestManager* strongSelf = weakSelf;
[strongSelf cancelRequestWithErrorMessage:@"Request canceled."];
metrics::Record("cancel");

Then you over-record the metric.

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request_manager.mm:388:
PaymentRequestManager* strongSelf = weakSelf;
Add early return if strongSelf == nil.

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request_manager.mm:390:
[_paymentRequestJsManager resolveResponsePromiseWithCompletionHandler:nil];
strongSelf->_paymentRequestJsManager, otherwise you retain self.

[Moe, 2017-02-23 14:44:01]

Thank you Louis!

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
File ios/chrome/browser/payments/payment_request_error_coordinator_unittest.mm
(right):

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request_error_coordinator_unittest.mm:22:
[[[UIViewController alloc] init] autorelease];
On 2017/02/23 09:17:41, lpromero wrote:
> The test target is not yet ARC? If not, nothing to do.

No it's not. I'll send out a CL to ARCify the test target in the future.

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request_manager.mm:359: [strongSelf
cancelRequestWithErrorMessage:@"Request canceled."];
On 2017/02/23 09:17:41, lpromero wrote:
> Add early return if strongSelf == nil. The behavior will technically be the
say
> (nil-messaging will no-op here), but it's more maintainable. For example if
> tomorrow this code is changed in:
> 
> PaymentRequestManager* strongSelf = weakSelf;
> [strongSelf cancelRequestWithErrorMessage:@"Request canceled."];
> metrics::Record("cancel");
> 
> Then you over-record the metric.

That makes sense. The base ACRify CL removes the early return in a couple of
places:
https://codereview.chromium.org/2710493006/patch/60001/70017
I'll add them back once it lands.

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request_manager.mm:388:
PaymentRequestManager* strongSelf = weakSelf;
On 2017/02/23 09:17:41, lpromero wrote:
> Add early return if strongSelf == nil.

Done.

https://codereview.chromium.org/2701923003/diff/100001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request_manager.mm:390:
[_paymentRequestJsManager resolveResponsePromiseWithCompletionHandler:nil];
On 2017/02/23 09:17:41, lpromero wrote:
> strongSelf->_paymentRequestJsManager, otherwise you retain self.

Done!

[Moe, 2017-02-24 23:07:41]

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

[commit-bot: I haz the power, 2017-02-24 23:08:45]

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-02-25 00:24:37]

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

[commit-bot: I haz the power, 2017-02-25 00:24:38]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Moe, 2017-02-27 13:17:10]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-02-27 13:17:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, rouslan@chromium.org, lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2701923003/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-27 13:17:25]

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-02-27 14:34:13]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488201430218480,
"parent_rev": "5d10b590e88508313c9e68d004cabc11bf6f55ce", "commit_rev":
"fac86e79522c21f6683ab4e973443733d3dfbacd"}

[commit-bot: I haz the power, 2017-02-27 14:34:50]

Description was changed from

==========
[Payment Request] Error message screen

Displays an error message if:
1. PaymentResponse.complete() gets called with 'fail'.
2. The promise returned by PaymentRequestUpdateEvent.updateWith()
doesn't get settled in a reasonable amount of time.

Payment Request will cancel once the user dismisses the error screen.

TBR=rouslan@,sdefresne@,

BUG=602666
==========

to

==========
[Payment Request] Error message screen

Displays an error message if:
1. PaymentResponse.complete() gets called with 'fail'.
2. The promise returned by PaymentRequestUpdateEvent.updateWith()
doesn't get settled in a reasonable amount of time.

Payment Request will cancel once the user dismisses the error screen.

TBR=rouslan@,sdefresne@,

BUG=602666

Review-Url: https://codereview.chromium.org/2701923003
Cr-Commit-Position: refs/heads/master@{#453208}
Committed:
https://chromium.googlesource.com/chromium/src/+/fac86e79522c21f6683ab4e97344...
==========

[commit-bot: I haz the power, 2017-02-27 14:34:52]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/fac86e79522c21f6683ab4e97344...