[Payment Request] Populates region field if region code/name is valid.

[Payment Request] Populates region field if region code/name is valid.

In addition to comparing the autofill profile's region against avaialable
region codes and names, it also avoids settings a default region when
creating an address.

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

[Moe, 2017-06-20 15:35:36]

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

[Moe, 2017-06-20 15:35:37]

Hi Rouslan, Please take a look.

[please use gerrit instead, 2017-06-20 15:38:31]

Overall idea is good.

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

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/address_edit_mediator.mm:143:
self.regionField.value = [regions allKeysForObject:region][0];
Please get this reviewed by someone who can read Objective C :-P

[Moe, 2017-06-20 16:05:19]

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

[Moe, 2017-06-20 16:05:19]

For sure. As long as it's good payments/autofill-wise. MAC, Please take a look.

[macourteau, 2017-06-20 16:25:00]

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

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/address_edit_mediator.mm:140: if ([regions
objectForKey:region])
This block needs more documentation:
- what are the keys and values in regions?
- why is |region| used as either a key or as a value when searching in
|regions|?

My understanding after looking at this for too long: |regions| maps codes to
names. |region| could be either a code or a name, but self.regionField.value
needs to be a code. If we find the |region| key in |regions|, then set it's a
valid code and set it to that. Otherwise, if we find a value that matches
|region|, use the key for that value as the code.

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/address_edit_mediator.mm:142: else if ([[regions
allKeysForObject:region] count])
Might want to check that count returns exactly one, unless there's a valid
reason why it wouldn't? If if there is, how do we decide which key is the
correct one to use?

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

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/region_data_loader.h:24: -
(void)regionDataLoaderDidSucceedWithRegions:
Please document what mapping |regions| is.

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

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/region_data_loader.mm:47:
region_model->GetRegions()[i].second)
nit: extract |region_model->GetRegions()[i]| (or even region_model->GetRegions()
?)

[Moe, 2017-06-20 18:28:34]

Thanks MAC, PTAL?

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

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/address_edit_mediator.mm:140: if ([regions
objectForKey:region])
On 2017/06/20 at 16:24:59, macourteau wrote:
> This block needs more documentation:
> - what are the keys and values in regions?
> - why is |region| used as either a key or as a value when searching in
|regions|?
> 
> My understanding after looking at this for too long: |regions| maps codes to
names. |region| could be either a code or a name, but self.regionField.value
needs to be a code. If we find the |region| key in |regions|, then set it's a
valid code and set it to that. Otherwise, if we find a value that matches
|region|, use the key for that value as the code.

Done.

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/address_edit_mediator.mm:142: else if ([[regions
allKeysForObject:region] count])
On 2017/06/20 at 16:24:59, macourteau wrote:
> Might want to check that count returns exactly one, unless there's a valid
reason why it wouldn't? If if there is, how do we decide which key is the
correct one to use?

There should be a one to one mapping between region codes and region names.
autofill::RegionComboboxModel should be unit tested for that. If for some reason
there's a many to one mapping from region codes to region names, there is no way
for us to know which code is the correct code for the given name, so picking the
first one should be good enough.

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

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/region_data_loader.h:24: -
(void)regionDataLoaderDidSucceedWithRegions:
On 2017/06/20 at 16:25:00, macourteau wrote:
> Please document what mapping |regions| is.

Done.

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

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/region_data_loader.mm:47:
region_model->GetRegions()[i].second)
On 2017/06/20 at 16:25:00, macourteau wrote:
> nit: extract |region_model->GetRegions()[i]| (or even
region_model->GetRegions() ?)

Done.

[macourteau, 2017-06-20 18:31:30]

lgtm % one last request

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

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/address_edit_mediator.mm:142: else if ([[regions
allKeysForObject:region] count])
On 2017/06/20 18:28:33, Moe wrote:
> On 2017/06/20 at 16:24:59, macourteau wrote:
> > Might want to check that count returns exactly one, unless there's a valid
> reason why it wouldn't? If if there is, how do we decide which key is the
> correct one to use?
> 
> There should be a one to one mapping between region codes and region names.
> autofill::RegionComboboxModel should be unit tested for that. If for some
reason
> there's a many to one mapping from region codes to region names, there is no
way
> for us to know which code is the correct code for the given name, so picking
the
> first one should be good enough.  

Right, so then maybe add a DCHECK that count is only one?

[Moe, 2017-06-20 18:58:34]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-06-20 18:58:35]

Thank you.

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

https://codereview.chromium.org/2950843002/diff/1/ios/chrome/browser/ui/payme...
ios/chrome/browser/ui/payments/address_edit_mediator.mm:143:
self.regionField.value = [regions allKeysForObject:region][0];
On 2017/06/20 at 15:38:31, rouslan wrote:
> Please get this reviewed by someone who can read Objective C :-P

Done.

[Moe, 2017-06-20 18:58:35]

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/2950843002/#ps40001
(title: "Addressed comment")

[commit-bot: I haz the power, 2017-06-20 18:59:09]

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-06-20 19:36:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497985114652360,
"parent_rev": "593bb862a1ed514e4c0f592bb79df5d913848829", "commit_rev":
"fc05aaf082afa71c49baf57b856e56cad356ab9f"}

[commit-bot: I haz the power, 2017-06-20 19:36:18]

Description was changed from

==========
[Payment Request] Populates region field if region code/name is valid.

In addition to comparing the autofill profile's region against avaialable
region codes and names, it also avoids settings a default region when
creating an address.

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

to

==========
[Payment Request] Populates region field if region code/name is valid.

In addition to comparing the autofill profile's region against avaialable
region codes and names, it also avoids settings a default region when
creating an address.

BUG=602666

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

[commit-bot: I haz the power, 2017-06-20 19:36:21]

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