Selected shipping option in payment summary view + shipping option selection view

Selected shipping option in payment summary view + shipping option selection view

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

[Moe, 2017-01-07 22:37:28]

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

[commit-bot: I haz the power, 2017-01-07 22:37: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-01-08 00:15:11]

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

[commit-bot: I haz the power, 2017-01-08 00:15:12]

Dry run: This issue passed the CQ dry run.

[Moe, 2017-01-09 00:11:19]

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

[commit-bot: I haz the power, 2017-01-09 00:11:28]

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

[Moe, 2017-01-09 00:20:22]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-01-09 01:03:32]

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

[commit-bot: I haz the power, 2017-01-09 01:03:33]

Dry run: This issue passed the CQ dry run.

[Moe, 2017-01-09 15:14:38]

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

[Moe, 2017-01-09 15:14:39]

Hi Justin, Please review this CL.

[Justin Donnelly, 2017-01-10 17:11:52]

jdonnelly@chromium.org changed reviewers:
+ lpromero@chromium.org

[Justin Donnelly, 2017-01-10 17:11:53]

In general, looks good, just a few nits. +lpromero for his review.

Moving forward, I think you ought to start sending your payments CLs directly to
him. I'm pretty focused on omnibox now so I'm just adding additional latency to
your code reviews at this point.

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

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_coordinator.mm:84: // The selected
shipping option must be the last shipping option with
At first, I thought you were saying here that the last option in the list had to
have selected set. How about wording this more like how the spec puts it?
Something like, "If more than one option has |selected| set, the last one in the
sequence should be treated as the selected item."

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_coordinator.mm:249:
[_shippingOptionSelectionCoordinator setShippingOptions:shippingOptions];
Why not just pass _paymentRequest.details.shipping_options here (as we do with
_paymentRequest.details.display_items above)? If you're just trying to pass
pointers instead of copies of these objects, I'd strongly prefer to avoid that.
It complicates the code for no meaningful efficiency gain.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_address_selection_view_controller.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_address_selection_view_controller.mm:145:
NSIndexPath* currentlySeletedIndexPath = [self.collectionViewModel
s/Seleted/Selected/

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_option_selection_coordinator.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:84:
_viewController.get().view.userInteractionEnabled = YES;
Why re-enable interaction here? Wouldn't it be safer to leave it disabled?

[lpromero, 2017-01-11 12:51:15]

lgtm

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_option_selection_coordinator.h
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.h:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
2017. You can use boilerplate.py to create the boilerplate for new files (maybe
you did start these files in 2016?).

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.h:18: -
(void)shippingOptionSelectionCoordinator:
Comments for these two methods.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_option_selection_coordinator.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
2017, here and in every file.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:46:
DCHECK(self.baseViewController.navigationController);
No action needed: We never need baseViewController for anything else than its
navigationController, right? We should consider having the baseViewController be
the navigationController itself. I will think about it for the future.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:84:
_viewController.get().view.userInteractionEnabled = YES;
On 2017/01/10 17:11:53, Justin Donnelly wrote:
> Why re-enable interaction here? Wouldn't it be safer to leave it disabled?

Why not, good idea.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File
ios/chrome/browser/payments/shipping_option_selection_coordinator_unittest.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator_unittest.mm:39:
}
New line before and // namespace

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_option_selection_view_controller.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_view_controller.mm:125:
#pragma mark UICollectionViewDataSource
Add new line after.

[Moe, 2017-01-12 00:05:39]

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

[Moe, 2017-01-12 00:06:19]

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

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_coordinator.mm:84: // The selected
shipping option must be the last shipping option with
On 2017/01/10 17:11:53, Justin Donnelly wrote:
> At first, I thought you were saying here that the last option in the list had
to
> have selected set. How about wording this more like how the spec puts it?
> Something like, "If more than one option has |selected| set, the last one in
the
> sequence should be treated as the selected item."

Done.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_coordinator.mm:249:
[_shippingOptionSelectionCoordinator setShippingOptions:shippingOptions];
On 2017/01/10 17:11:53, Justin Donnelly wrote:
> Why not just pass _paymentRequest.details.shipping_options here (as we do with
> _paymentRequest.details.display_items above)? If you're just trying to pass
> pointers instead of copies of these objects, I'd strongly prefer to avoid
that.
> It complicates the code for no meaningful efficiency gain.

The purpose for that wasn't efficiency gain. I wanted to keep the implementation
as close as possible to the shipping address selection view/logic since the two
are extremely similar. I think it makes the maintenance easier too as the two
will likely have similar bugs and features can literally be copy/pasted across.
Also I think this a is slightly different case than the display items because
that doesn't deal with selection and communicating it back. I find it more
straightforward to work with pointers than values when keeping track of a an
item in a list of items. If you agree, ever so slightly, with those reasons
let's keep it this way? :)

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_address_selection_view_controller.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_address_selection_view_controller.mm:145:
NSIndexPath* currentlySeletedIndexPath = [self.collectionViewModel
On 2017/01/10 17:11:53, Justin Donnelly wrote:
> s/Seleted/Selected/

Done.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_option_selection_coordinator.h
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.h:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/11 12:51:15, lpromero wrote:
> 2017. You can use boilerplate.py to create the boilerplate for new files
(maybe
> you did start these files in 2016?).

Today I learned.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.h:18: -
(void)shippingOptionSelectionCoordinator:
On 2017/01/11 12:51:15, lpromero wrote:
> Comments for these two methods.

Done.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_option_selection_coordinator.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/11 12:51:15, lpromero wrote:
> 2017, here and in every file.

Done.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:46:
DCHECK(self.baseViewController.navigationController);
On 2017/01/11 12:51:15, lpromero wrote:
> No action needed: We never need baseViewController for anything else than its
> navigationController, right? We should consider having the baseViewController
be
> the navigationController itself. I will think about it for the future.

Acknowledged.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File
ios/chrome/browser/payments/shipping_option_selection_coordinator_unittest.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator_unittest.mm:39:
}
On 2017/01/11 12:51:15, lpromero wrote:
> New line before and // namespace

Done.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_option_selection_view_controller.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_view_controller.mm:125:
#pragma mark UICollectionViewDataSource
On 2017/01/11 12:51:15, lpromero wrote:
> Add new line after.

Done.

[commit-bot: I haz the power, 2017-01-12 00:06:56]

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

[Justin Donnelly, 2017-01-12 00:23:29]

lgtm but please address the one remaining comment

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_option_selection_coordinator.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:84:
_viewController.get().view.userInteractionEnabled = YES;
On 2017/01/11 12:51:15, lpromero wrote:
> On 2017/01/10 17:11:53, Justin Donnelly wrote:
> > Why re-enable interaction here? Wouldn't it be safer to leave it disabled?
> 
> Why not, good idea.

What about this comment?

[commit-bot: I haz the power, 2017-01-12 01:31:29]

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

[commit-bot: I haz the power, 2017-01-12 01:31:30]

Dry run: This issue passed the CQ dry run.

[Moe, 2017-01-12 14:32:26]

Thank you Justin for the reviews and helpful resources you put together for the
Payment Request. I agree. Moving forward it makes sense to have Rouslan or
Mathieu take a look at the CLs from a Payment Request logic perspective and
continue having Louis as the main reviewer. Also thank you Louis for the
continued support and reviews.

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_option_selection_coordinator.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:84:
_viewController.get().view.userInteractionEnabled = YES;
On 2017/01/12 00:23:29, Justin Donnelly wrote:
> On 2017/01/11 12:51:15, lpromero wrote:
> > On 2017/01/10 17:11:53, Justin Donnelly wrote:
> > > Why re-enable interaction here? Wouldn't it be safer to leave it disabled?
> > 
> > Why not, good idea.
> 
> What about this comment?

I copied this logic from bookmark_folder_view_controller.mm.
I think it really doesn't matters to if we leave it disabled or reenable it,
since the view gets popped immediately. This in a way looks like any other
coordinator returning to its delegate. One could argue shouldn't we disable user
interaction before returning from any coordinator?

[Moe, 2017-01-12 14:32:32]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-01-12 14:32:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2621453002/#ps40001
(title: "Addressed comments by lpromero@ and jdonnelley@")

[commit-bot: I haz the power, 2017-01-12 14:32:52]

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-01-12 14:38:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484231552066730,
"parent_rev": "2555903a7553ec68f9114df16e6d442ef389a111", "commit_rev":
"9fb7181fd5cf504dc37aae3775c73c81e2fd4336"}

[commit-bot: I haz the power, 2017-01-12 14:38:31]

Description was changed from

==========
Selected shipping option in payment summary view + shipping option selection
view

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

to

==========
Selected shipping option in payment summary view + shipping option selection
view

BUG=602666

Review-Url: https://codereview.chromium.org/2621453002
Cr-Commit-Position: refs/heads/master@{#443226}
Committed:
https://chromium.googlesource.com/chromium/src/+/9fb7181fd5cf504dc37aae3775c7...
==========

[commit-bot: I haz the power, 2017-01-12 14:38:33]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9fb7181fd5cf504dc37aae3775c7...

[Justin Donnelly, 2017-01-12 16:05:11]

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/shipping_option_selection_coordinator.mm
(right):

https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:84:
_viewController.get().view.userInteractionEnabled = YES;
On 2017/01/12 14:32:26, moe wrote:
> On 2017/01/12 00:23:29, Justin Donnelly wrote:
> > On 2017/01/11 12:51:15, lpromero wrote:
> > > On 2017/01/10 17:11:53, Justin Donnelly wrote:
> > > > Why re-enable interaction here? Wouldn't it be safer to leave it
disabled?
> > > 
> > > Why not, good idea.
> > 
> > What about this comment?
> 
> I copied this logic from bookmark_folder_view_controller.mm.
> I think it really doesn't matters to if we leave it disabled or reenable it,
> since the view gets popped immediately. This in a way looks like any other
> coordinator returning to its delegate. One could argue shouldn't we disable
user
> interaction before returning from any coordinator?

I'm having trouble parsing your last statement. You're saying you think we
should disable user interaction?

If so, yes, that's what I'm arguing. :) This line is enabling it. You're right
that it doesn't currently matter but in case anything should change in the
future I'm suggesting it would be safest to not enable interaction here.

[Moe, 2017-01-12 16:14:12]

On 2017/01/12 16:05:11, Justin Donnelly wrote:
>
https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
> File ios/chrome/browser/payments/shipping_option_selection_coordinator.mm
> (right):
> 
>
https://codereview.chromium.org/2621453002/diff/20001/ios/chrome/browser/paym...
> ios/chrome/browser/payments/shipping_option_selection_coordinator.mm:84:
> _viewController.get().view.userInteractionEnabled = YES;
> On 2017/01/12 14:32:26, moe wrote:
> > On 2017/01/12 00:23:29, Justin Donnelly wrote:
> > > On 2017/01/11 12:51:15, lpromero wrote:
> > > > On 2017/01/10 17:11:53, Justin Donnelly wrote:
> > > > > Why re-enable interaction here? Wouldn't it be safer to leave it
> disabled?
> > > > 
> > > > Why not, good idea.
> > > 
> > > What about this comment?
> > 
> > I copied this logic from bookmark_folder_view_controller.mm.
> > I think it really doesn't matters to if we leave it disabled or reenable it,
> > since the view gets popped immediately. This in a way looks like any other
> > coordinator returning to its delegate. One could argue shouldn't we disable
> user
> > interaction before returning from any coordinator?
> 
> I'm having trouble parsing your last statement. You're saying you think we
> should disable user interaction?
> 
> If so, yes, that's what I'm arguing. :) This line is enabling it. You're right
> that it doesn't currently matter but in case anything should change in the
> future I'm suggesting it would be safest to not enable interaction here.

yes, I agree! I was reiterating your point that it's probably safer to disable
user interactions before *any* coordinator returns to its delegate. The reason
being we don't know what's happening in the delegate and when it is going to
stop the coordinator and if it's going to do so at all. As far as this
coordinator is concerned the user is done interacting with its view. 
But since we don't do it elsewhere, I argued we can re-enable user interaction
and be consistent with the other coordinators.

[lpromero, 2017-01-12 16:49:05]

It's true that reenabling user interactions keeps things more balanced and
doesn't propagate assumptions. Let's keep it for now.