[Payment Request] Selector view controller in showcase

[Payment Request] Selector view controller in showcase

BUG=602666
TBR=lpromero

Review-Url: https://codereview.chromium.org/2821263002
Cr-Commit-Position: refs/heads/master@{#465294}
Committed: https://chromium.googlesource.com/chromium/src/+/e6d9e36f2aa62cb67c817113f64cead9ae89ca3b

[Moe, 2017-04-18 13:26:51]

Patchset #1 (id:1) has been deleted

[Moe, 2017-04-18 13:31:03]

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

[Moe, 2017-04-18 13:31:04]

Hi Gauthier. Please take a look.

PS. we have a slight divergence from the expected behavior here. Since the
alerter is delegate to the VC, it doesn't really update the mediator with the
selection that user made. So we end up with multiple selected rows. This doesn't
happen in chrome since the delegate updates the mediator accordingly.

[gambard, 2017-04-18 14:12:31]

lgtm

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_selector_coordinator.h (right):

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_selector_coordinator.h:12: @interface
SCPaymentsSelectorCoordinator : NSObject<NavigationCoordinator>
Comment

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_selector_coordinator.mm (right):

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_selector_coordinator.mm:19: @interface
TestPaymentsTextItem : PaymentsTextItem
If this is just to have an init method on PaymentsTextItem, maybe a C function
or a helper method on SCPaymentsSelectorCoordinator would be more appropriate?

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_selector_coordinator.mm:40:
@property(nonatomic, readwrite) NSUInteger selectedItemIndex;
Comment for all properties

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_selector_egtest.mm (right):

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_selector_egtest.mm:95:
@"protocol_alerter_done")]
Don't you want to check the method called on the delegate?

[Moe, 2017-04-18 16:44:11]

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

[Moe, 2017-04-18 16:44:11]

Thank you Gauthier.
TBR=lpromero

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_selector_coordinator.h (right):

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_selector_coordinator.h:12: @interface
SCPaymentsSelectorCoordinator : NSObject<NavigationCoordinator>
On 2017/04/18 14:12:30, gambard wrote:
> Comment

Done.

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_selector_coordinator.mm (right):

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_selector_coordinator.mm:19: @interface
TestPaymentsTextItem : PaymentsTextItem
On 2017/04/18 14:12:31, gambard wrote:
> If this is just to have an init method on PaymentsTextItem, maybe a C function
> or a helper method on SCPaymentsSelectorCoordinator would be more appropriate?

Done.

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_selector_coordinator.mm:40:
@property(nonatomic, readwrite) NSUInteger selectedItemIndex;
On 2017/04/18 14:12:30, gambard wrote:
> Comment for all properties

Done.

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_selector_egtest.mm (right):

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_selector_egtest.mm:95:
@"protocol_alerter_done")]
On 2017/04/18 14:12:31, gambard wrote:
> Don't you want to check the method called on the delegate?

Ideally yes. But I'm not sure with the alert text as is, how easy that would be:
http://imgur.com/a/JC6xG
If you have a solution in mind, I'd happily go back and update these.

[Moe, 2017-04-18 16:44:19]

Description was changed from

==========
[Payment Request] Selector view controller in showcase

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

to

==========
[Payment Request] Selector view controller in showcase

BUG=602666

TBR=lpromero
==========

[Moe, 2017-04-18 16:44:27]

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

[commit-bot: I haz the power, 2017-04-18 16:44:56]

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-04-18 18:03:01]

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

[commit-bot: I haz the power, 2017-04-18 18:03:02]

Dry run: This issue passed the CQ dry run.

[Moe, 2017-04-18 18:09:39]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-04-18 18:09:39]

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

[commit-bot: I haz the power, 2017-04-18 18:10:01]

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-18 18:14:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492538979021970,
"parent_rev": "bfddf62e413e4588c0f507a45b4bd4c4405bf2ef", "commit_rev":
"e6d9e36f2aa62cb67c817113f64cead9ae89ca3b"}

[commit-bot: I haz the power, 2017-04-18 18:14:41]

Description was changed from

==========
[Payment Request] Selector view controller in showcase

BUG=602666

TBR=lpromero
==========

to

==========
[Payment Request] Selector view controller in showcase

BUG=602666

TBR=lpromero

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

[commit-bot: I haz the power, 2017-04-18 18:14:42]

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

[lpromero, 2017-04-18 22:43:02]

lgtm

https://codereview.chromium.org/2821263002/diff/40001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_selector_egtest.mm (right):

https://codereview.chromium.org/2821263002/diff/40001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_selector_egtest.mm:136:
@"protocol_alerter_done")]
Maybe instead of the done button you could check the message in the alert?

[gambard, 2017-04-19 06:53:38]

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_selector_egtest.mm (right):

https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_selector_egtest.mm:95:
@"protocol_alerter_done")]
On 2017/04/18 16:44:11, moe wrote:
> On 2017/04/18 14:12:31, gambard wrote:
> > Don't you want to check the method called on the delegate?
> 
> Ideally yes. But I'm not sure with the alert text as is, how easy that would
be:
> http://imgur.com/a/JC6xG
> If you have a solution in mind, I'd happily go back and update these.

As lpromero suggested, one solution would be to check the text of the alert. If
don't think EG provide a way to test if a label's text begins with a specific
test. You might have to write your own matcher.

[Moe, 2017-04-19 22:41:14]

On 2017/04/19 06:53:38, gambard wrote:
>
https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
> File ios/showcase/payments/sc_payments_selector_egtest.mm (right):
> 
>
https://codereview.chromium.org/2821263002/diff/20001/ios/showcase/payments/s...
> ios/showcase/payments/sc_payments_selector_egtest.mm:95:
> @"protocol_alerter_done")]
> On 2017/04/18 16:44:11, moe wrote:
> > On 2017/04/18 14:12:31, gambard wrote:
> > > Don't you want to check the method called on the delegate?
> > 
> > Ideally yes. But I'm not sure with the alert text as is, how easy that would
> be:
> > http://imgur.com/a/JC6xG
> > If you have a solution in mind, I'd happily go back and update these.
> 
> As lpromero suggested, one solution would be to check the text of the alert.
If
> don't think EG provide a way to test if a label's text begins with a specific
> test. You might have to write your own matcher.

This CL should do it: https://codereview.chromium.org/2827163002/