[Payments] Move parsing the PaymentMethodData to "core"

[Payments] Move parsing the PaymentMethodData to "core"

This will allow it to be used on iOS as well. We now copy the
mojom::PaymentMethodData entries to a common type
(PaymentMethodData) before parsing.

BUG=708052
TEST=existing components_unittests (PaymentRequestSpec)

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

[Mathieu, 2017-04-05 17:31:22]

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

[Mathieu, 2017-04-05 17:31:22]

Hi Rouslan, can you have a look? Thanks

[Mathieu, 2017-04-05 17:34:32]

Description was changed from

==========
[Payments] Move parsing the PaymentMethodData to common location

BUG=708052
TEST=existing components_unittests (PaymentRequestSpec)
==========

to

==========
[Payments] Move parsing the PaymentMethodData to "core"

This will allow it to be used on iOS as well.

BUG=708052
TEST=existing components_unittests (PaymentRequestSpec)
==========

[Mathieu, 2017-04-05 17:35:15]

Description was changed from

==========
[Payments] Move parsing the PaymentMethodData to "core"

This will allow it to be used on iOS as well.

BUG=708052
TEST=existing components_unittests (PaymentRequestSpec)
==========

to

==========
[Payments] Move parsing the PaymentMethodData to "core"

This will allow it to be used on iOS as well. We now copy the
mojom::PaymentMethodData entries to a common type
(PaymentMethodData) before parsing.

BUG=708052
TEST=existing components_unittests (PaymentRequestSpec)
==========

[please use gerrit instead, 2017-04-05 19:29:25]

lgtm % comments

https://codereview.chromium.org/2800713003/diff/20001/components/payments/con...
File components/payments/content/payment_request_spec.cc (right):

https://codereview.chromium.org/2800713003/diff/20001/components/payments/con...
components/payments/content/payment_request_spec.cc:22: return "amex";
Can we define an enum in components/payments/core/ and use that instead of
strings?

https://codereview.chromium.org/2800713003/diff/20001/components/payments/con...
components/payments/content/payment_request_spec.cc:148:
method_data.supported_networks = std::move(supported_networks);
What happens when moving a vector of strings, exactly?

https://codereview.chromium.org/2800713003/diff/20001/components/payments/cor...
File components/payments/core/payment_request_data_util.cc (right):

https://codereview.chromium.org/2800713003/diff/20001/components/payments/cor...
components/payments/core/payment_request_data_util.cc:23: const char
kBasicCardMethodName[] = "basic-card";
Nit: place immediately before where this is used, because it's used only once.

[Mathieu, 2017-04-05 20:28:33]

Thanks

https://codereview.chromium.org/2800713003/diff/20001/components/payments/con...
File components/payments/content/payment_request_spec.cc (right):

https://codereview.chromium.org/2800713003/diff/20001/components/payments/con...
components/payments/content/payment_request_spec.cc:22: return "amex";
On 2017/04/05 19:29:25, ಠ_ಠ wrote:
> Can we define an enum in components/payments/core/ and use that instead of
> strings?

There is still a moment where we need to convert to string to pass back to the
merchant in the PaymentResponse (say supported_methods = ["visa"]). One way or
another we will need a function like this, I believe.

https://codereview.chromium.org/2800713003/diff/20001/components/payments/con...
components/payments/content/payment_request_spec.cc:148:
method_data.supported_networks = std::move(supported_networks);
On 2017/04/05 19:29:25, ಠ_ಠ wrote:
> What happens when moving a vector of strings, exactly?

I'm moving this local variable to avoid a copy, essentially, since we know that
beyond this line, |supported_networks| is not needed.

https://codereview.chromium.org/2800713003/diff/20001/components/payments/cor...
File components/payments/core/payment_request_data_util.cc (right):

https://codereview.chromium.org/2800713003/diff/20001/components/payments/cor...
components/payments/core/payment_request_data_util.cc:23: const char
kBasicCardMethodName[] = "basic-card";
On 2017/04/05 19:29:25, ಠ_ಠ wrote:
> Nit: place immediately before where this is used, because it's used only once.

Done.

[Mathieu, 2017-04-05 20:32:44]

https://codereview.chromium.org/2800713003/diff/20001/components/payments/con...
File components/payments/content/payment_request_spec.cc (right):

https://codereview.chromium.org/2800713003/diff/20001/components/payments/con...
components/payments/content/payment_request_spec.cc:22: return "amex";
On 2017/04/05 20:28:33, Mathieu wrote:
> On 2017/04/05 19:29:25, ಠ_ಠ wrote:
> > Can we define an enum in components/payments/core/ and use that instead of
> > strings?
> 
> There is still a moment where we need to convert to string to pass back to the
> merchant in the PaymentResponse (say supported_methods = ["visa"]). One way or
> another we will need a function like this, I believe.

Actually this string here doesn't make its way to the PaymentResponse. But, it
eventually gets compared as part of autofill_data_util [1] with types in
CreditCard, and so forth. We could convert to enums some of the way, but
eventually we will need to go to std::string for the PaymentResponse.


[1]
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...

[Mathieu, 2017-04-05 20:42:31]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-04-05 20:42:31]

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

[commit-bot: I haz the power, 2017-04-05 20:42:53]

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

[Mathieu, 2017-04-05 21:05:00]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-04-05 21:05:00]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2800713003/#ps60001
(title: "switch statement NOTREACHED")

[commit-bot: I haz the power, 2017-04-05 21:05:38]

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-05 21:32:18]

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

[commit-bot: I haz the power, 2017-04-05 21:32:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2017-04-06 00:46:38]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-04-06 00:46:38]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2800713003/#ps80001
(title: "fix test")

[commit-bot: I haz the power, 2017-04-06 00:47:07]

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-06 02:01:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491439598408970,
"parent_rev": "b72edff87d31139a6c466b14c153776029bd6008", "commit_rev":
"b65623a9c0565962586271410beb98e53c2beaee"}

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

Description was changed from

==========
[Payments] Move parsing the PaymentMethodData to "core"

This will allow it to be used on iOS as well. We now copy the
mojom::PaymentMethodData entries to a common type
(PaymentMethodData) before parsing.

BUG=708052
TEST=existing components_unittests (PaymentRequestSpec)
==========

to

==========
[Payments] Move parsing the PaymentMethodData to "core"

This will allow it to be used on iOS as well. We now copy the
mojom::PaymentMethodData entries to a common type
(PaymentMethodData) before parsing.

BUG=708052
TEST=existing components_unittests (PaymentRequestSpec)

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

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b65623a9c0565962586271410beb...