Add billing address as a mandatory field of Payments credit cards.

Add billing address as a mandatory field of Payments credit cards.

TBR=markusheintz@chromium.org
For trivial adaptation to API change in:
chrome/browser/browsing_data/autofill_counter_browsertest.cc

BUG=709776
Review-Url: https://codereview.chromium.org/2849523003
Cr-Commit-Position: refs/heads/master@{#470536}
Committed: https://chromium.googlesource.com/chromium/src/+/faf8e1ebbc65063a01ada0f0c5ac8981076210cb

[MAD, 2017-04-27 18:10:32]

Description was changed from

==========
a# Enter a description of the change.
Add billing address as a mandatory field of Payments credit cards.

BUG=709776
==========

to

==========
Add billing address as a mandatory field of Payments credit cards.

BUG=709776
==========

[MAD, 2017-05-02 15:05:24]

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

[commit-bot: I haz the power, 2017-05-02 15:05:47]

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-05-02 15:10:25]

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

[commit-bot: I haz the power, 2017-05-02 15:10:26]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[MAD, 2017-05-02 15:45:03]

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

[commit-bot: I haz the power, 2017-05-02 15:45:39]

Dry run: CQ is trying da patch.

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

[MAD, 2017-05-02 15:53:20]

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

[MAD, 2017-05-02 15:53:21]

I think you can start reviewing this now, although I might add new tests
specific to the new code, which is currently mostly tested indirectly by
existing tests which needed to be modified to support the new code.

Let me know if you prefer I get other reviews (Seb, Anthony or Tommy) to look at
it and only ask you for OWNERs review at the end.

Thanks!

BYE
MAD

[commit-bot: I haz the power, 2017-05-02 15:57:39]

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

[commit-bot: I haz the power, 2017-05-02 15:57:40]

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...)

[MAD, 2017-05-02 17:49:02]

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

[commit-bot: I haz the power, 2017-05-02 17:49:27]

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-05-02 17:59:50]

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

[Mathieu, 2017-05-02 17:59:51]

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

[commit-bot: I haz the power, 2017-05-02 17:59:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mathieu, 2017-05-02 18:00:00]

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

[Mathieu, 2017-05-02 18:00:20]

Would love if Seb would take a look, he knows more about billing address!

[MAD, 2017-05-02 18:36:52]

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

[commit-bot: I haz the power, 2017-05-02 18:37:29]

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-05-02 18:48:54]

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

[commit-bot: I haz the power, 2017-05-02 18:48:55]

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

[sebsg, 2017-05-02 22:58:55]

Very glad to see this change! This is very important and a big task, thanks for
taking it on.

Some comments below.

https://codereview.chromium.org/2849523003/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2849523003/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7738: <message
name="IDS_AUTOFILL_FIELD_LABEL_BILLING_ADDRESS" desc="The label of the Billing
Address chooser.">
For all of these strings (in autofill_strings and payments_strings), do you
think we could add them all in the autofill_strings.grd? If we add them in both
Payments and Autofill they will be translated twice and take up more space. What
do you think? 

They can both be accessed in the generated components_strings anyway no?

Also I would put them in Autofill over Payments since all the data comes form
Autofill.

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

https://codereview.chromium.org/2849523003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:56: const
auto kBillingAddressType = autofill::MAX_VALID_FIELD_TYPE;
This feels hacky but I have no better solution to offer you and understand where
you are coming from :P. Let's see if the UI guys who implemented this system
have a better solution.

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

https://codereview.chromium.org/2849523003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:110: int
add_billing_address_button_tag_;
I'm still a bit confused about this field. I see you set it to 0 in the tests.
Can you explain to me how it works? Thanks!

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

https://codereview.chromium.org/2849523003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc:328:
IN_PROC_BROWSER_TEST_F(PaymentRequestCreditCardEditorTest, EditingExpiredCard) {
Can you add a test where the card has everything it needs except:

1) no billing address
2) bad billing address

Thanks!

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:39: // Add extra
items for the "Select" entry and a separator.
Do we also have the add address button if some addresses are present?

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:80:
profiles_cache_.push_back(base::MakeUnique<AutofillProfile>(profile));
One thing we did on Android is put the new address at the top of the list. That
way if the user clicks on the combobox again, the new address will be right
there at the top. 

Since the number of addresses is generally pretty low, it's not to expensive to
move all the other ones up.

What do you think?

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model_unittest.cc
(right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:23:
EXPECT_FALSE(model.IsItemSeparatorAt(0));
Can you also check that it's not at (1)?

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:28:
TEST(AddressComboboxModelTest, TwoAddresses) {
Can you also add a test with only one address? Thanks!

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:42: // The
order is not guaranteed.
Since you get profiles to suggest to get the billing address, one thing you
could do is to set a use count to one of the profiles

For example, if you set

profile1.set_use_count(100);

profile1 will always be shown before profile2 (which will have a default use
count of 1) since GetProfilesToSuggest gives you the profiles ordered by
frecency.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/credit_card_unittest.cc:345: "1", nullptr,
true}));
Can you add a test where the billing addresses are not the same? Thanks!

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/validation.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/validation.cc:144: if
(card.billing_address_id().empty()) {
This would be a bit more involved but we might want to check that the billing
address id is associated with a profile that we have on file. We can add it
later if you want (but let's create a bug in that case).

[MAD, 2017-05-03 16:14:57]

Patchset #9 (id:160001) has been deleted

[MAD, 2017-05-03 16:15:18]

Thanks! Please take another look...

BYE
MAD

https://codereview.chromium.org/2849523003/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2849523003/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7738: <message
name="IDS_AUTOFILL_FIELD_LABEL_BILLING_ADDRESS" desc="The label of the Billing
Address chooser.">
On 2017/05/02 22:58:54, sebsg wrote:
> For all of these strings (in autofill_strings and payments_strings), do you
> think we could add them all in the autofill_strings.grd? If we add them in
both
> Payments and Autofill they will be translated twice and take up more space.
What
> do you think? 
> 
> They can both be accessed in the generated components_strings anyway no?
> 
> Also I would put them in Autofill over Payments since all the data comes form
> Autofill.

I was trying to follow the existing pattern. If I move these strings, I should
also move the other ones above, right?

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

https://codereview.chromium.org/2849523003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:56: const
auto kBillingAddressType = autofill::MAX_VALID_FIELD_TYPE;
On 2017/05/02 22:58:54, sebsg wrote:
> This feels hacky but I have no better solution to offer you and understand
where
> you are coming from :P. Let's see if the UI guys who implemented this system
> have a better solution.

Acknowledged.

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

https://codereview.chromium.org/2849523003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:110: int
add_billing_address_button_tag_;
On 2017/05/02 22:58:54, sebsg wrote:
> I'm still a bit confused about this field. I see you set it to 0 in the tests.
> Can you explain to me how it works? Thanks!

It's to identify the button in the listener override. It's set to 0 in "unit"
tests since there is no UI. But it is set to the next available value by the
parent view when it creates the controller.

Should I update the comments to be more specific? I thought it was clear that
the tags were used to identify buttons.

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

https://codereview.chromium.org/2849523003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc:328:
IN_PROC_BROWSER_TEST_F(PaymentRequestCreditCardEditorTest, EditingExpiredCard) {
On 2017/05/02 22:58:55, sebsg wrote:
> Can you add a test where the card has everything it needs except:
> 
> 1) no billing address
> 2) bad billing address
> 
> Thanks!

Yes, was working on it! :-)

Added a few, let me know if you can think of other tests we are missing for
this.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:39: // Add extra
items for the "Select" entry and a separator.
On 2017/05/02 22:58:55, sebsg wrote:
> Do we also have the add address button if some addresses are present?

I'm not sure I understand. You mean the credit card controller may not need an
add address button if the combobox is not empty?
I personally think there should always be an option to add a new address whether
there are existing addresses or not.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:80:
profiles_cache_.push_back(base::MakeUnique<AutofillProfile>(profile));
On 2017/05/02 22:58:55, sebsg wrote:
> One thing we did on Android is put the new address at the top of the list.
That
> way if the user clicks on the combobox again, the new address will be right
> there at the top. 
> 
> Since the number of addresses is generally pretty low, it's not to expensive
to
> move all the other ones up.
> 
> What do you think?

OK, do you also put the selected one at the top even if it's not a newly created
one?

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model_unittest.cc
(right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:23:
EXPECT_FALSE(model.IsItemSeparatorAt(0));
On 2017/05/02 22:58:55, sebsg wrote:
> Can you also check that it's not at (1)?

I can't since there's only one item. :-)

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:28:
TEST(AddressComboboxModelTest, TwoAddresses) {
On 2017/05/02 22:58:55, sebsg wrote:
> Can you also add a test with only one address? Thanks!

We kind of do that in the test where we add a second one, but OK. :-)

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:42: // The
order is not guaranteed.
On 2017/05/02 22:58:55, sebsg wrote:
> Since you get profiles to suggest to get the billing address, one thing you
> could do is to set a use count to one of the profiles
> 
> For example, if you set
> 
> profile1.set_use_count(100);
> 
> profile1 will always be shown before profile2 (which will have a default use
> count of 1) since GetProfilesToSuggest gives you the profiles ordered by
> frecency.

Cool! Thanks...

Done.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/credit_card_unittest.cc:345: "1", nullptr,
true}));
On 2017/05/02 22:58:55, sebsg wrote:
> Can you add a test where the billing addresses are not the same? Thanks!

Done.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/validation.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/validation.cc:144: if
(card.billing_address_id().empty()) {
On 2017/05/02 22:58:55, sebsg wrote:
> This would be a bit more involved but we might want to check that the billing
> address id is associated with a profile that we have on file. We can add it
> later if you want (but let's create a bug in that case).

Done.

[MAD, 2017-05-03 16:15:19]

Thanks! Please take another look...

BYE
MAD

https://codereview.chromium.org/2849523003/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2849523003/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7738: <message
name="IDS_AUTOFILL_FIELD_LABEL_BILLING_ADDRESS" desc="The label of the Billing
Address chooser.">
On 2017/05/02 22:58:54, sebsg wrote:
> For all of these strings (in autofill_strings and payments_strings), do you
> think we could add them all in the autofill_strings.grd? If we add them in
both
> Payments and Autofill they will be translated twice and take up more space.
What
> do you think? 
> 
> They can both be accessed in the generated components_strings anyway no?
> 
> Also I would put them in Autofill over Payments since all the data comes form
> Autofill.

I was trying to follow the existing pattern. If I move these strings, I should
also move the other ones above, right?

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

https://codereview.chromium.org/2849523003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:56: const
auto kBillingAddressType = autofill::MAX_VALID_FIELD_TYPE;
On 2017/05/02 22:58:54, sebsg wrote:
> This feels hacky but I have no better solution to offer you and understand
where
> you are coming from :P. Let's see if the UI guys who implemented this system
> have a better solution.

Acknowledged.

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

https://codereview.chromium.org/2849523003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:110: int
add_billing_address_button_tag_;
On 2017/05/02 22:58:54, sebsg wrote:
> I'm still a bit confused about this field. I see you set it to 0 in the tests.
> Can you explain to me how it works? Thanks!

It's to identify the button in the listener override. It's set to 0 in "unit"
tests since there is no UI. But it is set to the next available value by the
parent view when it creates the controller.

Should I update the comments to be more specific? I thought it was clear that
the tags were used to identify buttons.

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

https://codereview.chromium.org/2849523003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc:328:
IN_PROC_BROWSER_TEST_F(PaymentRequestCreditCardEditorTest, EditingExpiredCard) {
On 2017/05/02 22:58:55, sebsg wrote:
> Can you add a test where the card has everything it needs except:
> 
> 1) no billing address
> 2) bad billing address
> 
> Thanks!

Yes, was working on it! :-)

Added a few, let me know if you can think of other tests we are missing for
this.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:39: // Add extra
items for the "Select" entry and a separator.
On 2017/05/02 22:58:55, sebsg wrote:
> Do we also have the add address button if some addresses are present?

I'm not sure I understand. You mean the credit card controller may not need an
add address button if the combobox is not empty?
I personally think there should always be an option to add a new address whether
there are existing addresses or not.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:80:
profiles_cache_.push_back(base::MakeUnique<AutofillProfile>(profile));
On 2017/05/02 22:58:55, sebsg wrote:
> One thing we did on Android is put the new address at the top of the list.
That
> way if the user clicks on the combobox again, the new address will be right
> there at the top. 
> 
> Since the number of addresses is generally pretty low, it's not to expensive
to
> move all the other ones up.
> 
> What do you think?

OK, do you also put the selected one at the top even if it's not a newly created
one?

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model_unittest.cc
(right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:23:
EXPECT_FALSE(model.IsItemSeparatorAt(0));
On 2017/05/02 22:58:55, sebsg wrote:
> Can you also check that it's not at (1)?

I can't since there's only one item. :-)

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:28:
TEST(AddressComboboxModelTest, TwoAddresses) {
On 2017/05/02 22:58:55, sebsg wrote:
> Can you also add a test with only one address? Thanks!

We kind of do that in the test where we add a second one, but OK. :-)

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:42: // The
order is not guaranteed.
On 2017/05/02 22:58:55, sebsg wrote:
> Since you get profiles to suggest to get the billing address, one thing you
> could do is to set a use count to one of the profiles
> 
> For example, if you set
> 
> profile1.set_use_count(100);
> 
> profile1 will always be shown before profile2 (which will have a default use
> count of 1) since GetProfilesToSuggest gives you the profiles ordered by
> frecency.

Cool! Thanks...

Done.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/credit_card_unittest.cc:345: "1", nullptr,
true}));
On 2017/05/02 22:58:55, sebsg wrote:
> Can you add a test where the billing addresses are not the same? Thanks!

Done.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/validation.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/validation.cc:144: if
(card.billing_address_id().empty()) {
On 2017/05/02 22:58:55, sebsg wrote:
> This would be a bit more involved but we might want to check that the billing
> address id is associated with a profile that we have on file. We can add it
> later if you want (but let's create a bug in that case).

Done.

[MAD, 2017-05-03 17:04:35]

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

[commit-bot: I haz the power, 2017-05-03 17:05:04]

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-05-03 17:18:20]

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

[commit-bot: I haz the power, 2017-05-03 17:18:21]

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_...)

[MAD, 2017-05-03 17:20:04]

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

[commit-bot: I haz the power, 2017-05-03 17:20:47]

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-05-03 17:39:28]

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

[commit-bot: I haz the power, 2017-05-03 17:39:29]

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_...)

[MAD, 2017-05-03 17:50:24]

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

[commit-bot: I haz the power, 2017-05-03 17:50:54]

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-05-03 18:13:30]

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

[commit-bot: I haz the power, 2017-05-03 18:13:32]

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

[MAD, 2017-05-03 19:02:40]

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

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

Dry run: CQ is trying da patch.

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

[sebsg, 2017-05-03 19:19:29]

A few remaining questions, it's coming along well!

https://codereview.chromium.org/2849523003/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2849523003/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7738: <message
name="IDS_AUTOFILL_FIELD_LABEL_BILLING_ADDRESS" desc="The label of the Billing
Address chooser.">
On 2017/05/03 16:15:19, MAD wrote:
> On 2017/05/02 22:58:54, sebsg wrote:
> > For all of these strings (in autofill_strings and payments_strings), do you
> > think we could add them all in the autofill_strings.grd? If we add them in
> both
> > Payments and Autofill they will be translated twice and take up more space.
> What
> > do you think? 
> > 
> > They can both be accessed in the generated components_strings anyway no?
> > 
> > Also I would put them in Autofill over Payments since all the data comes
form
> > Autofill.
> 
> I was trying to follow the existing pattern. If I move these strings, I should
> also move the other ones above, right?

Ideally but We can do it after, the CL is big enough :)

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:39: // Add extra
items for the "Select" entry and a separator.
On 2017/05/03 16:15:19, MAD wrote:
> On 2017/05/02 22:58:55, sebsg wrote:
> > Do we also have the add address button if some addresses are present?
> 
> I'm not sure I understand. You mean the credit card controller may not need an
> add address button if the combobox is not empty?
> I personally think there should always be an option to add a new address
whether
> there are existing addresses or not.

Confusion! :) Yes I also think the option to add an address should always be
present. What confuses me is that you add 2 to the count (1 for the "Select" and
1 for the separator) but shouldn't it be 3, to also include the "Add Address"
option?

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:80:
profiles_cache_.push_back(base::MakeUnique<AutofillProfile>(profile));
On 2017/05/03 16:15:19, MAD wrote:
> On 2017/05/02 22:58:55, sebsg wrote:
> > One thing we did on Android is put the new address at the top of the list.
> That
> > way if the user clicks on the combobox again, the new address will be right
> > there at the top. 
> > 
> > Since the number of addresses is generally pretty low, it's not to expensive
> to
> > move all the other ones up.
> > 
> > What do you think?
> 
> OK, do you also put the selected one at the top even if it's not a newly
created
> one?

No, we put in in all caps, which we may or may not want to do here. The new
address is at the top only because it was just added. If you were to go back to
the main screen and then back again to the card editor, that billing address
doesn't have to be on top (Profiles ordered by frecency).

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model_unittest.cc
(right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:23:
EXPECT_FALSE(model.IsItemSeparatorAt(0));
On 2017/05/03 16:15:19, MAD wrote:
> On 2017/05/02 22:58:55, sebsg wrote:
> > Can you also check that it's not at (1)?
> 
> I can't since there's only one item. :-)

That makes sense :P

https://codereview.chromium.org/2849523003/diff/260001/chrome/browser/ui/view...
File
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2849523003/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc:504:

Can you add one where the billing address id is not empty but not referring to a
profile?

[commit-bot: I haz the power, 2017-05-03 19:58:03]

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

[commit-bot: I haz the power, 2017-05-03 19:58:04]

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_...)

[MAD, 2017-05-03 23:40:51]

Patchset #14 (id:280001) has been deleted

[MAD, 2017-05-03 23:43:33]

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

[commit-bot: I haz the power, 2017-05-03 23:44: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-05-03 23:50:47]

Thanks again... Prise 2!

BYE
MAD

https://codereview.chromium.org/2849523003/diff/140001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2849523003/diff/140001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7738: <message
name="IDS_AUTOFILL_FIELD_LABEL_BILLING_ADDRESS" desc="The label of the Billing
Address chooser.">
On 2017/05/03 19:19:29, sebsg wrote:
> On 2017/05/03 16:15:19, MAD wrote:
> > On 2017/05/02 22:58:54, sebsg wrote:
> > > For all of these strings (in autofill_strings and payments_strings), do
you
> > > think we could add them all in the autofill_strings.grd? If we add them in
> > both
> > > Payments and Autofill they will be translated twice and take up more
space.
> > What
> > > do you think? 
> > > 
> > > They can both be accessed in the generated components_strings anyway no?
> > > 
> > > Also I would put them in Autofill over Payments since all the data comes
> form
> > > Autofill.
> > 
> > I was trying to follow the existing pattern. If I move these strings, I
should
> > also move the other ones above, right?
> 
> Ideally but We can do it after, the CL is big enough :)

Acknowledged.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:39: // Add extra
items for the "Select" entry and a separator.
On 2017/05/03 19:19:29, sebsg wrote:
> On 2017/05/03 16:15:19, MAD wrote:
> > On 2017/05/02 22:58:55, sebsg wrote:
> > > Do we also have the add address button if some addresses are present?
> > 
> > I'm not sure I understand. You mean the credit card controller may not need
an
> > add address button if the combobox is not empty?
> > I personally think there should always be an option to add a new address
> whether
> > there are existing addresses or not.
> 
> Confusion! :) Yes I also think the option to add an address should always be
> present. What confuses me is that you add 2 to the count (1 for the "Select"
and
> 1 for the separator) but shouldn't it be 3, to also include the "Add Address"
> option?

Ha! You were talking about the Add Address entry which is only there when there
are no addresses, as opposed to the Add Address "button" which is aways there.
The idea is that, if there are available addresses, the user can select from
them and not need to add new one (but still can), but if there are no addresses
at all, the user doesn't have the choice but to add a new one, and in that case,
it wouldn't make sense to show the "Select" entry, nor the separator, since
there are no addresses to show and select.

Makes more sense?

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:80:
profiles_cache_.push_back(base::MakeUnique<AutofillProfile>(profile));
On 2017/05/03 19:19:29, sebsg wrote:
> On 2017/05/03 16:15:19, MAD wrote:
> > On 2017/05/02 22:58:55, sebsg wrote:
> > > One thing we did on Android is put the new address at the top of the list.
> > That
> > > way if the user clicks on the combobox again, the new address will be
right
> > > there at the top. 
> > > 
> > > Since the number of addresses is generally pretty low, it's not to
expensive
> > to
> > > move all the other ones up.
> > > 
> > > What do you think?
> > 
> > OK, do you also put the selected one at the top even if it's not a newly
> created
> > one?
> 
> No, we put in in all caps, which we may or may not want to do here. The new
> address is at the top only because it was just added. If you were to go back
to
> the main screen and then back again to the card editor, that billing address
> doesn't have to be on top (Profiles ordered by frecency).

What do we put all in caps? 

The usual behavior in desktop app, is to keep the new entry at the same place
but when the user click on the combobox, the selected item is highlighted, but I
now see that the Chrome UI doesn't do this anymore... :-(

It would not be expensive to reorder the items, but would make things a bit more
complex to manage with the "Select" and separator headers. I would rather leave
it as is if you don't mind.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model_unittest.cc
(right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model_unittest.cc:23:
EXPECT_FALSE(model.IsItemSeparatorAt(0));
On 2017/05/03 19:19:29, sebsg wrote:
> On 2017/05/03 16:15:19, MAD wrote:
> > On 2017/05/02 22:58:55, sebsg wrote:
> > > Can you also check that it's not at (1)?
> > 
> > I can't since there's only one item. :-)
> 
> That makes sense :P

Acknowledged.

https://codereview.chromium.org/2849523003/diff/260001/chrome/browser/ui/view...
File
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2849523003/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc:504:

On 2017/05/03 19:19:29, sebsg wrote:
> Can you add one where the billing address id is not empty but not referring to
a
> profile?

Good idea. 
Done.

[commit-bot: I haz the power, 2017-05-03 23:55:45]

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

[commit-bot: I haz the power, 2017-05-03 23:55:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[MAD, 2017-05-04 01:01:40]

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

[commit-bot: I haz the power, 2017-05-04 01:02:04]

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-05-04 01:36:23]

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

[commit-bot: I haz the power, 2017-05-04 01:36:24]

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_...)

[MAD, 2017-05-04 02:20:51]

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

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

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-05-04 02:51:38]

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

[commit-bot: I haz the power, 2017-05-04 02:51:39]

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_...)

[MAD, 2017-05-04 14:28:36]

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

[commit-bot: I haz the power, 2017-05-04 14:28:51]

Dry run: CQ is trying da patch.

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

[MAD, 2017-05-04 15:38:26]

Description was changed from

==========
Add billing address as a mandatory field of Payments credit cards.

BUG=709776
==========

to

==========
Add billing address as a mandatory field of Payments credit cards.

TBR=markusheintz@chromium.org
For trivial adaptation to API change in:
chrome/browser/browsing_data/autofill_counter_browsertest.cc

BUG=709776
==========

[MAD, 2017-05-04 15:38:26]

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

[commit-bot: I haz the power, 2017-05-04 15:46:50]

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

[commit-bot: I haz the power, 2017-05-04 15:46:51]

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-05-04 16:50:54]

LGTM!  

I created crbug.com/718467 on my side to check why an empty billing address is
necessary in some tests.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:39: // Add extra
items for the "Select" entry and a separator.
On 2017/05/03 23:50:47, MAD wrote:
> On 2017/05/03 19:19:29, sebsg wrote:
> > On 2017/05/03 16:15:19, MAD wrote:
> > > On 2017/05/02 22:58:55, sebsg wrote:
> > > > Do we also have the add address button if some addresses are present?
> > > 
> > > I'm not sure I understand. You mean the credit card controller may not
need
> an
> > > add address button if the combobox is not empty?
> > > I personally think there should always be an option to add a new address
> > whether
> > > there are existing addresses or not.
> > 
> > Confusion! :) Yes I also think the option to add an address should always be
> > present. What confuses me is that you add 2 to the count (1 for the "Select"
> and
> > 1 for the separator) but shouldn't it be 3, to also include the "Add
Address"
> > option?
> 
> Ha! You were talking about the Add Address entry which is only there when
there
> are no addresses, as opposed to the Add Address "button" which is aways there.
> The idea is that, if there are available addresses, the user can select from
> them and not need to add new one (but still can), but if there are no
addresses
> at all, the user doesn't have the choice but to add a new one, and in that
case,
> it wouldn't make sense to show the "Select" entry, nor the separator, since
> there are no addresses to show and select.
> 
> Makes more sense?

Ahh I didn't realize the "Add address" was a button, on Android it is in the
combobox. Makes sense then.

https://codereview.chromium.org/2849523003/diff/140001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:80:
profiles_cache_.push_back(base::MakeUnique<AutofillProfile>(profile));
On 2017/05/03 23:50:47, MAD wrote:
> On 2017/05/03 19:19:29, sebsg wrote:
> > On 2017/05/03 16:15:19, MAD wrote:
> > > On 2017/05/02 22:58:55, sebsg wrote:
> > > > One thing we did on Android is put the new address at the top of the
list.
> > > That
> > > > way if the user clicks on the combobox again, the new address will be
> right
> > > > there at the top. 
> > > > 
> > > > Since the number of addresses is generally pretty low, it's not to
> expensive
> > > to
> > > > move all the other ones up.
> > > > 
> > > > What do you think?
> > > 
> > > OK, do you also put the selected one at the top even if it's not a newly
> > created
> > > one?
> > 
> > No, we put in in all caps, which we may or may not want to do here. The new
> > address is at the top only because it was just added. If you were to go back
> to
> > the main screen and then back again to the card editor, that billing address
> > doesn't have to be on top (Profiles ordered by frecency).
> 
> What do we put all in caps? 
> 
> The usual behavior in desktop app, is to keep the new entry at the same place
> but when the user click on the combobox, the selected item is highlighted, but
I
> now see that the Chrome UI doesn't do this anymore... :-(
> 
> It would not be expensive to reorder the items, but would make things a bit
more
> complex to manage with the "Select" and separator headers. I would rather
leave
> it as is if you don't mind.

I think it's good as it is and we can ask Bruno for his UI expert opinion :)

[MAD, 2017-05-04 19:56:14]

OK, Mathieu, we're only missing your OWNER LGTM for the chrome\browser files
now.

Thanks!

BYE
MAD

[Mathieu, 2017-05-04 20:29:20]

Very cool, some suggestions!

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:56: const
auto kBillingAddressType = autofill::MAX_VALID_FIELD_TYPE;
we could use ADDRESS_BILLING_LINE_1, which is otherwise not used. It doesn't fit
perfectly but it could work.

https://cs.chromium.org/chromium/src/components/autofill/core/browser/field_t...

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:162:
std::unique_ptr<views::View>
I would add ASCII art about what you are building here so that it's easy to
modify later. See an example in editor_view_controller:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/editor_...

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:203: int
selected_index = -1;
nit: I would move this closer to the if block

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:204:
std::unique_ptr<autofill::AddressComboboxModel> address_combobox_model(
can use = here?

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:396:
base::OnceClosure(),
TODO: there is a possibility that the billing address is not complete and we
will want to edit it before assigning :)

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:438: //
The address combobox value only need to not be an empty item identifier.
rephrase/fix?

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.h
(right):

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:96: //
Called when a new address was created to be used as the billing address.
mention the lifetime of |profile|?

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
File
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc:322:
PaymentRequestBrowserTestBase::AddAutofillProfile(billing_profile);
no need for PaymentRequestBrowserTestBase:

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:241: NOTREACHED();
I add an idea when looking at this. You could define a special ControlType
similar to COMBOBOX or TEXTFIELD, and if this base class doesn't know about it,
it could query the subclass to get the view for it.

this line would become
layout->AddView(GetViewForControlType(field.control_type). 

In the subclass you would save all the layout/label creation, which should be
the same for all fields. It doesn't preclude you from having layouts *inside*
the view you are returning to the base class.

Also this way we can have special input fields all over the view, not just at
the end. 

Let me know if this would work.

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:92: //
|back_navigation_type| identify which other view to navigate to when the
*identifies what sort of back navigation should be done when editing is
successful. This is independent of the back arrow which always goes back one
step.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:144: // intput fields
and their label.
*input

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:120: int
next_ui_tag,
can you document this one

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:93:
ShippingAddressEditorViewController::CreateCustomFieldsView() {
A pattern that we sometimes use is that you could have the base implementation
of this return nullptr in EditorViewController, and when it's time to use it,
do.

... custom_view = CreateCustomFieldsView();
if (custom_view) {
  view->AddChildView(custom_view);
}

You would then avoid these empty implementations.

https://codereview.chromium.org/2849523003/diff/350001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/350001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:126: base::string16
ellipsis(base::ASCIIToUTF16("..."));
There appears to be kEllipsisUTF16 already defined in gfx::

Also could you use gfx::ElideText?

https://cs.chromium.org/chromium/src/ui/gfx/text_elider.cc?rcl=51adbbad187d9f...

https://codereview.chromium.org/2849523003/diff/350001/components/payments/co...
File components/payments/core/autofill_payment_instrument_unittest.cc (right):

https://codereview.chromium.org/2849523003/diff/350001/components/payments/co...
components/payments/core/autofill_payment_instrument_unittest.cc:95:
TEST_F(AutofillPaymentInstrumentTest, IsCompleteForPayment_NoName) {
can we have a test for when there is no billing address?

[MAD, 2017-05-05 00:01:41]

Patchset #18 (id:370001) has been deleted

[MAD, 2017-05-05 00:23:58]

Patchset #19 (id:410001) has been deleted

[MAD, 2017-05-05 00:49:46]

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

[MAD, 2017-05-05 00:50:07]

Thanks Math!

How about this?

BYE
MAD...

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:56: const
auto kBillingAddressType = autofill::MAX_VALID_FIELD_TYPE;
On 2017/05/04 20:29:19, Mathieu wrote:
> we could use ADDRESS_BILLING_LINE_1, which is otherwise not used. It doesn't
fit
> perfectly but it could work.
> 
>
https://cs.chromium.org/chromium/src/components/autofill/core/browser/field_t...

Done.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:162:
std::unique_ptr<views::View>
On 2017/05/04 20:29:19, Mathieu wrote:
> I would add ASCII art about what you are building here so that it's easy to
> modify later. See an example in editor_view_controller:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/editor_...

Done.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:203: int
selected_index = -1;
On 2017/05/04 20:29:19, Mathieu wrote:
> nit: I would move this closer to the if block

Done.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:204:
std::unique_ptr<autofill::AddressComboboxModel> address_combobox_model(
On 2017/05/04 20:29:18, Mathieu wrote:
> can use = here?

OK, but I thought that constructor were prefered in cases like these.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:396:
base::OnceClosure(),
On 2017/05/04 20:29:19, Mathieu wrote:
> TODO: there is a possibility that the billing address is not complete and we
> will want to edit it before assigning :)

Really? Should't the editor prevent the creation of an invalid address?

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:438: //
The address combobox value only need to not be an empty item identifier.
On 2017/05/04 20:29:19, Mathieu wrote:
> rephrase/fix?

Done.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.h
(right):

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:96: //
Called when a new address was created to be used as the billing address.
On 2017/05/04 20:29:19, Mathieu wrote:
> mention the lifetime of |profile|?

Why? It's a const ref that shouldn't be kept beyond this call.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
File
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc:322:
PaymentRequestBrowserTestBase::AddAutofillProfile(billing_profile);
On 2017/05/04 20:29:19, Mathieu wrote:
> no need for PaymentRequestBrowserTestBase:

Done.

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:241: NOTREACHED();
On 2017/05/04 20:29:19, Mathieu wrote:
> I add an idea when looking at this. You could define a special ControlType
> similar to COMBOBOX or TEXTFIELD, and if this base class doesn't know about
it,
> it could query the subclass to get the view for it.
> 
> this line would become
> layout->AddView(GetViewForControlType(field.control_type). 
> 
> In the subclass you would save all the layout/label creation, which should be
> the same for all fields. It doesn't preclude you from having layouts *inside*
> the view you are returning to the base class.
> 
> Also this way we can have special input fields all over the view, not just at
> the end. 
> 
> Let me know if this would work.

Excellent idea... Done!

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:92: //
|back_navigation_type| identify which other view to navigate to when the
On 2017/05/04 20:29:19, Mathieu wrote:
> *identifies what sort of back navigation should be done when editing is
> successful. This is independent of the back arrow which always goes back one
> step.

Done.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:144: // intput fields
and their label.
On 2017/05/04 20:29:19, Mathieu wrote:
> *input

Done.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:120: int
next_ui_tag,
On 2017/05/04 20:29:19, Mathieu wrote:
> can you document this one

Done.

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:93:
ShippingAddressEditorViewController::CreateCustomFieldsView() {
On 2017/05/04 20:29:19, Mathieu wrote:
> A pattern that we sometimes use is that you could have the base implementation
> of this return nullptr in EditorViewController, and when it's time to use it,
> do.
> 
> ... custom_view = CreateCustomFieldsView();
> if (custom_view) {
>   view->AddChildView(custom_view);
> }
> 
> You would then avoid these empty implementations.

I was following the pattern used by other similar methods (e.g.,
CreateHeaderView). Should we change those other too?

https://codereview.chromium.org/2849523003/diff/350001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/350001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:126: base::string16
ellipsis(base::ASCIIToUTF16("..."));
On 2017/05/04 20:29:19, Mathieu wrote:
> There appears to be kEllipsisUTF16 already defined in gfx::
> 
> Also could you use gfx::ElideText?
> 
>
https://cs.chromium.org/chromium/src/ui/gfx/text_elider.cc?rcl=51adbbad187d9f...
> 

ElideText requires a font list which we don't have, and ElideString always try
put the ellipsis in the middle if it fits.

Thanks for kEllipsisUTF16, I had not noticed it, and I found interesting that
it's not always used, even within text_elider.cc... weird...

https://codereview.chromium.org/2849523003/diff/350001/components/payments/co...
File components/payments/core/autofill_payment_instrument_unittest.cc (right):

https://codereview.chromium.org/2849523003/diff/350001/components/payments/co...
components/payments/core/autofill_payment_instrument_unittest.cc:95:
TEST_F(AutofillPaymentInstrumentTest, IsCompleteForPayment_NoName) {
On 2017/05/04 20:29:19, Mathieu wrote:
> can we have a test for when there is no billing address?

Done.

[commit-bot: I haz the power, 2017-05-05 00:50:09]

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-05-05 00:54:23]

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

[commit-bot: I haz the power, 2017-05-05 00:54:24]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[MAD, 2017-05-05 13:35:47]

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

[commit-bot: I haz the power, 2017-05-05 13:36: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-05-05 13:51:33]

A few small things and this is good to go!

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:396:
base::OnceClosure(),
On 2017/05/05 00:50:07, MAD wrote:
> On 2017/05/04 20:29:19, Mathieu wrote:
> > TODO: there is a possibility that the billing address is not complete and we
> > will want to edit it before assigning :)
> 
> Really? Should't the editor prevent the creation of an invalid address?

We may have an incomplete billing address because of the Autofill editor, which
doesn't strictly enforce the same criteria.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.h
(right):

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:96: //
Called when a new address was created to be used as the billing address.
On 2017/05/05 00:50:07, MAD wrote:
> On 2017/05/04 20:29:19, Mathieu wrote:
> > mention the lifetime of |profile|?
> 
> Why? It's a const ref that shouldn't be kept beyond this call.

I haven't double-checked but I think |profile| is very short-lived, because the
editor is destroyed after this call. Therefore AddAndSelectNewBillingAddress (or
the combobox model) should possibly make a copy? If that's the case, we might
want to mention it here to make sure it's not changed in the future.

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:93:
ShippingAddressEditorViewController::CreateCustomFieldsView() {
On 2017/05/05 00:50:07, MAD wrote:
> On 2017/05/04 20:29:19, Mathieu wrote:
> > A pattern that we sometimes use is that you could have the base
implementation
> > of this return nullptr in EditorViewController, and when it's time to use
it,
> > do.
> > 
> > ... custom_view = CreateCustomFieldsView();
> > if (custom_view) {
> >   view->AddChildView(custom_view);
> > }
> > 
> > You would then avoid these empty implementations.
> 
> I was following the pattern used by other similar methods (e.g.,
> CreateHeaderView). Should we change those other too?

I have a slightly preference for what I suggested because otherwise you are
adding empty views to the layout, which is theoretically more work. But I agree
with you it's not done consistently.

Also this way, it makes it so that some subclasses don't have to override
CreateCustomFieldsView if they don't need to.

https://codereview.chromium.org/2849523003/diff/350001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/350001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:126: base::string16
ellipsis(base::ASCIIToUTF16("..."));
On 2017/05/05 00:50:07, MAD wrote:
> On 2017/05/04 20:29:19, Mathieu wrote:
> > There appears to be kEllipsisUTF16 already defined in gfx::
> > 
> > Also could you use gfx::ElideText?
> > 
> >
>
https://cs.chromium.org/chromium/src/ui/gfx/text_elider.cc?rcl=51adbbad187d9f...
> > 
> 
> ElideText requires a font list which we don't have, and ElideString always try
> put the ellipsis in the middle if it fits.
> 
> Thanks for kEllipsisUTF16, I had not noticed it, and I found interesting that
> it's not always used, even within text_elider.cc... weird...

I find this "cutting to 20" a bit weird, I'm not sure if it could produce weird
results in some circumstances...

https://codereview.chromium.org/2849523003/diff/490001/chrome/browser/ui/view...
File
chrome/browser/ui/views/payments/error_message_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2849523003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/error_message_view_controller_browsertest.cc:31:
PaymentRequestBrowserTestBase::AddAutofillProfile(billing_profile);
remove PaymentRequestBrowserTestBase:: in all the tests

[Mathieu, 2017-05-05 13:59:07]

https://codereview.chromium.org/2849523003/diff/350001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/350001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:126: base::string16
ellipsis(base::ASCIIToUTF16("..."));
On 2017/05/05 13:51:33, Mathieu wrote:
> On 2017/05/05 00:50:07, MAD wrote:
> > On 2017/05/04 20:29:19, Mathieu wrote:
> > > There appears to be kEllipsisUTF16 already defined in gfx::
> > > 
> > > Also could you use gfx::ElideText?
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/gfx/text_elider.cc?rcl=51adbbad187d9f...
> > > 
> > 
> > ElideText requires a font list which we don't have, and ElideString always
try
> > put the ellipsis in the middle if it fits.
> > 
> > Thanks for kEllipsisUTF16, I had not noticed it, and I found interesting
that
> > it's not always used, even within text_elider.cc... weird...
> 
> I find this "cutting to 20" a bit weird, I'm not sure if it could produce
weird
> results in some circumstances... 

Also I've asked Anthony and gfx::ElideRectangleText seems to be a better method
for what you want: 

https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/payment...

[commit-bot: I haz the power, 2017-05-05 14:18:07]

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

[commit-bot: I haz the power, 2017-05-05 14:18:12]

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

[MAD, 2017-05-05 18:24:19]

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

[commit-bot: I haz the power, 2017-05-05 18:24:53]

Dry run: CQ is trying da patch.

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

[MAD, 2017-05-05 18:32:34]

OK, all addressed, and fixed new test that came with a rebase.

I put screenshots here:
https://drive.google.com/drive/folders/0ByHueeWLQ0j9Sm9YZW5FUUI4NjA

Thanks!

BYE
MAD

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:396:
base::OnceClosure(),
On 2017/05/05 13:51:33, Mathieu wrote:
> On 2017/05/05 00:50:07, MAD wrote:
> > On 2017/05/04 20:29:19, Mathieu wrote:
> > > TODO: there is a possibility that the billing address is not complete and
we
> > > will want to edit it before assigning :)
> > 
> > Really? Should't the editor prevent the creation of an invalid address?
> 
> We may have an incomplete billing address because of the Autofill editor,
which
> doesn't strictly enforce the same criteria.

HA OK! Got it... Added a TODO and a bug.

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.h
(right):

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:96: //
Called when a new address was created to be used as the billing address.
On 2017/05/05 13:51:33, Mathieu wrote:
> On 2017/05/05 00:50:07, MAD wrote:
> > On 2017/05/04 20:29:19, Mathieu wrote:
> > > mention the lifetime of |profile|?
> > 
> > Why? It's a const ref that shouldn't be kept beyond this call.
> 
> I haven't double-checked but I think |profile| is very short-lived, because
the
> editor is destroyed after this call. Therefore AddAndSelectNewBillingAddress
(or
> the combobox model) should possibly make a copy? If that's the case, we might
> want to mention it here to make sure it's not changed in the future.

Yes, the lifespan of the passed in profile is unknown at this point, but as I
said, it should be known to the implementers of this method that they can't keep
a reference to this object unless it is clear that the lifespan is long enough.
I'll a comment, but I feel it's over-defensive, since it's only in the cases
where we would need it to outlive us that we would mention it, but anyway, I'll
go with your preference.

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

https://codereview.chromium.org/2849523003/diff/350001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:93:
ShippingAddressEditorViewController::CreateCustomFieldsView() {
On 2017/05/05 13:51:33, Mathieu wrote:
> On 2017/05/05 00:50:07, MAD wrote:
> > On 2017/05/04 20:29:19, Mathieu wrote:
> > > A pattern that we sometimes use is that you could have the base
> implementation
> > > of this return nullptr in EditorViewController, and when it's time to use
> it,
> > > do.
> > > 
> > > ... custom_view = CreateCustomFieldsView();
> > > if (custom_view) {
> > >   view->AddChildView(custom_view);
> > > }
> > > 
> > > You would then avoid these empty implementations.
> > 
> > I was following the pattern used by other similar methods (e.g.,
> > CreateHeaderView). Should we change those other too?
> 
> I have a slightly preference for what I suggested because otherwise you are
> adding empty views to the layout, which is theoretically more work. But I
agree
> with you it's not done consistently.
> 
> Also this way, it makes it so that some subclasses don't have to override
> CreateCustomFieldsView if they don't need to.

OK, then I'll change all the others too.
Note that for the custom fields, it should only be called when a derived class
has explicitly specified it needed one.

https://codereview.chromium.org/2849523003/diff/350001/components/autofill/co...
File components/autofill/core/browser/address_combobox_model.cc (right):

https://codereview.chromium.org/2849523003/diff/350001/components/autofill/co...
components/autofill/core/browser/address_combobox_model.cc:126: base::string16
ellipsis(base::ASCIIToUTF16("..."));
On 2017/05/05 13:59:07, Mathieu wrote:
> On 2017/05/05 13:51:33, Mathieu wrote:
> > On 2017/05/05 00:50:07, MAD wrote:
> > > On 2017/05/04 20:29:19, Mathieu wrote:
> > > > There appears to be kEllipsisUTF16 already defined in gfx::
> > > > 
> > > > Also could you use gfx::ElideText?
> > > > 
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/gfx/text_elider.cc?rcl=51adbbad187d9f...
> > > > 
> > > 
> > > ElideText requires a font list which we don't have, and ElideString always
> try
> > > put the ellipsis in the middle if it fits.
> > > 
> > > Thanks for kEllipsisUTF16, I had not noticed it, and I found interesting
> that
> > > it's not always used, even within text_elider.cc... weird...
> > 
> > I find this "cutting to 20" a bit weird, I'm not sure if it could produce
> weird
> > results in some circumstances... 
> 
> Also I've asked Anthony and gfx::ElideRectangleText seems to be a better
method
> for what you want: 
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/payments/payment...
> 

I talked to Anthony and we found this simpler solution instead. :-)

https://codereview.chromium.org/2849523003/diff/490001/chrome/browser/ui/view...
File
chrome/browser/ui/views/payments/error_message_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2849523003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/error_message_view_controller_browsertest.cc:31:
PaymentRequestBrowserTestBase::AddAutofillProfile(billing_profile);
On 2017/05/05 13:51:33, Mathieu wrote:
> remove PaymentRequestBrowserTestBase:: in all the tests

Done.

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

https://codereview.chromium.org/2849523003/diff/510001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:180:
views::GridLayout::FIXED, kMaximumLabelWidth, 0);
We could also use kNumCharactersInLongField if you preferm but then we need to
convert it to pixels using the font list.

[Mathieu, 2017-05-05 18:56:04]

lgtm, thanks!

[commit-bot: I haz the power, 2017-05-05 19:28:48]

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

[commit-bot: I haz the power, 2017-05-05 19:28:49]

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

[MAD, 2017-05-08 18:31:17]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-05-08 18:31:18]

The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org,
mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2849523003/#ps530001
(title: "Fixed a new test brought in by a Rebase")

[commit-bot: I haz the power, 2017-05-08 18:32: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-05-08 21:30:03]

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

[commit-bot: I haz the power, 2017-05-08 21:30:06]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[MAD, 2017-05-09 02:35:04]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2017-05-09 02:35:55]

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-05-09 05:12:59]

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

[commit-bot: I haz the power, 2017-05-09 05:13:01]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[MAD, 2017-05-09 15:08:57]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-05-09 15:08:58]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
sebsg@chromium.org
Link to the patchset: https://codereview.chromium.org/2849523003/#ps550001
(title: "Merge branch 'master' into billing")

[commit-bot: I haz the power, 2017-05-09 15:09:23]

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-05-09 21:35:20]

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

[commit-bot: I haz the power, 2017-05-09 21:35:23]

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_...)

[MAD, 2017-05-10 01:03:14]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2017-05-10 01:04:42]

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-05-10 03:08:39]

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

[commit-bot: I haz the power, 2017-05-10 03:08:42]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[MAD, 2017-05-10 11:22:37]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2017-05-10 11:23:33]

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-05-10 11:30:06]

CQ is committing da patch.

Bot data: {"patchset_id": 550001, "attempt_start_ts": 1494415357087390,
"parent_rev": "5ff993e5d0a0ef8d28d32c8f7daff78482acfa66", "commit_rev":
"faf8e1ebbc65063a01ada0f0c5ac8981076210cb"}

[commit-bot: I haz the power, 2017-05-10 11:30:18]

Description was changed from

==========
Add billing address as a mandatory field of Payments credit cards.

TBR=markusheintz@chromium.org
For trivial adaptation to API change in:
chrome/browser/browsing_data/autofill_counter_browsertest.cc

BUG=709776
==========

to

==========
Add billing address as a mandatory field of Payments credit cards.

TBR=markusheintz@chromium.org
For trivial adaptation to API change in:
chrome/browser/browsing_data/autofill_counter_browsertest.cc

BUG=709776

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

[commit-bot: I haz the power, 2017-05-10 11:30:20]

Committed patchset #25 (id:550001) as
https://chromium.googlesource.com/chromium/src/+/faf8e1ebbc65063a01ada0f0c5ac...