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

chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc

Index: chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc
diff --git a/chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc b/chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..59219b7daae7959e91ba8a670defd5228bcd4794
--- /dev/null
+++ b/chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc
@@ -0,0 +1,208 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <algorithm>
+
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/ui/views/payments/payment_request_browsertest_base.h"
+#include "chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h"
+#include "chrome/browser/ui/views/payments/validating_textfield.h"
+#include "components/autofill/core/browser/autofill_country.h"
+#include "components/autofill/core/browser/country_combobox_model.h"
+#include "components/autofill/core/browser/personal_data_manager.h"
+#include "components/autofill/core/browser/region_combobox_model.h"
+#include "components/payments/content/payment_request_spec.h"
+#include "content/public/test/browser_test_utils.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/views/controls/combobox/combobox.h"
+
+namespace payments {
+
+class PaymentRequestShippingAddressEditorTest
+    : public PaymentRequestBrowserTestBase {
+ protected:
+  PaymentRequestShippingAddressEditorTest()
+      : PaymentRequestBrowserTestBase(
+            "/payment_request_dynamic_shipping_test.html") {}
+
+  PersonalDataLoadedObserverMock personal_data_observer_;
+
+  void SetRequiredFields() {
+    SetEditorTextfieldValue(base::ASCIIToUTF16(kNameFull), autofill::NAME_FULL);
+    SetEditorTextfieldValue(base::ASCIIToUTF16(kHomeAddress),
+                            autofill::ADDRESS_HOME_STREET_ADDRESS);
+    SetEditorTextfieldValue(base::ASCIIToUTF16(kHomeCity),
+                            autofill::ADDRESS_HOME_CITY);
+    SetEditorTextfieldValue(base::ASCIIToUTF16(kHomeZipp),
+                            autofill::ADDRESS_HOME_ZIP);
+  }
+  static const char kNameFull[];
+  static const char kHomeAddress[];
+  static const char kHomeCity[];
+  static const char kHomeZipp[];
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PaymentRequestShippingAddressEditorTest);
+};
+const char PaymentRequestShippingAddressEditorTest::kNameFull[] = "Bob Jones";

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

nit: put these above the test class?

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

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

+const char PaymentRequestShippingAddressEditorTest::kHomeAddress[] =
+    "42 Answers-All Avenue";
+const char PaymentRequestShippingAddressEditorTest::kHomeCity[] =
+    "Question-City";
+const char PaymentRequestShippingAddressEditorTest::kHomeZipp[] = "ziiiiiip";

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

nit: kHomeZip :)

[MAD, 2017/04/07 18:50:40]

Done.:)

+
+IN_PROC_BROWSER_TEST_F(PaymentRequestShippingAddressEditorTest,
+                       EnteringValidDataWithDefaultCountry) {
+  InvokePaymentRequestUI();
+
+  OpenPaymentMethodScreen();

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

this one is not needed

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

Oupsss... :-)
Done.

+
+  OpenShippingSectionScreen();
+
+  OpenShippingAddressEditorScreen();
+
+  views::Combobox* combobox =
+      static_cast<views::Combobox*>(dialog_view()->GetViewByID(
+          static_cast<int>(autofill::ADDRESS_HOME_COUNTRY)));
+  DCHECK(combobox);
+  int selected_country_row = combobox->GetSelectedRow();

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

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.

[MAD, 2017/04/07 18:50:40]

Done.

+  autofill::CountryComboboxModel* model =
+      static_cast<autofill::CountryComboboxModel*>(combobox->model());
+
+  std::string country_code(
+      model->countries()[selected_country_row]->country_code());
+
+  // The view update will invalidate the combobox / model pointers.

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

Do we need to clear them? They weren't used anyway after line 71...

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

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

+  combobox = nullptr;
+  model = nullptr;
+
+  SetRequiredFields();
+
+  // Verifying the data is in the DB.

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

Let's bring this block to line 88

[MAD, 2017/04/07 18:50:40]

Done.

+  autofill::PersonalDataManager* personal_data_manager = GetDataManager();
+  personal_data_manager->AddObserver(&personal_data_observer_);
+
+  ResetEventObserver(DialogEvent::BACK_NAVIGATION);
+
+  // Wait until the web database has been updated and the notification sent.
+  base::RunLoop data_loop;
+  EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged())
+      .WillOnce(QuitMessageLoop(&data_loop));
+  ClickOnDialogViewAndWait(DialogViewID::EDITOR_SAVE_BUTTON);
+  data_loop.Run();
+
+  ASSERT_EQ(1UL, personal_data_manager->GetProfiles().size());
+  autofill::AutofillProfile* profile = personal_data_manager->GetProfiles()[0];
+  DCHECK(profile);
+  EXPECT_EQ(base::ASCIIToUTF16(country_code),
+            profile->GetRawInfo(autofill::ADDRESS_HOME_COUNTRY));
+  EXPECT_EQ(base::ASCIIToUTF16(kNameFull),
+            profile->GetRawInfo(autofill::NAME_FULL));
+  EXPECT_EQ(base::ASCIIToUTF16(kHomeAddress),
+            profile->GetRawInfo(autofill::ADDRESS_HOME_STREET_ADDRESS));
+  EXPECT_EQ(base::ASCIIToUTF16(kHomeCity),
+            profile->GetRawInfo(autofill::ADDRESS_HOME_CITY));
+  EXPECT_EQ(base::ASCIIToUTF16(kHomeZipp),
+            profile->GetRawInfo(autofill::ADDRESS_HOME_ZIP));
+}
+
+IN_PROC_BROWSER_TEST_F(PaymentRequestShippingAddressEditorTest,
+                       SwitchingCountryUpdatesView) {
+  InvokePaymentRequestUI();
+
+  OpenPaymentMethodScreen();

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

remove

[MAD, 2017/04/07 18:50:40]

Done.

+
+  OpenShippingSectionScreen();
+
+  OpenShippingAddressEditorScreen();
+
+  views::Combobox* combobox =
+      static_cast<views::Combobox*>(dialog_view()->GetViewByID(
+          static_cast<int>(autofill::ADDRESS_HOME_COUNTRY)));
+  ASSERT_NE(nullptr, combobox);
+  ASSERT_EQ(0, combobox->GetSelectedRow());
+  autofill::CountryComboboxModel* model =
+      static_cast<autofill::CountryComboboxModel*>(combobox->model());
+  size_t num_countries = model->countries().size();
+  ASSERT_GT(num_countries, 10UL);
+  for (size_t country_index = 1; country_index < num_countries;
+       country_index += num_countries / 10) {
+    combobox->SetSelectedRow(country_index);
+    combobox->OnBlur();
+
+    // The view update will invalidate the combobox / model pointers.
+    combobox = nullptr;
+    model = nullptr;
+
+    // The editor updates asynchronously when the country changes.
+    ResetEventObserver(DialogEvent::EDITOR_VIEW_UPDATED);

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

would reset the observer before the event causing it happens (before 132?), and
WaitForObserved event after the event happened.

[MAD, 2017/04/07 18:50:40]

Done.

+    WaitForObservedEvent();
+
+    // Make sure the country combo box was properly reset to the chosen country.
+    combobox = static_cast<views::Combobox*>(dialog_view()->GetViewByID(
+        static_cast<int>(autofill::ADDRESS_HOME_COUNTRY)));
+    DCHECK(combobox);
+    EXPECT_EQ(country_index, static_cast<size_t>(combobox->GetSelectedRow()));
+
+    combobox = static_cast<views::Combobox*>(dialog_view()->GetViewByID(

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

do we need this statement?

[MAD, 2017/04/07 18:50:40]

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

+        static_cast<int>(autofill::ADDRESS_HOME_COUNTRY)));
+    DCHECK(combobox);
+    model = static_cast<autofill::CountryComboboxModel*>(combobox->model());
+    ASSERT_EQ(num_countries, model->countries().size());
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(PaymentRequestShippingAddressEditorTest,
+                       FailToLoadRegionData) {
+  InvokePaymentRequestUI();
+
+  OpenPaymentMethodScreen();
+
+  OpenShippingSectionScreen();
+
+  OpenShippingAddressEditorScreen();
+
+  views::Combobox* combobox =
+      static_cast<views::Combobox*>(dialog_view()->GetViewByID(
+          static_cast<int>(autofill::ADDRESS_HOME_STATE)));
+  ASSERT_NE(nullptr, combobox);
+  autofill::RegionComboboxModel* model =
+      static_cast<autofill::RegionComboboxModel*>(combobox->model());
+  model->SetFailureModeForTests(true);
+
+  // The view update will invalidate the combobox / model pointers.
+  combobox = nullptr;

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

same comment about need to clear?

[MAD, 2017/04/07 18:50:40]

Acknowledged.

+  model = nullptr;
+
+  // The editor updates asynchronously when the regions fail to load.
+  ResetEventObserver(DialogEvent::EDITOR_VIEW_UPDATED);

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

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:40]

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

+  WaitForObservedEvent();
+
+  // Now any textual value can be set as the state.
+  SetEditorTextfieldValue(base::ASCIIToUTF16("any state"),
+                          autofill::ADDRESS_HOME_STATE);
+
+  SetRequiredFields();
+
+  // Verifying the data is in the DB.
+  autofill::PersonalDataManager* personal_data_manager = GetDataManager();
+  personal_data_manager->AddObserver(&personal_data_observer_);
+
+  ResetEventObserver(DialogEvent::BACK_NAVIGATION);
+
+  // Wait until the web database has been updated and the notification sent.
+  base::RunLoop data_loop;
+  EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged())
+      .WillOnce(QuitMessageLoop(&data_loop));
+  ClickOnDialogViewAndWait(DialogViewID::EDITOR_SAVE_BUTTON);
+  data_loop.Run();
+
+  ASSERT_EQ(1, personal_data_manager->GetProfiles().size());
+  autofill::AutofillProfile* profile = personal_data_manager->GetProfiles()[0];
+  DCHECK(profile);
+  EXPECT_EQ(base::ASCIIToUTF16("any state"),
+            profile->GetRawInfo(autofill::ADDRESS_HOME_STATE));
+}
+
+}  // namespace payments