[Payment Request] Refactors the edit view controller

[Payment Request] Refactors the edit view controller

The edit view controller now handles interleaved selector-backed fields
and text fields.
Now the billing address field doesn't have to be treated specially by
the subclass credit card view controller. This allows for more code to
be moved to the superclass.

BUG=602666
Review-Url: https://codereview.chromium.org/2876603005
Cr-Commit-Position: refs/heads/master@{#472069}
Committed: https://chromium.googlesource.com/chromium/src/+/68175653b309c2d4df784d3a9530bf03f28411b2

[Moe, 2017-05-11 15:38:20]

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

[Moe, 2017-05-11 15:38:21]

Hi MAC, Please take a look.

[macourteau, 2017-05-11 18:59:40]

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File ios/chrome/browser/ui/payments/credit_card_edit_coordinator_unittest.mm
(right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_coordinator_unittest.mm:73: 
nit: blank line?

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File ios/chrome/browser/ui/payments/credit_card_edit_mediator.mm (right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_mediator.mm:87: } else {
nit: get rid of the else {}, since the 'if' always returns if it enters the
statement (also, it's weird that there's some code in the else and some after,
when both always get executed together)

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_mediator.mm:172: // Only the
billing address of server credit cards can be edited.
nit: this comment had me stumped for a while, maybe reword it as "for server
cards, only the billing address can be edited" (I parsed as that "only [the
billing address of server cards] can be edited", and not as "only [the billing
address [of server cards]] can be edited")

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File ios/chrome/browser/ui/payments/credit_card_edit_view_controller_unittest.mm
(right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_view_controller_unittest.mm:68:
[[EditorField alloc] initWithAutofillUIType:AutofillUITypeCreditCardExpYear
ExpYear?

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_view_controller_unittest.mm:115:
ASSERT_EQ(7, NumberOfSections());
Please update comment, which describes 8 sections :)

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File ios/chrome/browser/ui/payments/payment_request_edit_view_controller.mm
(right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/payment_request_edit_view_controller.mm:226:
EditorField* dummyField =
nit: dummyField -> fieldForValidation or something like that?

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/payment_request_edit_view_controller.mm:405:
while (nextCellPath) {
optional nit: this could be made into a for loop (there's initialization above,
condition here and assignment below)

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File
ios/chrome/browser/ui/payments/payment_request_edit_view_controller_data_source.h
(right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/payment_request_edit_view_controller_data_source.h:30:
// Returns weather the header item should hide its background.
weather -> whether

[macourteau, 2017-05-11 19:05:53]

Also, seems like there's a few different things going on in here. Maybe, in
future CL, that could be split in smaller CLs?

[Moe, 2017-05-11 19:12:49]

Patchset #2 (id:20001) has been deleted

[Moe, 2017-05-11 19:37:39]

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

[commit-bot: I haz the power, 2017-05-11 19:38:52]

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-11 19:39:38]

Thank you.

I took the new payment request editor unit test into its own CL. Other changes
in the CL are a result of the refactor.

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File ios/chrome/browser/ui/payments/credit_card_edit_coordinator_unittest.mm
(right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_coordinator_unittest.mm:73: 
On 2017/05/11 18:59:39, macourteau wrote:
> nit: blank line?

Done.

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File ios/chrome/browser/ui/payments/credit_card_edit_mediator.mm (right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_mediator.mm:87: } else {
On 2017/05/11 18:59:39, macourteau wrote:
> nit: get rid of the else {}, since the 'if' always returns if it enters the
> statement (also, it's weird that there's some code in the else and some after,
> when both always get executed together)

Done. Not sure If I understand the comment in parentheses.

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_mediator.mm:172: // Only the
billing address of server credit cards can be edited.
On 2017/05/11 18:59:39, macourteau wrote:
> nit: this comment had me stumped for a while, maybe reword it as "for server
> cards, only the billing address can be edited" (I parsed as that "only [the
> billing address of server cards] can be edited", and not as "only [the billing
> address [of server cards]] can be edited")

Done.

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File ios/chrome/browser/ui/payments/credit_card_edit_view_controller_unittest.mm
(right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_view_controller_unittest.mm:68:
[[EditorField alloc] initWithAutofillUIType:AutofillUITypeCreditCardExpYear
On 2017/05/11 18:59:40, macourteau wrote:
> ExpYear?

Done.

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_view_controller_unittest.mm:115:
ASSERT_EQ(7, NumberOfSections());
On 2017/05/11 18:59:40, macourteau wrote:
> Please update comment, which describes 8 sections :)

Done.

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File ios/chrome/browser/ui/payments/payment_request_edit_view_controller.mm
(right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/payment_request_edit_view_controller.mm:226:
EditorField* dummyField =
On 2017/05/11 18:59:40, macourteau wrote:
> nit: dummyField -> fieldForValidation or something like that?

Done.

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/payment_request_edit_view_controller.mm:405:
while (nextCellPath) {
On 2017/05/11 18:59:40, macourteau wrote:
> optional nit: this could be made into a for loop (there's initialization
above,
> condition here and assignment below)

Acknowledged. The for loop syntax isn't the most intuitive for things other than
a simple index incrementation. I find this easier to follow.

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File
ios/chrome/browser/ui/payments/payment_request_edit_view_controller_data_source.h
(right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/payment_request_edit_view_controller_data_source.h:30:
// Returns weather the header item should hide its background.
On 2017/05/11 18:59:40, macourteau wrote:
> weather -> whether

Done. I blame this on the weather :)

[commit-bot: I haz the power, 2017-05-11 19:54:21]

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

[commit-bot: I haz the power, 2017-05-11 19:54:22]

Dry run: This issue passed the CQ dry run.

[macourteau, 2017-05-11 19:58:24]

lgtm

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File ios/chrome/browser/ui/payments/credit_card_edit_mediator.mm (right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/credit_card_edit_mediator.mm:87: } else {
On 2017/05/11 19:39:38, Moe wrote:
> On 2017/05/11 18:59:39, macourteau wrote:
> > nit: get rid of the else {}, since the 'if' always returns if it enters the
> > statement (also, it's weird that there's some code in the else and some
after,
> > when both always get executed together)
> 
> Done. Not sure If I understand the comment in parentheses. 

Never mind that comment, didn't match the braces correctly in my head. /facepalm

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File ios/chrome/browser/ui/payments/payment_request_edit_view_controller.mm
(right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/payment_request_edit_view_controller.mm:405:
while (nextCellPath) {
On 2017/05/11 19:39:38, Moe wrote:
> On 2017/05/11 18:59:40, macourteau wrote:
> > optional nit: this could be made into a for loop (there's initialization
> above,
> > condition here and assignment below)
> 
> Acknowledged. The for loop syntax isn't the most intuitive for things other
than
> a simple index incrementation. I find this easier to follow.

Acknowledged.

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
File
ios/chrome/browser/ui/payments/payment_request_edit_view_controller_data_source.h
(right):

https://codereview.chromium.org/2876603005/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/payment_request_edit_view_controller_data_source.h:30:
// Returns weather the header item should hide its background.
On 2017/05/11 19:39:38, Moe wrote:
> On 2017/05/11 18:59:40, macourteau wrote:
> > weather -> whether
> 
> Done. I blame this on the weather :)

I can understand!

[Moe, 2017-05-11 20:01:11]

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

[Moe, 2017-05-11 20:01:12]

Hi Ed, could you please take a look at ios/showcase?

[edchin, 2017-05-15 17:38:17]

lgtm on showcase with minor comment.

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

https://codereview.chromium.org/2876603005/diff/40001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_editor_coordinator.mm:53:
setDelegate:reinterpret_cast<
I think this should be a static_cast.

[Moe, 2017-05-15 18:36:42]

Thank you.

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

https://codereview.chromium.org/2876603005/diff/40001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_editor_coordinator.mm:53:
setDelegate:reinterpret_cast<
On 2017/05/15 17:38:17, edchin wrote:
> I think this should be a static_cast.

Done.

[Moe, 2017-05-16 11:26:41]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-05-16 11:26:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
macourteau@chromium.org, edchin@chromium.org
Link to the patchset: https://codereview.chromium.org/2876603005/#ps60001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-05-16 11:27: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-05-16 11:48:48]

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

[commit-bot: I haz the power, 2017-05-16 11:48:49]

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-16 12:50:10]

The CQ bit was checked by mahmadi@chromium.org

[commit-bot: I haz the power, 2017-05-16 12:50:32]

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-16 12:55:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494939010647190,
"parent_rev": "134190a648fe48fed0486cf8889569863e4a544c", "commit_rev":
"68175653b309c2d4df784d3a9530bf03f28411b2"}

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

Description was changed from

==========
[Payment Request] Refactors the edit view controller

The edit view controller now handles interleaved selector-backed fields
and text fields.
Now the billing address field doesn't have to be treated specially by
the subclass credit card view controller. This allows for more code to
be moved to the superclass.

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

to

==========
[Payment Request] Refactors the edit view controller

The edit view controller now handles interleaved selector-backed fields
and text fields.
Now the billing address field doesn't have to be treated specially by
the subclass credit card view controller. This allows for more code to
be moved to the superclass.

BUG=602666

Review-Url: https://codereview.chromium.org/2876603005
Cr-Commit-Position: refs/heads/master@{#472069}
Committed:
https://chromium.googlesource.com/chromium/src/+/68175653b309c2d4df784d3a9530...
==========

[commit-bot: I haz the power, 2017-05-16 12:55:20]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/68175653b309c2d4df784d3a9530...