Adding new shipping address editor view to payment flow.

Adding new shipping address editor view to payment flow.

BUG=703758
Review-Url: https://codereview.chromium.org/2709093006
Cr-Commit-Position: refs/heads/master@{#458983}
Committed: https://chromium.googlesource.com/chromium/src/+/e3602142ccdc1689eb297c43e1f5e8d27f680776

[MAD, 2017-02-23 21:52:43]

Patchset #1 (id:1) has been deleted

[MAD, 2017-03-20 20:37:38]

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

[MAD, 2017-03-20 20:37:39]

This has all the basic functionality but is missing the stuff listed below.
Still, I would appreciate if you could take a look at the code and see if I took
some wrong turns on the way there.

Missing stuff:
- Proper testing
  I'm working on a browser test similar to
credit_card_editor_view_controller_browsertest.cc

- Address validation / sanitization
  We can't use the phone validation api as is, and I still need to understand if
and how I need to use the address sanitizer.

- minor UI glitches
  - The vertical scrollbar doesn't update its size when we relayout the editor
(i.e., when the country changes).
  - We don't change the region (state/provice) combo box into an edit box when
we can't fetch the values.
  - We only use one item per row, even for the small ones that could fit more
than one per row.

- More testing... :-)

Thanks!

BYE
MAD

[MAD, 2017-03-20 20:37:45]

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

[commit-bot: I haz the power, 2017-03-20 20:38:30]

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-20 20:54:12]

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

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

Dry run: Try jobs failed on following builders:
  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-21 15:53:54]

Description was changed from

==========
Adding new shipping address editor view to payment flow.

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

to

==========
Adding new shipping address editor view to payment flow.

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

[Mathieu, 2017-03-21 15:53:54]

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

[MAD, 2017-03-21 15:55:13]

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

[Mathieu, 2017-03-21 15:55:26]

Adding Anthony as a views owner.

I will look into the libphonenumber thing (sorry for the delay!).

Also can you create a crbug for the whole CL? Follow-ups will be able to
reference it, too.

I'm very excited for this, thanks!

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
(right):

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:35:
#include "components/payments/content/payment_request.h"
nit: payment_request_state.h

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:53:
const char* kAddressValidationDataURL =
See how the URL is used as a compile-time constant in
personal_data_manager_android

https://cs.chromium.org/chromium/src/chrome/browser/autofill/android/personal...

Can we replicate here?

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:56:
autofill::ServerFieldType GetFieldTypeFromString(const std::string& type) {
Let's put a small comment to define input and output.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:205:
std::unique_ptr<base::ListValue> components(new base::ListValue);
nit: would bring closer to line 211

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:206:
std::string unused;
same

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:212:
chosen_country_code, state()->GetPersonalDataManager()->app_locale(),
state()->GetApplicationLocale()

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:215:
for (size_t lineIndex = 0; lineIndex < components->GetSize(); ++lineIndex) {
nit: line_index

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:222:
for (size_t componentIndex = 0; componentIndex < line->GetSize();
same nit

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:259:
server_field_type != autofill::COMPANY_NAME,  // required.
nit: In other places we use /*required=*/server_field_type !=
autofill::COMPANY_NAME which is kind of neat.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:267:
EditorField::LengthHint::HINT_LONG, true,
/*required=*/true

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:276:
EditorField::LengthHint::HINT_SHORT, false,  // required.
nit: /*required=*/false

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:282:
// Make sure to unfocus the combo box so we can remove it when we update the
nit: combobox

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:315:
TODO(mad): Bring back when we can include libphonenumber/phonenumber_api.h.
Let's create and put a bug number for this: TODO(crbug.com/XXXXXX)

I would remove the commented-out code from this patch and document it instead in
the bug.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:319:
return phone_util->Parse(
note for later: after parsing, there is also "IsValidNumber" [1]

https://cs.chromium.org/chromium/src/chrome/browser/autofill/android/phone_nu...

Might be worth asking gogerald@ which we should use here, he's been thinking
about phone numbers in PR a lot

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:325:
// TODO(mathp): Display "required" error if applicable.
Let's create a TODO(crbug.com/XXXXXX) for this since we are many people working
on this code it's no longer tractable :)

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h
(right):

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:46:
explicit ShippingAddressValidationDelegate(
remove explicit

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:56:
// Validates a specific |value|.
nit: remove obvious comment :)

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:64:
friend class ShippingAddressValidationDelegate;
Curious, why is this needed?

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:69:
// The currently chosen country.
Can you add a comment about what the "0" means. Is it a dummy value (e.g.,
"Select...") or an actual country?

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:72:
// The list of country codes as ordered in the country combo box model.
nit: here and below: "combobox"

[commit-bot: I haz the power, 2017-03-21 15:55: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-21 16:10:09]

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

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

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_...)
  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-21 17:08:35]

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

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:50:
EditorField(autofill::ServerFieldType type,
Let's change it like so:

  EditorField(autofill::ServerFieldType type,
              base::string16 label,
              LengthHint length_hint,
              bool required,
              ControlType control_type = ControlType::TEXTFIELD)
      : type(type),
        label(std::move(label)),
        length_hint(length_hint),
        required(required),
        control_type(control_type) {}

This way there is the minimum amount of work.

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:68: const
autofill::ServerFieldType type;
we can remove const here and on |label|

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h
(right):

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:71:
int chosen_country_index_{0};
let's have this be size_t?

[Mathieu, 2017-03-21 17:10:15]

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
(right):

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:276:
EditorField::LengthHint::HINT_SHORT, false,  // required.
Let's make the phone field LONG

[MAD, 2017-03-21 18:52:17]

Description was changed from

==========
Adding new shipping address editor view to payment flow.

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

to

==========
Adding new shipping address editor view to payment flow.

BUG=703758
==========

[MAD, 2017-03-21 19:10:46]

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

[commit-bot: I haz the power, 2017-03-21 19:11:16]

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

[MAD, 2017-03-21 19:25:00]

Patchset #8 (id:160001) has been deleted

[MAD, 2017-03-21 19:26:40]

Patchset #8 (id:180001) has been deleted

[MAD, 2017-03-21 19:28:34]

Addressed all comments.

Now on to add the browser test. Then, I think we can simply add bugs for the
minor UI glitches:
  - The vertical scrollbar doesn't update its size when we relayout the editor
(i.e., when the country changes).
  - We don't change the region (state/provice) combo box into an edit box when
we can't fetch the values.
  - We only use one item per row, even for the small ones that could fit more
than one per row.

I alread filed a bug and added TODOs for validation / sanitization.

OK?

BYE
MAD

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
(right):

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:35:
#include "components/payments/content/payment_request.h"
On 2017/03/21 15:55:24, Mathieu Perreault wrote:
> nit: payment_request_state.h

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:53:
const char* kAddressValidationDataURL =
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> See how the URL is used as a compile-time constant in
> personal_data_manager_android
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/autofill/android/personal...
> 
> Can we replicate here?

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:56:
autofill::ServerFieldType GetFieldTypeFromString(const std::string& type) {
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> Let's put a small comment to define input and output.

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:205:
std::unique_ptr<base::ListValue> components(new base::ListValue);
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> nit: would bring closer to line 211

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:206:
std::string unused;
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> same

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:212:
chosen_country_code, state()->GetPersonalDataManager()->app_locale(),
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> state()->GetApplicationLocale()

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:215:
for (size_t lineIndex = 0; lineIndex < components->GetSize(); ++lineIndex) {
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> nit: line_index

Héhé... I've done too much JavaScript in 2016... ;-)
Actually, I think I stole this from the Options Web UI Javascript...

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:222:
for (size_t componentIndex = 0; componentIndex < line->GetSize();
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> same nit

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:259:
server_field_type != autofill::COMPANY_NAME,  // required.
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> nit: In other places we use /*required=*/server_field_type !=
> autofill::COMPANY_NAME which is kind of neat.

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:267:
EditorField::LengthHint::HINT_LONG, true,
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> /*required=*/true

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:276:
EditorField::LengthHint::HINT_SHORT, false,  // required.
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> nit: /*required=*/false

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:282:
// Make sure to unfocus the combo box so we can remove it when we update the
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> nit: combobox

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:315:
TODO(mad): Bring back when we can include libphonenumber/phonenumber_api.h.
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> Let's create and put a bug number for this: TODO(crbug.com/XXXXXX)
> 
> I would remove the commented-out code from this patch and document it instead
in
> the bug.

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:319:
return phone_util->Parse(
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> note for later: after parsing, there is also "IsValidNumber" [1]
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/autofill/android/phone_nu...
> 
> Might be worth asking gogerald@ which we should use here, he's been thinking
> about phone numbers in PR a lot

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:325:
// TODO(mathp): Display "required" error if applicable.
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> Let's create a TODO(crbug.com/XXXXXX) for this since we are many people
working
> on this code it's no longer tractable :)

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h
(right):

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:46:
explicit ShippingAddressValidationDelegate(
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> remove explicit

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:56:
// Validates a specific |value|.
On 2017/03/21 15:55:26, Mathieu Perreault wrote:
> nit: remove obvious comment :)

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:64:
friend class ShippingAddressValidationDelegate;
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> Curious, why is this needed?

It will be needed once we use the parent to validate fields based on the current
country.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:69:
// The currently chosen country.
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> Can you add a comment about what the "0" means. Is it a dummy value (e.g.,
> "Select...") or an actual country?

Done.

https://codereview.chromium.org/2709093006/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:72:
// The list of country codes as ordered in the country combo box model.
On 2017/03/21 15:55:25, Mathieu Perreault wrote:
> nit: here and below: "combobox"

Done.

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

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:50:
EditorField(autofill::ServerFieldType type,
On 2017/03/21 17:08:35, Mathieu Perreault wrote:
> Let's change it like so:
> 
>   EditorField(autofill::ServerFieldType type,
>               base::string16 label,
>               LengthHint length_hint,
>               bool required,
>               ControlType control_type = ControlType::TEXTFIELD)
>       : type(type),
>         label(std::move(label)),
>         length_hint(length_hint),
>         required(required),
>         control_type(control_type) {}
> 
> This way there is the minimum amount of work. 

Done.

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:68: const
autofill::ServerFieldType type;
On 2017/03/21 17:08:35, Mathieu Perreault wrote:
> we can remove const here and on |label|

Done.

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
(right):

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:276:
EditorField::LengthHint::HINT_SHORT, false,  // required.
On 2017/03/21 17:10:15, Mathieu Perreault wrote:
> Let's make the phone field LONG

Done.

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h
(right):

https://codereview.chromium.org/2709093006/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:71:
int chosen_country_index_{0};
On 2017/03/21 17:08:35, Mathieu Perreault wrote:
> let's have this be size_t?

Yeah, I changed it to int because the combobox API we get it from and compare
with is an int, and we get compile errors if we don't static cast it on non
Windows platforms. But now I see that we also compare it to the
std::vector::size() too, so might as well... set it back to size_t... :-/

[MAD, 2017-03-21 20:01:36]

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

[commit-bot: I haz the power, 2017-03-21 20:02:02]

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-21 21:09:26]

lgtm

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h
(right):

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:72:
size_t chosen_country_index_{0};
Is this the user's country? If not, we should TODO to have it so.

[commit-bot: I haz the power, 2017-03-21 21:11:58]

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

[commit-bot: I haz the power, 2017-03-21 21:11:59]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-03-21 21:18:25]

Looking good, only a few comments :) Thanks!

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

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:129:
views::GridLayout* editor_layout = new views::GridLayout(editor_view_.get());
For consistency, this should be a unique_ptr created with base::MakeUnique that
gets released in the call to SetLayoutManager. The rest of payments code does it
that way to make ownership clearer (except in cases where the result of the new
call is passed directly to a views:: function, like my_view->AddChildView(new
views::View);)

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/profile_list_view_controller.cc (right):

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:110:
std::unique_ptr<views::View> CreateExtraFooterView() override {
This code could probably be in the base ProfileListViewController with its
derived classes ({Shipping|Contact}ProfileViewController) implementing a
function to get the "Add" button string and another to perform the action when
it's clicked.

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
(right):

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:146:
autofill::CountryComboboxModel* model =
Same comment as above about using unique_ptr and .release().

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/view_stack.cc (right):

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/view_stack.cc:12: #include
"ui/views/widget/widget.h"
I don't think this include is required.

[MAD, 2017-03-22 20:14:27]

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

[commit-bot: I haz the power, 2017-03-22 20:15:17]

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

[MAD, 2017-03-22 20:15:43]

Addressed all comments and started adding support for testing but wasn't able to
get the browser test working just yet, so I have not included it in the CL yet.

If you want, I could add it in a subsequent CL, or wait to commit this one until
the test is ready.

Thanks!

BYE
MAD

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

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:129:
views::GridLayout* editor_layout = new views::GridLayout(editor_view_.get());
On 2017/03/21 21:18:24, anthonyvd wrote:
> For consistency, this should be a unique_ptr created with base::MakeUnique
that
> gets released in the call to SetLayoutManager. The rest of payments code does
it
> that way to make ownership clearer (except in cases where the result of the
new
> call is passed directly to a views:: function, like my_view->AddChildView(new
> views::View);)

Done.

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/profile_list_view_controller.cc (right):

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:110:
std::unique_ptr<views::View> CreateExtraFooterView() override {
On 2017/03/21 21:18:24, anthonyvd wrote:
> This code could probably be in the base ProfileListViewController with its
> derived classes ({Shipping|Contact}ProfileViewController) implementing a
> function to get the "Add" button string and another to perform the action when
> it's clicked.

Done.

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
(right):

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:146:
autofill::CountryComboboxModel* model =
On 2017/03/21 21:18:24, anthonyvd wrote:
> Same comment as above about using unique_ptr and .release().

Done.

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h
(right):

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:72:
size_t chosen_country_index_{0};
On 2017/03/21 21:09:25, Mathieu Perreault wrote:
> Is this the user's country? If not, we should TODO to have it so.

Done.

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/view_stack.cc (right):

https://codereview.chromium.org/2709093006/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/view_stack.cc:12: #include
"ui/views/widget/widget.h"
On 2017/03/21 21:18:24, anthonyvd wrote:
> I don't think this include is required.

Done.

[commit-bot: I haz the power, 2017-03-22 20:18:35]

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

[commit-bot: I haz the power, 2017-03-22 20:18:36]

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

[anthonyvd, 2017-03-22 20:21:08]

lgtm.

I'd land this as-is and the tests in a follow-up since this CL is getting big.
Plus, I'll need to rebase on this CL for another I'm working on so this'll let
me do that :)

Thanks!

[MAD, 2017-03-22 22:28:16]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-03-22 22:28:16]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2709093006/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-22 22:29:12]

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-22 23:08:36]

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

[commit-bot: I haz the power, 2017-03-22 23:08:38]

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

[MAD, 2017-03-23 00:04:35]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-03-23 00:04:36]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2709093006/#ps260001
(title: "Bot failure fix")

[commit-bot: I haz the power, 2017-03-23 00:05: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-03-23 02:30:06]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1490227475880320,
"parent_rev": "25b307908f44425f58b08445e42b1a3a7ca3ab00", "commit_rev":
"e3602142ccdc1689eb297c43e1f5e8d27f680776"}

[commit-bot: I haz the power, 2017-03-23 02:30:53]

Description was changed from

==========
Adding new shipping address editor view to payment flow.

BUG=703758
==========

to

==========
Adding new shipping address editor view to payment flow.

BUG=703758

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

[commit-bot: I haz the power, 2017-03-23 02:30:55]

Committed patchset #11 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/e3602142ccdc1689eb297c43e1f5...