[Payment Request] Error message screen

ios/chrome/browser/payments/payment_request_error_coordinator.mm

Index: ios/chrome/browser/payments/payment_request_error_coordinator.mm
diff --git a/ios/chrome/browser/payments/payment_request_error_coordinator.mm b/ios/chrome/browser/payments/payment_request_error_coordinator.mm
new file mode 100644
index 0000000000000000000000000000000000000000..30c294cc159128602a4d21899c9d42f6bb4cc482
--- /dev/null
+++ b/ios/chrome/browser/payments/payment_request_error_coordinator.mm
@@ -0,0 +1,55 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/chrome/browser/payments/payment_request_error_coordinator.h"
+
+#import "base/ios/weak_nsobject.h"
+#include "base/mac/scoped_nsobject.h"
+#include "components/strings/grit/components_strings.h"
+#include "ui/base/l10n/l10n_util.h"
+
+@interface PaymentRequestErrorCoordinator () {
+  base::WeakNSProtocol<id<PaymentRequestErrorCoordinatorDelegate>> _delegate;
+  base::scoped_nsobject<PaymentRequestErrorViewController> _viewController;
+}
+
+@end
+
+@implementation PaymentRequestErrorCoordinator
+
+@synthesize callback = _callback;
+
+- (id<PaymentRequestErrorCoordinatorDelegate>)delegate {
+  return _delegate.get();
+}
+
+- (void)setDelegate:(id<PaymentRequestErrorCoordinatorDelegate>)delegate {
+  _delegate.reset(delegate);
+}
+
+- (void)start {
+  _viewController.reset([[PaymentRequestErrorViewController alloc] init]);
+  [_viewController
+      setErrorMessage:l10n_util::GetNSString(IDS_PAYMENTS_ERROR_MESSAGE)];
+  [_viewController setDelegate:self];
+  [_viewController loadModel];
+
+  [[self baseViewController] presentViewController:_viewController
+                                          animated:YES
+                                        completion:nil];
+}
+
+- (void)stop {
+  [_viewController dismissViewControllerAnimated:YES completion:nil];

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

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]

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:40]

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?

+  _viewController.reset();
+}
+
+#pragma mark - PaymentRequestErrorViewControllerDelegate
+
+- (void)paymentRequestErrorViewControllerDidDismiss:
+    (PaymentRequestErrorViewController*)controller {
+  [_delegate paymentRequestErrorCoordinatorDidDismiss:self];
+}
+
+@end