[Payment Request] Refactors PaymentRequestCoordinator.

[Payment Request] Refactors PaymentRequestCoordinator.

Refactoring asked for in CL https://codereview.chromium.org/2844783002/.

Moves some logic up into PaymentRequestManager, and other logic into
PaymentRequestMediator.

There's more to refactor, but this is a start.

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

[macourteau, 2017-05-05 20:12:44]

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

[macourteau, 2017-05-05 20:13:12]

Description was changed from

==========
[Payment Request] Refactors PaymentRequestCoordinator.

Moves some logic up into PaymentRequestManager, and other logic into
PaymentRequestMediator.

There's more to refactor, but this is a start.

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

to

==========
[Payment Request] Refactors PaymentRequestCoordinator.

Refactoring asked for in CL https://codereview.chromium.org/2844783002/.

Moves some logic up into PaymentRequestManager, and other logic into
PaymentRequestMediator.

There's more to refactor, but this is a start.

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

[commit-bot: I haz the power, 2017-05-05 20:13:24]

Dry run: CQ is trying da patch.

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

[macourteau, 2017-05-05 20:13:59]

macourteau@chromium.org changed reviewers:
+ marq@chromium.org

[commit-bot: I haz the power, 2017-05-05 21:20:37]

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

[commit-bot: I haz the power, 2017-05-05 21:20:37]

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-05-08 17:21:24]

mahmadi@chromium.org changed reviewers:
+ mahmadi@chromium.org

[macourteau, 2017-05-08 20:15:00]

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

[commit-bot: I haz the power, 2017-05-08 20:15:57]

Dry run: CQ is trying da patch.

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

[Moe, 2017-05-08 21:24:13]

lgtm. Thank you for the refactoring.

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/payments/payment_request_view_controller.h (right):

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/payments/payment_request_view_controller.h:25: -
(NSString*)authenticatedAccountName;
Optional: You could have the data source simply return the footer text and that
way refactor a bunch of logic into the mediator and also get rid of
showPaymentDataSource property.

[commit-bot: I haz the power, 2017-05-08 22:51:04]

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

[commit-bot: I haz the power, 2017-05-08 22:51:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[marq (ping after 24h), 2017-05-09 09:00:33]

This is a great start, thanks for digging into this!

As you continue refactoring, I would suggest that you consider this question:
What's the difference between the roles of the payment request manager and the
payment request mediator? Both are interfaces between the model layer and other
objects, but the distinction between them should be as clear as possible.

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/payments/payment_request_coordinator.h (right):

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/payments/payment_request_coordinator.h:57: cvc:(const
base::string16&)cvc;
The rules for using initialisms and other abbreviations in Objective-C are (1)
if they are used, they should be in uppercase (URL, HTTP, etc), and (2) they
shouldn't be used unless they are very widely-understood software engineering
terms.

So I would prefer that you s/cvc/verfificationCode/.

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/payments/payment_request_mediator.h (right):

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/payments/payment_request_mediator.h:16: :
NSObject<PaymentRequestViewControllerDataSource>
This is fine as-is; no need to make any change. For context, however, the
direction we're moving in is to have mediators "push" configuration into view
controllers rather than having view controllers pull it from a data source.

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/payments/payment_request_view_controller.h (right):

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/payments/payment_request_view_controller.h:25: -
(NSString*)authenticatedAccountName;
On 2017/05/08 21:24:12, Moe wrote:
> Optional: You could have the data source simply return the footer text and
that
> way refactor a bunch of logic into the mediator and also get rid of
> showPaymentDataSource property.

+1, good suggestion. This can be in a follow-up CL.

[macourteau, 2017-05-09 15:21:53]

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/payments/payment_request_coordinator.h (right):

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/payments/payment_request_coordinator.h:57: cvc:(const
base::string16&)cvc;
On 2017/05/09 09:00:33, marq (ping after 24h) wrote:
> The rules for using initialisms and other abbreviations in Objective-C are (1)
> if they are used, they should be in uppercase (URL, HTTP, etc), and (2) they
> shouldn't be used unless they are very widely-understood software engineering
> terms.
> 
> So I would prefer that you s/cvc/verfificationCode/.

Done.

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/payments/payment_request_mediator.h (right):

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/payments/payment_request_mediator.h:16: :
NSObject<PaymentRequestViewControllerDataSource>
On 2017/05/09 09:00:33, marq (ping after 24h) wrote:
> This is fine as-is; no need to make any change. For context, however, the
> direction we're moving in is to have mediators "push" configuration into view
> controllers rather than having view controllers pull it from a data source.

Acknowledged.

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/payments/payment_request_view_controller.h (right):

https://codereview.chromium.org/2866793002/diff/80001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/payments/payment_request_view_controller.h:25: -
(NSString*)authenticatedAccountName;
On 2017/05/09 09:00:33, marq (ping after 24h) wrote:
> On 2017/05/08 21:24:12, Moe wrote:
> > Optional: You could have the data source simply return the footer text and
> that
> > way refactor a bunch of logic into the mediator and also get rid of
> > showPaymentDataSource property.
> 
> +1, good suggestion. This can be in a follow-up CL.

Ack, will do in a follow-up CL.

[macourteau, 2017-05-09 15:21:57]

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

[commit-bot: I haz the power, 2017-05-09 15:22:28]

Dry run: CQ is trying da patch.

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

[marq (ping after 24h), 2017-05-09 15:40:12]

lgtm

https://codereview.chromium.org/2866793002/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/payments/payment_request_coordinator_unittest.mm
(right):

https://codereview.chromium.org/2866793002/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/payments/payment_request_coordinator_unittest.mm:154: SEL
selector = @selector(paymentRequestCoordinator:didCompletePaymentRequest
Nit -- this formatting confuses me (although I'm sure clang-format likes it just
fine). Can you fit the whole selector name on one line if you break after the
"(":

SEL selector = @selector(
    paymentRequestCoordinator:didCompletePaymentRequest:card:verificationCode:);

[macourteau, 2017-05-09 15:48:36]

https://codereview.chromium.org/2866793002/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/payments/payment_request_coordinator_unittest.mm
(right):

https://codereview.chromium.org/2866793002/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/payments/payment_request_coordinator_unittest.mm:154: SEL
selector = @selector(paymentRequestCoordinator:didCompletePaymentRequest
On 2017/05/09 15:40:12, marq (ping after 24h) wrote:
> Nit -- this formatting confuses me (although I'm sure clang-format likes it
just
> fine). Can you fit the whole selector name on one line if you break after the
> "(":
> 
> SEL selector = @selector(
>    
paymentRequestCoordinator:didCompletePaymentRequest:card:verificationCode:);

That doesn't fit. Changed to something clearer.

[macourteau, 2017-05-09 15:48:43]

The CQ bit was checked by macourteau@chromium.org

[macourteau, 2017-05-09 15:48:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
mahmadi@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2866793002/#ps120001
(title: "Reformats selector.")

[commit-bot: I haz the power, 2017-05-09 15:49:19]

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-05-09 19:15:55]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494344923550490,
"parent_rev": "83cfebca143f8af8a562d177d91c933697ab3d62", "commit_rev":
"3cf9dc16046236cc9772df62de7272a3412ded0a"}

[commit-bot: I haz the power, 2017-05-09 19:16:03]

Description was changed from

==========
[Payment Request] Refactors PaymentRequestCoordinator.

Refactoring asked for in CL https://codereview.chromium.org/2844783002/.

Moves some logic up into PaymentRequestManager, and other logic into
PaymentRequestMediator.

There's more to refactor, but this is a start.

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

to

==========
[Payment Request] Refactors PaymentRequestCoordinator.

Refactoring asked for in CL https://codereview.chromium.org/2844783002/.

Moves some logic up into PaymentRequestManager, and other logic into
PaymentRequestMediator.

There's more to refactor, but this is a start.

BUG=602666

Review-Url: https://codereview.chromium.org/2866793002
Cr-Commit-Position: refs/heads/master@{#470403}
Committed:
https://chromium.googlesource.com/chromium/src/+/3cf9dc16046236cc9772df62de72...
==========

[commit-bot: I haz the power, 2017-05-09 19:16:05]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3cf9dc16046236cc9772df62de72...