[Payments] Use PaymentInstrument/AutofillPaymentInstrument throughout.

[Payments] Use PaymentInstrument/AutofillPaymentInstrument throughout.

Limit the concept of CreditCard to PaymentRequestState (when creating
instruments) and the Credit Card editor.

BUG=702061
TEST=Existing

Review-Url: https://codereview.chromium.org/2757523002
Cr-Commit-Position: refs/heads/master@{#457491}
Committed: https://chromium.googlesource.com/chromium/src/+/363735b939c7ce86349552cda7b012310f384fcb

[Mathieu, 2017-03-16 02:49:07]

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

[commit-bot: I haz the power, 2017-03-16 02:49:22]

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

[Mathieu, 2017-03-16 02:49:32]

Patchset #1 (id:1) has been deleted

[Mathieu, 2017-03-16 02:50:02]

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

[Mathieu, 2017-03-16 02:50:03]

Hi Rouslan, PTAL

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

https://codereview.chromium.org/2757523002/diff/20001/components/payments/con...
components/payments/content/payment_request_spec.h:96: std::set<std::string>
supported_card_networks_set_;
this I'm not sure about. It's used in PaymentRequestState when creating the
initial list of instruments. If you have many credit cards, I think the O(1)
lookup will be nice, and it's relatively low overhead to create this set. But I
could see it both ways. WDYT?

[commit-bot: I haz the power, 2017-03-16 02:53:10]

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

[commit-bot: I haz the power, 2017-03-16 02:53:11]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  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-03-16 02:53:36]

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

[commit-bot: I haz the power, 2017-03-16 02:53:50]

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-03-16 03:36:51]

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

[commit-bot: I haz the power, 2017-03-16 03:36:52]

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

[Mathieu, 2017-03-16 12:37:50]

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

[commit-bot: I haz the power, 2017-03-16 12:38:00]

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-03-16 13:20:05]

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

[commit-bot: I haz the power, 2017-03-16 13:20:06]

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

[Mathieu, 2017-03-16 13:53:22]

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

[commit-bot: I haz the power, 2017-03-16 13:53: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-03-16 14:05:01]

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

[commit-bot: I haz the power, 2017-03-16 14:05:02]

Dry run: 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_...)

[Mathieu, 2017-03-16 14:10:10]

Patchset #4 (id:80001) has been deleted

[Mathieu, 2017-03-16 14:10:21]

Description was changed from

==========
[Payments] Use PaymentInstrument/AutofillPaymentInstrument throughout.

Limit the concept of CreditCard to PaymentRequestState (when creating
instruments) and the Credit Card editor.

BUG=702061
TEST=Existing
==========

to

==========
[Payments] Use PaymentInstrument/AutofillPaymentInstrument throughout.

Limit the concept of CreditCard to PaymentRequestState (when creating
instruments) and the Credit Card editor.

BUG=702061
TEST=Existing
==========

[Mathieu, 2017-03-16 14:10:21]

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

[Mathieu, 2017-03-16 14:10:36]

Removed the instrument_cache as per Anthony's feedback. PTAL

[please use gerrit instead, 2017-03-16 14:27:03]

Overall design looks good!

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

https://codereview.chromium.org/2757523002/diff/20001/components/payments/con...
components/payments/content/payment_request_spec.h:96: std::set<std::string>
supported_card_networks_set_;
On 2017/03/16 02:50:03, Mathieu Perreault wrote:
> this I'm not sure about. It's used in PaymentRequestState when creating the
> initial list of instruments. If you have many credit cards, I think the O(1)
> lookup will be nice, and it's relatively low overhead to create this set. But
I
> could see it both ways. WDYT?

This is OK.

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
File
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:51:
EXPECT_EQ(request->state()->available_instruments()[0].get(),
.front() is more readable than [0], IMHO

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:76:
EXPECT_EQ(request->state()->available_instruments()[0].get(),
Ditto

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:84:
EXPECT_EQ(request->state()->available_instruments()[0].get(),
Ditto

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:97:
EXPECT_EQ(request->state()->available_instruments()[1].get(),
.back() ?

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:105:
EXPECT_EQ(request->state()->available_instruments()[1].get(),
Ditto

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
File components/payments/content/payment_request_spec.h (right):

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
components/payments/content/payment_request_spec.h:53: const
std::set<std::string>& supported_card_networks_set() {
These methods should be const.

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
File components/payments/content/payment_request_state.h (right):

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
components/payments/content/payment_request_state.h:164: // Credit cards are
indirectly owned by this instrument list.
What do you mean by "indirectly"?

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
File components/payments/core/BUILD.gn (right):

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
components/payments/core/BUILD.gn:17: "payment_instrument.cc",
.h files should be here as well.

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
File components/payments/core/payment_instrument.h (right):

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
components/payments/core/payment_instrument.h:36: virtual bool IsValid() = 0;
Add a comment.

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
components/payments/core/payment_instrument.h:38: const std::string&
method_name() { return method_name_; }
These methods should be const.

[Mathieu, 2017-03-16 14:56:57]

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

[Mathieu, 2017-03-16 14:57:04]

PTAL

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
File
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:51:
EXPECT_EQ(request->state()->available_instruments()[0].get(),
On 2017/03/16 14:27:02, ಠ_ಠ wrote:
> .front() is more readable than [0], IMHO

Done.

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:76:
EXPECT_EQ(request->state()->available_instruments()[0].get(),
On 2017/03/16 14:27:03, ಠ_ಠ wrote:
> Ditto

Done.

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:84:
EXPECT_EQ(request->state()->available_instruments()[0].get(),
On 2017/03/16 14:27:02, ಠ_ಠ wrote:
> Ditto

Done.

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:97:
EXPECT_EQ(request->state()->available_instruments()[1].get(),
On 2017/03/16 14:27:02, ಠ_ಠ wrote:
> .back() ?

Done.

https://codereview.chromium.org/2757523002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:105:
EXPECT_EQ(request->state()->available_instruments()[1].get(),
On 2017/03/16 14:27:02, ಠ_ಠ wrote:
> Ditto

Done.

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
File components/payments/content/payment_request_spec.h (right):

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
components/payments/content/payment_request_spec.h:53: const
std::set<std::string>& supported_card_networks_set() {
On 2017/03/16 14:27:03, ಠ_ಠ wrote:
> These methods should be const.

Done.

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
File components/payments/content/payment_request_state.h (right):

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
components/payments/content/payment_request_state.h:164: // Credit cards are
indirectly owned by this instrument list.
On 2017/03/16 14:27:03, ಠ_ಠ wrote:
> What do you mean by "indirectly"?

reworded

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
File components/payments/core/BUILD.gn (right):

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
components/payments/core/BUILD.gn:17: "payment_instrument.cc",
On 2017/03/16 14:27:03, ಠ_ಠ wrote:
> .h files should be here as well.

One line down?

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
File components/payments/core/payment_instrument.h (right):

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
components/payments/core/payment_instrument.h:36: virtual bool IsValid() = 0;
On 2017/03/16 14:27:03, ಠ_ಠ wrote:
> Add a comment.

Done.

https://codereview.chromium.org/2757523002/diff/100001/components/payments/co...
components/payments/core/payment_instrument.h:38: const std::string&
method_name() { return method_name_; }
On 2017/03/16 14:27:03, ಠ_ಠ wrote:
> These methods should be const.

Done.

[commit-bot: I haz the power, 2017-03-16 14:57:26]

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

[please use gerrit instead, 2017-03-16 15:01:24]

LGTM % comments

https://codereview.chromium.org/2757523002/diff/120001/components/payments/co...
File components/payments/core/payment_instrument.h (right):

https://codereview.chromium.org/2757523002/diff/120001/components/payments/co...
components/payments/core/payment_instrument.h:36: // Returns if the card is
valid to be used as a payment method.
Add "true" after "Returns". (Or replace "if" with "whether".)

https://codereview.chromium.org/2757523002/diff/120001/components/payments/co...
components/payments/core/payment_instrument.h:42: int icon_resource_id() {
return icon_resource_id_; }
const this method as well, please.

[Mathieu, 2017-03-16 15:44:00]

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

[commit-bot: I haz the power, 2017-03-16 15:44:32]

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

[anthonyvd, 2017-03-16 15:57:38]

lgtm

[Mathieu, 2017-03-16 16:03:30]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-03-16 16:03:30]

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/2757523002/#ps140001
(title: "addressed nits")

[Mathieu, 2017-03-16 16:03:31]

The CQ bit was unchecked by mathp@chromium.org

[Mathieu, 2017-03-16 17:30:46]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2017-03-16 17:31:56]

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-03-16 18:08:10]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489685446778200,
"parent_rev": "3226ce85f3ba466797db44fe134abb8b898d2452", "commit_rev":
"363735b939c7ce86349552cda7b012310f384fcb"}

[commit-bot: I haz the power, 2017-03-16 18:08:53]

Description was changed from

==========
[Payments] Use PaymentInstrument/AutofillPaymentInstrument throughout.

Limit the concept of CreditCard to PaymentRequestState (when creating
instruments) and the Credit Card editor.

BUG=702061
TEST=Existing
==========

to

==========
[Payments] Use PaymentInstrument/AutofillPaymentInstrument throughout.

Limit the concept of CreditCard to PaymentRequestState (when creating
instruments) and the Credit Card editor.

BUG=702061
TEST=Existing

Review-Url: https://codereview.chromium.org/2757523002
Cr-Commit-Position: refs/heads/master@{#457491}
Committed:
https://chromium.googlesource.com/chromium/src/+/363735b939c7ce86349552cda7b0...
==========

[commit-bot: I haz the power, 2017-03-16 18:08:58]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/363735b939c7ce86349552cda7b0...