[Payments] Implements the credit card editor for Desktop UI

[Payments] Implements the credit card editor for Desktop UI

Creates the EditorViewController base class, which will handle events and UI that is common between the address, contact and credit card editors. 

Introducing the CreditCardViewController as well, the first subclass possibly the most complex of the editors. 


BUG=687601
TEST=PaymentRequest* interactive_ui_tests

Review-Url: https://codereview.chromium.org/2676663002
Cr-Commit-Position: refs/heads/master@{#448339}
Committed: https://chromium.googlesource.com/chromium/src/+/34728b5bf15dfb51a5206ac8a109096f28cc21e6

[Mathieu, 2017-02-02 19:07:37]

Description was changed from

==========
[Payments] Credit card editor



BUG=
==========

to

==========
[Payments] Implements the credit card editor for Desktop UI

Creates the EditorViewController base class, which will handle events and UI
that is common between the address, contact and credit card editors. 

Introducing the CreditCardViewController as well, the first subclass possibly
the most complex of the editors. 


BUG=687601
TEST=PaymentRequest* interactive_ui_tests
==========

[Mathieu, 2017-02-02 19:07:37]

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

[Mathieu, 2017-02-02 19:13:48]

Patchset #1 (id:1) has been deleted

[Mathieu, 2017-02-02 19:13:54]

Patchset #1 (id:20001) has been deleted

[Mathieu, 2017-02-02 19:14:19]

Hi Rouslan, PTAL at the whole thing

sky for views/

[please use gerrit instead, 2017-02-02 20:19:03]

Good stuff!

https://codereview.chromium.org/2676663002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15379: +    <message
name="IDS_PAYMENT_REQUEST_CREDIT_CARD_EDITOR_ADD_TITLE" desc="The title on top
of the desktop Payment Request credit card editor, to add a card">
Translators might be confused by "Payment Request". Let's rephrase to something
like "The title on the top of the dialog for adding a new credit card." WDYT?

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:34:
autofill::CREDIT_CARD_NUMBER,
Android puts number first, by the way.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:37:
fields.push_back(base::MakeUnique<EditorField>(
Adding the exp-month field in a follow up patch?

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.h
(right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:9:
#include <vector>
Generally it's not necessary to add includes for the types that are used only to
override method declartions in the parent class.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:22: //
Does not take ownership of the arguments, which should outlive this object.
Love this comment! :-D

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:7: #include <memory>
Already included in the corresponding header file, so not necessary, according
to Chromium style guide.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:11: #include
"base/memory/ptr_util.h"
Ditto

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:33: 
enum class?

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:35: base::string16
label,
const-ref to avoid an extra copy.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:39:
autofill::ServerFieldType type;
Here and below: "const" to make this type immutable. (Makes reasoning about it
easier.)

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:42: 
nit: No need for newlines on lines 40 and 42.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:77: views::Textfield**
text_field);
Is |text_field| an output?

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/view_stack.h (right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/view_stack.h:15: 
Is this forward declaration necessary?

[Mathieu, 2017-02-03 02:18:57]

PTAL

https://codereview.chromium.org/2676663002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15379: +    <message
name="IDS_PAYMENT_REQUEST_CREDIT_CARD_EDITOR_ADD_TITLE" desc="The title on top
of the desktop Payment Request credit card editor, to add a card">
On 2017/02/02 20:19:02, rouslan wrote:
> Translators might be confused by "Payment Request". Let's rephrase to
something
> like "The title on the top of the dialog for adding a new credit card." WDYT?

Sure, done

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:34:
autofill::CREDIT_CARD_NUMBER,
On 2017/02/02 20:19:02, rouslan wrote:
> Android puts number first, by the way.

Done.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:37:
fields.push_back(base::MakeUnique<EditorField>(
On 2017/02/02 20:19:02, rouslan wrote:
> Adding the exp-month field in a follow up patch?

Not meant to be functional just yet, but for now it's expiration *date*, so
"mm/yy"

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.h
(right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:9:
#include <vector>
On 2017/02/02 20:19:02, rouslan wrote:
> Generally it's not necessary to add includes for the types that are used only
to
> override method declartions in the parent class.

Done.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:22: //
Does not take ownership of the arguments, which should outlive this object.
On 2017/02/02 20:19:02, rouslan wrote:
> Love this comment! :-D

got it from a wise person

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:7: #include <memory>
On 2017/02/02 20:19:02, rouslan wrote:
> Already included in the corresponding header file, so not necessary, according
> to Chromium style guide.

Done.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:11: #include
"base/memory/ptr_util.h"
On 2017/02/02 20:19:02, rouslan wrote:
> Ditto

Done.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:33: 
On 2017/02/02 20:19:02, rouslan wrote:
> enum class?

Done.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:35: base::string16
label,
On 2017/02/02 20:19:02, rouslan wrote:
> const-ref to avoid an extra copy.

Done.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:39:
autofill::ServerFieldType type;
On 2017/02/02 20:19:02, rouslan wrote:
> Here and below: "const" to make this type immutable. (Makes reasoning about it
> easier.)

Done.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:42: 
On 2017/02/02 20:19:02, rouslan wrote:
> nit: No need for newlines on lines 40 and 42.

Done.

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:77: views::Textfield**
text_field);
On 2017/02/02 20:19:02, rouslan wrote:
> Is |text_field| an output?

Yes. This is a way for the caller to provide the Textfield* they would like to
see populated (it's kept in a map, later)

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/view_stack.h (right):

https://codereview.chromium.org/2676663002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/view_stack.h:15: 
On 2017/02/02 20:19:03, rouslan wrote:
> Is this forward declaration necessary?

Yes :\ It was cryptic to me why it was needed; someone explained that without
it, the "friend class" statement below does not know what the symbol is, and
compiler may think we are just declaring an inner class.

[Mathieu, 2017-02-03 02:19:03]

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

[commit-bot: I haz the power, 2017-02-03 02:19:31]

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-02-03 02:33:43]

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

[commit-bot: I haz the power, 2017-02-03 02:33:44]

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

[please use gerrit instead, 2017-02-03 02:39:28]

lgtm

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

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

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

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-02-03 03:38:28]

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

[commit-bot: I haz the power, 2017-02-03 03:38:29]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-02-04 03:00:11]

Friendly ping Scott?

[Mathieu, 2017-02-06 14:50:30]

mathp@chromium.org changed reviewers:
- sky@chromium.org

[Mathieu, 2017-02-06 16:27:37]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-02-06 16:27: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/2676663002/#ps80001
(title: "missing test")

[commit-bot: I haz the power, 2017-02-06 16:28:10]

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-02-06 16:30:22]

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

[commit-bot: I haz the power, 2017-02-06 16:30:23]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Mathieu, 2017-02-06 16:46:13]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-02-06 16:46:13]

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/2676663002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-06 16:46:47]

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-02-06 16:49:12]

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

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

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[sky, 2017-02-06 17:05:10]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2017-02-06 17:05:11]

What is the plan for the mac side? Is the hope that it uses views?

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

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:35: SAVE_BUTTON =
kFirstTagValue,
It's worth a comment why this enum starts at kFirstTagValue.

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:109: EditorField*
field,
const EditorField& ?

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:79:
std::unordered_map<views::Textfield*, std::unique_ptr<EditorField>>
optional: EditorField is cheap, I personally wouldn't bother with the unique_ptr
for EditorField. It's worth documenting who owns the textfields.

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

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:31:
ADD_CREDIT_CARD_BUTTON = kFirstTagValue,
Same comment about why this has to start at kFirstTagValue.

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

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_credit_card_editor_interactive_uitest.cc:16:
};
private: DISALLOW...

[Mathieu, 2017-02-06 17:28:27]

Patchset #5 (id:120001) has been deleted

[Mathieu, 2017-02-06 17:28:32]

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

[commit-bot: I haz the power, 2017-02-06 17:29:00]

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

[Mathieu, 2017-02-06 17:46:24]

Hi Scott, PTAL

Yeah the plan is to ship this with MacViews, possibly delaying the Mac release
until they are ready to ship secondary UI with views.

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

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:35: SAVE_BUTTON =
kFirstTagValue,
On 2017/02/06 17:05:11, sky wrote:
> It's worth a comment why this enum starts at kFirstTagValue.

Done.

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:109: EditorField*
field,
On 2017/02/06 17:05:11, sky wrote:
> const EditorField& ?

Done.

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:79:
std::unordered_map<views::Textfield*, std::unique_ptr<EditorField>>
On 2017/02/06 17:05:11, sky wrote:
> optional: EditorField is cheap, I personally wouldn't bother with the
unique_ptr
> for EditorField. It's worth documenting who owns the textfields.

Agree, it simplifies code thanks

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

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:31:
ADD_CREDIT_CARD_BUTTON = kFirstTagValue,
On 2017/02/06 17:05:11, sky wrote:
> Same comment about why this has to start at kFirstTagValue.

Done.

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

https://codereview.chromium.org/2676663002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_credit_card_editor_interactive_uitest.cc:16:
};
On 2017/02/06 17:05:11, sky wrote:
> private: DISALLOW...

Done.

[Mathieu, 2017-02-06 17:56:17]

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

[commit-bot: I haz the power, 2017-02-06 17:56:44]

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

[sky, 2017-02-06 18:07:14]

LGTM

https://codereview.chromium.org/2676663002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2676663002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:36:
autofill::CREDIT_CARD_EXP_DATE_2_DIGIT_YEAR,
Make sure you run git cl format.

[Mathieu, 2017-02-06 18:09:17]

Thanks

https://codereview.chromium.org/2676663002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2676663002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:36:
autofill::CREDIT_CARD_EXP_DATE_2_DIGIT_YEAR,
On 2017/02/06 18:07:14, sky wrote:
> Make sure you run git cl format.

I relied on `git cl format' a little too heavily :) Done

[Mathieu, 2017-02-06 18:09:21]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-02-06 18:09:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2676663002/#ps180001
(title: "formatting")

[commit-bot: I haz the power, 2017-02-06 18:09:57]

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-02-06 19:14:36]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486404561245990,
"parent_rev": "1b8ec1250bd59a373acfeb6040ded92310ad315a", "commit_rev":
"34728b5bf15dfb51a5206ac8a109096f28cc21e6"}

[commit-bot: I haz the power, 2017-02-06 19:15:13]

Description was changed from

==========
[Payments] Implements the credit card editor for Desktop UI

Creates the EditorViewController base class, which will handle events and UI
that is common between the address, contact and credit card editors. 

Introducing the CreditCardViewController as well, the first subclass possibly
the most complex of the editors. 


BUG=687601
TEST=PaymentRequest* interactive_ui_tests
==========

to

==========
[Payments] Implements the credit card editor for Desktop UI

Creates the EditorViewController base class, which will handle events and UI
that is common between the address, contact and credit card editors. 

Introducing the CreditCardViewController as well, the first subclass possibly
the most complex of the editors. 


BUG=687601
TEST=PaymentRequest* interactive_ui_tests

Review-Url: https://codereview.chromium.org/2676663002
Cr-Commit-Position: refs/heads/master@{#448339}
Committed:
https://chromium.googlesource.com/chromium/src/+/34728b5bf15dfb51a5206ac8a109...
==========

[commit-bot: I haz the power, 2017-02-06 19:15:14]

Committed patchset #7 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/34728b5bf15dfb51a5206ac8a109...