[Payments] Added region load failure tolerance and tests to PR editor.

[Payments] Added region load failure tolerance and tests to PR editor.

BUG=703758
TEST=browser_tests

Review-Url: https://codereview.chromium.org/2803443003
Cr-Commit-Position: refs/heads/master@{#464427}
Committed: https://chromium.googlesource.com/chromium/src/+/45271937a3aeee0c06e267d33e75d7ea0bee9605

[MAD, 2017-04-04 20:28:23]

Patchset #1 (id:1) has been deleted

[MAD, 2017-04-04 20:32:14]

Patchset #1 (id:20001) has been deleted

[MAD, 2017-04-04 20:32:26]

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

[commit-bot: I haz the power, 2017-04-04 20:33:03]

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-04-04 20:44:39]

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

[commit-bot: I haz the power, 2017-04-04 20:44:40]

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...)
  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-04-04 20:55:44]

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

[commit-bot: I haz the power, 2017-04-04 20:56:46]

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

[MAD, 2017-04-04 20:59:27]

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

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

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

[commit-bot: I haz the power, 2017-04-04 21:33:00]

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_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Mathieu, 2017-04-05 00:03:39]

Description was changed from

==========
Added region load failure tolerance and tests to payment address editor.

BUG=None
==========

to

==========
[Payments] Added region load failure tolerance and tests to PR editor.

BUG=703758
TEST=browser_tests
==========

[Mathieu, 2017-04-05 00:25:14]

Hi MAD! Thanks for these tests. A few comments

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

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:61: void
ComboboxModelChanged(views::Combobox* combobox) override{};
nit: space between override {}

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
(right):

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:166:
// we have check for failure here.
nit: *we have to check

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h
(right):

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:92:
// there is no data in the combox and it must be converted to a text field.
nit: combobox

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
File
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:48:
const char PaymentRequestShippingAddressEditorTest::kNameFull[] = "Bob Jones";
nit: put these above the test class?

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:53:
const char PaymentRequestShippingAddressEditorTest::kHomeZipp[] = "ziiiiiip";
nit: kHomeZip :)

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:59:
OpenPaymentMethodScreen();
this one is not needed

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:69:
int selected_country_row = combobox->GetSelectedRow();
It took me a bit of time to understand L65-L74 but now I get it. Could we add a
comment in the block saying something like

// Getting the value of the default selected country code.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:76:
// The view update will invalidate the combobox / model pointers.
Do we need to clear them? They weren't used anyway after line 71...

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:82:
// Verifying the data is in the DB.
Let's bring this block to line 88

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:114:
OpenPaymentMethodScreen();
remove

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:139:
ResetEventObserver(DialogEvent::EDITOR_VIEW_UPDATED);
would reset the observer before the event causing it happens (before 132?), and
WaitForObserved event after the event happened.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:148:
combobox = static_cast<views::Combobox*>(dialog_view()->GetViewByID(
do we need this statement?

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:175:
combobox = nullptr;
same comment about need to clear?

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:179:
ResetEventObserver(DialogEvent::EDITOR_VIEW_UPDATED);
Since the event that will trigger the view update is started at line 172, I
would move this statement before then. 

If it works as coded now, it probably means there is a bug in the
DialogEventObserver, right? Basically if I understand correctly, L172 will lead
to EDITOR_VIEW_UPDATED being triggered, with no observer to listen to it (this
is non-crashing). However I'm not sure how L179-L180 can work, because the event
occurred in the past... What do you think?

[MAD, 2017-04-07 18:50:18]

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

[MAD, 2017-04-07 18:50:41]

Thanks for the comments. How about this?

BYE
MAD

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

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:61: void
ComboboxModelChanged(views::Combobox* combobox) override{};
On 2017/04/05 00:25:14, Mathieu wrote:
> nit: space between override {}

Done.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
(right):

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:166:
// we have check for failure here.
On 2017/04/05 00:25:14, Mathieu wrote:
> nit: *we have to check

Done.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h
(right):

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:92:
// there is no data in the combox and it must be converted to a text field.
On 2017/04/05 00:25:14, Mathieu wrote:
> nit: combobox

Done.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
File
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:48:
const char PaymentRequestShippingAddressEditorTest::kNameFull[] = "Bob Jones";
On 2017/04/05 00:25:14, Mathieu wrote:
> nit: put these above the test class?

Can't, they are data member of the class, so they must be declared first.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:53:
const char PaymentRequestShippingAddressEditorTest::kHomeZipp[] = "ziiiiiip";
On 2017/04/05 00:25:14, Mathieu wrote:
> nit: kHomeZip :)

Done.:)

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:59:
OpenPaymentMethodScreen();
On 2017/04/05 00:25:14, Mathieu wrote:
> this one is not needed

Oupsss... :-)
Done.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:69:
int selected_country_row = combobox->GetSelectedRow();
On 2017/04/05 00:25:14, Mathieu wrote:
> It took me a bit of time to understand L65-L74 but now I get it. Could we add
a
> comment in the block saying something like
> 
> // Getting the value of the default selected country code.

Done.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:76:
// The view update will invalidate the combobox / model pointers.
On 2017/04/05 00:25:14, Mathieu wrote:
> Do we need to clear them? They weren't used anyway after line 71...

Defensive programming, to make sure they don't get used in future code updates.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:82:
// Verifying the data is in the DB.
On 2017/04/05 00:25:14, Mathieu wrote:
> Let's bring this block to line 88

Done.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:114:
OpenPaymentMethodScreen();
On 2017/04/05 00:25:14, Mathieu wrote:
> remove

Done.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:139:
ResetEventObserver(DialogEvent::EDITOR_VIEW_UPDATED);
On 2017/04/05 00:25:14, Mathieu wrote:
> would reset the observer before the event causing it happens (before 132?),
and
> WaitForObserved event after the event happened.

Done.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:148:
combobox = static_cast<views::Combobox*>(dialog_view()->GetViewByID(
On 2017/04/05 00:25:14, Mathieu wrote:
> do we need this statement?

Yes, we need the combo box to get its model.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:175:
combobox = nullptr;
On 2017/04/05 00:25:14, Mathieu wrote:
> same comment about need to clear?

Acknowledged.

https://codereview.chromium.org/2803443003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:179:
ResetEventObserver(DialogEvent::EDITOR_VIEW_UPDATED);
On 2017/04/05 00:25:14, Mathieu wrote:
> Since the event that will trigger the view update is started at line 172, I
> would move this statement before then. 
> 
> If it works as coded now, it probably means there is a bug in the
> DialogEventObserver, right? Basically if I understand correctly, L172 will
lead
> to EDITOR_VIEW_UPDATED being triggered, with no observer to listen to it (this
> is non-crashing). However I'm not sure how L179-L180 can work, because the
event
> occurred in the past... What do you think?

No bug, it works because the change is asynchronous and only happens when we let
the message loop pump messages.

[commit-bot: I haz the power, 2017-04-07 18:50:58]

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

[Mathieu, 2017-04-07 18:56:03]

lgtm, thanks!

[sebsg, 2017-04-07 19:07:32]

You don't need it, but LGTM too :)

[MAD, 2017-04-07 19:18:18]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-04-07 19:18:18]

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/2803443003/#ps140001
(title: "Fixed a minor goof.")

[commit-bot: I haz the power, 2017-04-07 19:19:14]

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

[commit-bot: I haz the power, 2017-04-07 19:28:23]

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

[commit-bot: I haz the power, 2017-04-07 19:28:25]

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

[MAD, 2017-04-11 19:31:03]

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

[commit-bot: I haz the power, 2017-04-11 19:31:34]

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

[MAD, 2017-04-11 19:31:36]

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

[MAD, 2017-04-11 19:31:36]

Fixed a unittests failure and added rouslan@chromium.org for OWNERs review.

Thanks!

BYE
MAD

[commit-bot: I haz the power, 2017-04-11 19:43:50]

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

[commit-bot: I haz the power, 2017-04-11 19:43:51]

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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[MAD, 2017-04-11 19:47:17]

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

[commit-bot: I haz the power, 2017-04-11 19:47:48]

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-04-11 20:04:35]

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

[commit-bot: I haz the power, 2017-04-11 20:04:36]

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-04-11 20:29:19]

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

[commit-bot: I haz the power, 2017-04-11 20:29:52]

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

[please use gerrit instead, 2017-04-11 21:05:35]

Looks good in general. A few minor comments.

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

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:71: void
ComboboxModelChanged(views::Combobox* combobox) override{};
Please add a space before "{}" and remove the ";".

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

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:209:
return std::unique_ptr<ui::ComboboxModel>(model.release());
This should be "return std::move(model);" instead. Otherwise the code creates an
extra std::unique_ptr, which is not necessary.

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

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:94:
bool failed_to_load_region_data_{false};
This should be initialized in the constructor like so:

ShippingAddressEditorViewController() : failed_to_load_region_data_(false) {}

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
File
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:53:
static const char kNameFull[];
In general, I prefer to use literal strings instead of constants in tests,
because they make tests more clear. However, if you prefer to use constants,
that's OK as well.

One approach I would recommend here is placing the constants inside an anonymous
namespace instead. That looks more natural, IMHO.

namespace payments {
namespace {

const char kNameFull[] = "Bob Jones";

}  // namespace

class PaymentRequestShippingAddressEditorTest ...

https://codereview.chromium.org/2803443003/diff/220001/components/autofill/co...
File components/autofill/core/browser/region_combobox_model.cc (right):

https://codereview.chromium.org/2803443003/diff/220001/components/autofill/co...
components/autofill/core/browser/region_combobox_model.cc:26:
region_data_supplier_(source.release(), storage.release()) {
Constructor initialization is neat to organize like this:

RegionComboboxModel::RegionComboboxModel() : pending_region_data_load_(true) {}

https://codereview.chromium.org/2803443003/diff/220001/components/autofill/co...
components/autofill/core/browser/region_combobox_model.cc:91: // For some
reason, some countries expose a state field but have not
Please remove "For some reason". That's a property of our data. ¯\_(ツ)_/¯

https://codereview.chromium.org/2803443003/diff/220001/components/autofill/co...
File components/autofill/core/browser/region_combobox_model.h (right):

https://codereview.chromium.org/2803443003/diff/220001/components/autofill/co...
components/autofill/core/browser/region_combobox_model.h:61: bool
pending_region_data_load_{false};
Please initialize in the constructor, unless this is the new style. If it's the
new style, please point me to the e-mail announcement. I miss those sometimes.
:)

[commit-bot: I haz the power, 2017-04-11 21:19:45]

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

[commit-bot: I haz the power, 2017-04-11 21:19:46]

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

[MAD, 2017-04-12 02:45:24]

Patchset #11 (id:240001) has been deleted

[MAD, 2017-04-12 02:45:53]

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

[MAD, 2017-04-12 02:46:08]

Thanks! How about this?

BYE
MAD

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

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:71: void
ComboboxModelChanged(views::Combobox* combobox) override{};
On 2017/04/11 21:05:35, ಠ_ಠ wrote:
> Please add a space before "{}" and remove the ";".

I was wondering why I kept changing this, and I just realized that git cl format
now does this.

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

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:209:
return std::unique_ptr<ui::ComboboxModel>(model.release());
On 2017/04/11 21:05:35, ಠ_ಠ wrote:
> This should be "return std::move(model);" instead. Otherwise the code creates
an
> extra std::unique_ptr, which is not necessary.

Done.

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

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h:94:
bool failed_to_load_region_data_{false};
On 2017/04/11 21:05:35, ಠ_ಠ wrote:
> This should be initialized in the constructor like so:
> 
> ShippingAddressEditorViewController() : failed_to_load_region_data_(false) {}

Acknowledged.

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
File
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:53:
static const char kNameFull[];
On 2017/04/11 21:05:35, ಠ_ಠ wrote:
> In general, I prefer to use literal strings instead of constants in tests,
> because they make tests more clear. However, if you prefer to use constants,
> that's OK as well.
> 
> One approach I would recommend here is placing the constants inside an
anonymous
> namespace instead. That looks more natural, IMHO.
> 
> namespace payments {
> namespace {
> 
> const char kNameFull[] = "Bob Jones";
> 
> }  // namespace
> 
> class PaymentRequestShippingAddressEditorTest ...

I prefer to use constants when the string is used more than once to make sure we
use the same string.

But I don't mind turning them into free statics as opposed to data members,
although I do prefer data members personally.

https://codereview.chromium.org/2803443003/diff/220001/components/autofill/co...
File components/autofill/core/browser/region_combobox_model.cc (right):

https://codereview.chromium.org/2803443003/diff/220001/components/autofill/co...
components/autofill/core/browser/region_combobox_model.cc:26:
region_data_supplier_(source.release(), storage.release()) {
On 2017/04/11 21:05:35, ಠ_ಠ wrote:
> Constructor initialization is neat to organize like this:
> 
> RegionComboboxModel::RegionComboboxModel() : pending_region_data_load_(true)
{}

I had done it like this to make it clear that it's because of the following call
to LoadRules. Is it clearer like this?

https://codereview.chromium.org/2803443003/diff/220001/components/autofill/co...
components/autofill/core/browser/region_combobox_model.cc:91: // For some
reason, some countries expose a state field but have not
On 2017/04/11 21:05:35, ಠ_ಠ wrote:
> Please remove "For some reason". That's a property of our data. ¯\_(ツ)_/¯

Done.

https://codereview.chromium.org/2803443003/diff/220001/components/autofill/co...
File components/autofill/core/browser/region_combobox_model.h (right):

https://codereview.chromium.org/2803443003/diff/220001/components/autofill/co...
components/autofill/core/browser/region_combobox_model.h:61: bool
pending_region_data_load_{false};
On 2017/04/11 21:05:35, ಠ_ಠ wrote:
> Please initialize in the constructor, unless this is the new style. If it's
the
> new style, please point me to the e-mail announcement. I miss those sometimes.
> :)

Here's the thread:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0

Though it's "allowed" an not imposed, but I've been asked in previous code
reviews to use this form. I don't mind changing it to constructor initialization
if you prefer.

[commit-bot: I haz the power, 2017-04-12 02:46:19]

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-04-12 02:54:17]

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

[commit-bot: I haz the power, 2017-04-12 02:54:18]

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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[MAD, 2017-04-12 03:02:35]

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

[commit-bot: I haz the power, 2017-04-12 03:02:58]

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-04-12 03:11:11]

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

[commit-bot: I haz the power, 2017-04-12 03:11:12]

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

[MAD, 2017-04-12 11:03:59]

Patchset #12 (id:280001) has been deleted

[MAD, 2017-04-12 11:04:20]

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

[commit-bot: I haz the power, 2017-04-12 11:04:36]

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-04-12 13:25:09]

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

[commit-bot: I haz the power, 2017-04-12 13:25:11]

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

[please use gerrit instead, 2017-04-12 14:01:18]

LGTM % comments/questions

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

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:71: void
ComboboxModelChanged(views::Combobox* combobox) override{};
On 2017/04/12 02:46:07, MAD wrote:
> On 2017/04/11 21:05:35, ಠ_ಠ wrote:
> > Please add a space before "{}" and remove the ";".
> 
> I was wondering why I kept changing this, and I just realized that git cl
format
> now does this.

git cl format does this? Yikes! Please file a bug against it.

https://codereview.chromium.org/2803443003/diff/300001/components/autofill/co...
File components/autofill/core/browser/region_combobox_model.cc (right):

https://codereview.chromium.org/2803443003/diff/300001/components/autofill/co...
components/autofill/core/browser/region_combobox_model.cc:26:
pending_region_data_load_(false),
You've changed the initial value of pending_region_data_load_ from true in patch
10 to false in patch 12. Is that intentional?

[MAD, 2017-04-12 15:49:15]

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

[commit-bot: I haz the power, 2017-04-12 15:49:33]

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

[MAD, 2017-04-12 15:50:12]

Thanks!

All answered... 

Now I need to double check why ASAN is seeing leaks in the LibAddressInput code.
:-(

BYE
MAD

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

https://codereview.chromium.org/2803443003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.h:71: void
ComboboxModelChanged(views::Combobox* combobox) override{};
On 2017/04/12 14:01:17, ಠ_ಠ wrote:
> On 2017/04/12 02:46:07, MAD wrote:
> > On 2017/04/11 21:05:35, ಠ_ಠ wrote:
> > > Please add a space before "{}" and remove the ";".
> > 
> > I was wondering why I kept changing this, and I just realized that git cl
> format
> > now does this.
> 
> git cl format does this? Yikes! Please file a bug against it.

D'Ho! I realized that I forgot to remove the semi-column and it's only with the
semi-column there that git cl format removes the space, which "might" be OK....

https://codereview.chromium.org/2803443003/diff/300001/components/autofill/co...
File components/autofill/core/browser/region_combobox_model.cc (right):

https://codereview.chromium.org/2803443003/diff/300001/components/autofill/co...
components/autofill/core/browser/region_combobox_model.cc:26:
pending_region_data_load_(false),
On 2017/04/12 14:01:17, ಠ_ಠ wrote:
> You've changed the initial value of pending_region_data_load_ from true in
patch
> 10 to false in patch 12. Is that intentional?

Yes, as explained poorly in answer to the previous comment (:-), the initial
state should be false, and only be set to true when there is a pending region
data load. It just so happen that the current implementation start a region data
in the constructor, but if we would ever change that, we wouldn't want the flag
to be set to true on construction.

So I moved the start of the region data load in a new separate function, and set
the flag to true in there. OK? Clearer now? :-)

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

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

[commit-bot: I haz the power, 2017-04-12 18:40:15]

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

[MAD, 2017-04-12 20:57:39]

Patchset #14 (id:340001) has been deleted

[MAD, 2017-04-13 12:00:03]

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

[commit-bot: I haz the power, 2017-04-13 12:00: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-04-13 12:13:57]

Please review again, at least against previous patch. I needed to add support to
mock the address input source/storage to avoid a leak.

Sorry about that...

Thanks!

BYE
MAD

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

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

[commit-bot: I haz the power, 2017-04-13 12:54:22]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-04-13 13:28:31]

Thanks, this is good lgtm

https://codereview.chromium.org/2803443003/diff/360001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2803443003/diff/360001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:17: #include
"third_party/libaddressinput/messages.h"
not sure we need this

[MAD, 2017-04-13 13:53:45]

Thanks Math, done!

Rouslan, all good?

BYE
MAD

[please use gerrit instead, 2017-04-13 14:17:42]

lgtm

[MAD, 2017-04-13 14:57:39]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-04-13 14:57:40]

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/2803443003/#ps380001
(title: "Removed unneeded include.")

[commit-bot: I haz the power, 2017-04-13 14:58:03]

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

[MAD, 2017-04-13 14:59:20]

Thanks! CQ' ing...

[commit-bot: I haz the power, 2017-04-13 16:07:43]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1492095459939260,
"parent_rev": "d348e46e81ea3d4996ee304ac47bc6b47495a7c4", "commit_rev":
"45271937a3aeee0c06e267d33e75d7ea0bee9605"}

[commit-bot: I haz the power, 2017-04-13 16:08:34]

Description was changed from

==========
[Payments] Added region load failure tolerance and tests to PR editor.

BUG=703758
TEST=browser_tests
==========

to

==========
[Payments] Added region load failure tolerance and tests to PR editor.

BUG=703758
TEST=browser_tests

Review-Url: https://codereview.chromium.org/2803443003
Cr-Commit-Position: refs/heads/master@{#464427}
Committed:
https://chromium.googlesource.com/chromium/src/+/45271937a3aeee0c06e267d33e75...
==========

[commit-bot: I haz the power, 2017-04-13 16:08:35]

Committed patchset #15 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/45271937a3aeee0c06e267d33e75...