[Payment Request] Refactors the remaining selector view controllers

[Payment Request] Refactors the remaining selector view controllers

This CL (which is a continuation of https://codereview.chromium.org/2827453004)
replaces the following view controllers with PaymentRequestSelectorViewController:
* ShippingOptionSelectionViewController
* PaymentMethodSelectionViewController

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

[Moe, 2017-04-18 18:14:43]

Description was changed from

==========
[Payment Request] Refactors the remaining selector view controllers

Replaces the following selector view controllers with the
PaymentRequestSelectorViewController:
* ShippingOptionSelectionViewController
* PaymentMethodSelectionViewController

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

to

==========
[Payment Request] Refactors the remaining selector view controllers

This CL (which is a continuation of https://codereview.chromium.org/2827453004)
replaces the following view controllers with
PaymentRequestSelectorViewController:
* ShippingOptionSelectionViewController
* PaymentMethodSelectionViewController

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

[Moe, 2017-04-18 18:15:55]

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

[Moe, 2017-04-18 18:15:55]

Hi MAC, Please take a look at this CL.

[macourteau, 2017-04-19 18:03:13]

A few comments & questions

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_method_selection_coordinator.mm:43:
self.mediator = [[PaymentMethodSelectionMediator alloc]
Any reason we're accessing these through the getters/setters and not mutating
the variables directly?

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_method_selection_coordinator.mm:118:
static_cast<int64_t>(0.2 * NSEC_PER_SEC)),
nit: maybe make this a constant at top of file?

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_method_selection_coordinator_unittest.mm:95:
autofill::CreditCard* credit_card = payment_request_->credit_cards()[1];
Why index 1 and not index 0?

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:103:
NOTREACHED() << "This should not get called.";
What about moving this into the base class, and not override it if it's not
supported?

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/shipping_option_selection_coordinator_unittest.mm:88:
web::PaymentShippingOption* option = payment_request_->shipping_options()[1];
Why index 1?

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/shipping_option_selection_mediator.mm:40:
@synthesize headerText = _headerText;
nit: Should this list match order of declaration in .h/.mm files?

[Moe, 2017-04-19 20:12:44]

Hi, PTAL.

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_method_selection_coordinator.mm:43:
self.mediator = [[PaymentMethodSelectionMediator alloc]
On 2017/04/19 18:03:13, macourteau wrote:
> Any reason we're accessing these through the getters/setters and not mutating
> the variables directly?

It's a matter of Objective-C style guide. It's safer and more "correct" to
access the properties through accessors than access the synthesized properties
directly, ***unless you're in -init or the function you're in can be called from
-init***. The reason is that there could be things going on behind the scenes
such as readonly-ness, copying, etc that you could bypass by accessing the
synthesized properties directly. some reviewers are more strict about this than
others and as you can see the older code doesn't follow this pattern. We should
write new code following this pattern or at the very minimum not mix and match!

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_method_selection_coordinator.mm:118:
static_cast<int64_t>(0.2 * NSEC_PER_SEC)),
On 2017/04/19 18:03:13, macourteau wrote:
> nit: maybe make this a constant at top of file?

Done.

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_method_selection_coordinator_unittest.mm:95:
autofill::CreditCard* credit_card = payment_request_->credit_cards()[1];
On 2017/04/19 18:03:13, macourteau wrote:
> Why index 1 and not index 0?

Because of the following call:
  [coordinator paymentRequestSelectorViewController:view_controller
                               didSelectItemAtIndex:1];
No specific reason for using 1 instead of 0. The numbers need to match. that's
all.

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:103:
NOTREACHED() << "This should not get called.";
On 2017/04/19 18:03:13, macourteau wrote:
> What about moving this into the base class, and not override it if it's not
> supported?

There's no base class to move this to. Any coordinator that implements the
PaymentRequestSelectorViewControllerDelegate protocol needs to handle all the
methods. PaymentRequestSelectorViewController is a generic view controller that
does provide an optional "Add" button and this is its respective delegate
function. But since this coordinator doesn't set up the view controller to have
an "Add" button, this function should never gets called.

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/shipping_option_selection_coordinator_unittest.mm:88:
web::PaymentShippingOption* option = payment_request_->shipping_options()[1];
On 2017/04/19 18:03:13, macourteau wrote:
> Why index 1?

ditto.

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/shipping_option_selection_mediator.mm:40:
@synthesize headerText = _headerText;
On 2017/04/19 18:03:13, macourteau wrote:
> nit: Should this list match order of declaration in .h/.mm files?

Yes! Done.

[macourteau, 2017-04-20 14:43:00]

lgtm w/ one question still

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_method_selection_coordinator.mm:43:
self.mediator = [[PaymentMethodSelectionMediator alloc]
On 2017/04/19 20:12:43, moe wrote:
> On 2017/04/19 18:03:13, macourteau wrote:
> > Any reason we're accessing these through the getters/setters and not
mutating
> > the variables directly?
> 
> It's a matter of Objective-C style guide. It's safer and more "correct" to
> access the properties through accessors than access the synthesized properties
> directly, ***unless you're in -init or the function you're in can be called
from
> -init***. The reason is that there could be things going on behind the scenes
> such as readonly-ness, copying, etc that you could bypass by accessing the
> synthesized properties directly. some reviewers are more strict about this
than
> others and as you can see the older code doesn't follow this pattern. We
should
> write new code following this pattern or at the very minimum not mix and
match!

Ha, interesting. I would have thought that direct access made more sense from
within the class, but I guess this makes sense. Thanks for the clarification1

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_method_selection_coordinator_unittest.mm:95:
autofill::CreditCard* credit_card = payment_request_->credit_cards()[1];
On 2017/04/19 20:12:43, moe wrote:
> On 2017/04/19 18:03:13, macourteau wrote:
> > Why index 1 and not index 0?
> 
> Because of the following call:
>   [coordinator paymentRequestSelectorViewController:view_controller
>                                didSelectItemAtIndex:1];
> No specific reason for using 1 instead of 0. The numbers need to match. that's
> all.

Acknowledged.

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:103:
NOTREACHED() << "This should not get called.";
On 2017/04/19 20:12:44, moe wrote:
> On 2017/04/19 18:03:13, macourteau wrote:
> > What about moving this into the base class, and not override it if it's not
> > supported?
> 
> There's no base class to move this to. Any coordinator that implements the
> PaymentRequestSelectorViewControllerDelegate protocol needs to handle all the
> methods. PaymentRequestSelectorViewController is a generic view controller
that
> does provide an optional "Add" button and this is its respective delegate
> function. But since this coordinator doesn't set up the view controller to
have
> an "Add" button, this function should never gets called.

TIL about protocols in Objective-C. :)

n00b question: can this method be in an @optional section in the protocol, and
then use respondsToSelector: to figure out if it supports an Add item?

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/shipping_option_selection_coordinator_unittest.mm:88:
web::PaymentShippingOption* option = payment_request_->shipping_options()[1];
On 2017/04/19 20:12:44, moe wrote:
> On 2017/04/19 18:03:13, macourteau wrote:
> > Why index 1?
> 
> ditto.

Ack.

[Moe, 2017-04-20 15:23:30]

Thanks.

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

https://codereview.chromium.org/2826713002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:103:
NOTREACHED() << "This should not get called.";
On 2017/04/20 14:42:59, macourteau wrote:
> On 2017/04/19 20:12:44, moe wrote:
> > On 2017/04/19 18:03:13, macourteau wrote:
> > > What about moving this into the base class, and not override it if it's
not
> > > supported?
> > 
> > There's no base class to move this to. Any coordinator that implements the
> > PaymentRequestSelectorViewControllerDelegate protocol needs to handle all
the
> > methods. PaymentRequestSelectorViewController is a generic view controller
> that
> > does provide an optional "Add" button and this is its respective delegate
> > function. But since this coordinator doesn't set up the view controller to
> have
> > an "Add" button, this function should never gets called.
> 
> TIL about protocols in Objective-C. :)
> 
> n00b question: can this method be in an @optional section in the protocol, and
> then use respondsToSelector: to figure out if it supports an Add item?

Good idea! TIL too!

[Moe, 2017-04-20 15:23:36]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-04-20 15:23:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
macourteau@chromium.org
Link to the patchset: https://codereview.chromium.org/2826713002/#ps40001
(title: "Addressed comment")

[commit-bot: I haz the power, 2017-04-20 15:24:17]

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-04-20 17:12:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492701816201220,
"parent_rev": "d3ca34febe2838703bfd71217be83fb26e372257", "commit_rev":
"5b23f583c5014cba0aad2e6837d64fe60e694d37"}

[commit-bot: I haz the power, 2017-04-20 17:12:30]

Description was changed from

==========
[Payment Request] Refactors the remaining selector view controllers

This CL (which is a continuation of https://codereview.chromium.org/2827453004)
replaces the following view controllers with
PaymentRequestSelectorViewController:
* ShippingOptionSelectionViewController
* PaymentMethodSelectionViewController

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

to

==========
[Payment Request] Refactors the remaining selector view controllers

This CL (which is a continuation of https://codereview.chromium.org/2827453004)
replaces the following view controllers with
PaymentRequestSelectorViewController:
* ShippingOptionSelectionViewController
* PaymentMethodSelectionViewController

BUG=602666

Review-Url: https://codereview.chromium.org/2826713002
Cr-Commit-Position: refs/heads/master@{#466033}
Committed:
https://chromium.googlesource.com/chromium/src/+/5b23f583c5014cba0aad2e6837d6...
==========

[commit-bot: I haz the power, 2017-04-20 17:12:31]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5b23f583c5014cba0aad2e6837d6...