[Payments] Fix bug in shipping option selection.

The code was not handling a shipping option going away very well.

BUG=707240
TEST=browser_tests

Review-Url: https://codereview.chromium.org/2808883002
Cr-Commit-Position: refs/heads/master@{#463306}

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

@@ -51,7 +51,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestOrderSummaryViewControllerTest,
 
   // Go to the shipping address screen and select the first address (MI state).
   ClickOnBackArrow();
-  OpenShippingSectionScreen();
+  OpenShippingAddressSectionScreen();
   ResetEventObserverForSequence(std::list<DialogEvent>{
       DialogEvent::BACK_NAVIGATION, DialogEvent::SPEC_DONE_UPDATING});
   ClickOnChildInListViewAndWait(
@@ -59,8 +59,8 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestOrderSummaryViewControllerTest,
       DialogViewID::SHIPPING_ADDRESS_SHEET_LIST_VIEW);
 
   // Michigan address is selected and has standard shipping.
-  std::vector<base::string16> shipping_address_labels =
-      GetThreeLineLabelValues(DialogViewID::PAYMENT_SHEET_SHIPPING_SECTION);
+  std::vector<base::string16> shipping_address_labels = GetThreeLineLabelValues(
+      DialogViewID::PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION);
   EXPECT_EQ(base::ASCIIToUTF16("Jane A. Smith"), shipping_address_labels[0]);
   EXPECT_EQ(
       base::ASCIIToUTF16("ACME, 123 Main Street, Unit 1, Greensdale, MI 48838"),
@@ -87,7 +87,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestOrderSummaryViewControllerTest,
 
   // Go to the shipping address screen and select the second address (CA state).
   ClickOnBackArrow();
-  OpenShippingSectionScreen();
+  OpenShippingAddressSectionScreen();
   ResetEventObserverForSequence(std::list<DialogEvent>{
       DialogEvent::BACK_NAVIGATION, DialogEvent::SPEC_DONE_UPDATING});
   ClickOnChildInListViewAndWait(
@@ -95,8 +95,8 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestOrderSummaryViewControllerTest,
       DialogViewID::SHIPPING_ADDRESS_SHEET_LIST_VIEW);
 
   // California address is selected and has free shipping.
-  shipping_address_labels =
-      GetThreeLineLabelValues(DialogViewID::PAYMENT_SHEET_SHIPPING_SECTION);
+  shipping_address_labels = GetThreeLineLabelValues(
+      DialogViewID::PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION);
   EXPECT_EQ(base::ASCIIToUTF16("John H. Doe"), shipping_address_labels[0]);
   EXPECT_EQ(base::ASCIIToUTF16(
                 "Underworld, 666 Erebus St., Apt 8, Elysium, CA 91111"),

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

@@ -111,9 +111,14 @@ void PaymentRequestBrowserTestBase::OnPaymentMethodOpened() {
     event_observer_->Observe(DialogEvent::PAYMENT_METHOD_OPENED);
 }
 
-void PaymentRequestBrowserTestBase::OnShippingSectionOpened() {
+void PaymentRequestBrowserTestBase::OnShippingAddressSectionOpened() {
   if (event_observer_)
-    event_observer_->Observe(DialogEvent::SHIPPING_SECTION_OPENED);
+    event_observer_->Observe(DialogEvent::SHIPPING_ADDRESS_SECTION_OPENED);
+}
+
+void PaymentRequestBrowserTestBase::OnShippingOptionSectionOpened() {
+  if (event_observer_)
+    event_observer_->Observe(DialogEvent::SHIPPING_OPTION_SECTION_OPENED);
 }
 
 void PaymentRequestBrowserTestBase::OnCreditCardEditorOpened() {
@@ -206,12 +211,18 @@ void PaymentRequestBrowserTestBase::OpenPaymentMethodScreen() {
   ClickOnDialogViewAndWait(DialogViewID::PAYMENT_SHEET_PAYMENT_METHOD_SECTION);
 }
 
-void PaymentRequestBrowserTestBase::OpenShippingSectionScreen() {
-  ResetEventObserver(DialogEvent::SHIPPING_SECTION_OPENED);
+void PaymentRequestBrowserTestBase::OpenShippingAddressSectionScreen() {
+  ResetEventObserver(DialogEvent::SHIPPING_ADDRESS_SECTION_OPENED);
 
-  ClickOnDialogViewAndWait(DialogViewID::PAYMENT_SHEET_SHIPPING_SECTION);
+  ClickOnDialogViewAndWait(
+      DialogViewID::PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION);
 }
 
+void PaymentRequestBrowserTestBase::OpenShippingOptionSectionScreen() {
+  ResetEventObserver(DialogEvent::SHIPPING_OPTION_SECTION_OPENED);
+
+  ClickOnDialogViewAndWait(DialogViewID::PAYMENT_SHEET_SHIPPING_OPTION_SECTION);
+}
 void PaymentRequestBrowserTestBase::OpenCreditCardEditorScreen() {
   ResetEventObserver(DialogEvent::CREDIT_CARD_EDITOR_OPENED);
 

chrome/browser/ui/views/payments/payment_request_browsertest_base.h

@@ -80,7 +80,8 @@ class PaymentRequestBrowserTestBase
   void OnDialogOpened() override;
   void OnOrderSummaryOpened() override;
   void OnPaymentMethodOpened() override;
-  void OnShippingSectionOpened() override;
+  void OnShippingAddressSectionOpened() override;
+  void OnShippingOptionSectionOpened() override;
   void OnCreditCardEditorOpened() override;
   void OnShippingAddressEditorOpened() override;
   void OnBackNavigation() override;
@@ -105,7 +106,8 @@ class PaymentRequestBrowserTestBase
   // associated action to happen.
   void OpenOrderSummaryScreen();
   void OpenPaymentMethodScreen();
-  void OpenShippingSectionScreen();
+  void OpenShippingAddressSectionScreen();
+  void OpenShippingOptionSectionScreen();
   void OpenCreditCardEditorScreen();
   void OpenShippingAddressEditorScreen();
   void ClickOnBackArrow();
@@ -182,7 +184,8 @@ class PaymentRequestBrowserTestBase
     DIALOG_CLOSED,
     ORDER_SUMMARY_OPENED,
     PAYMENT_METHOD_OPENED,
-    SHIPPING_SECTION_OPENED,
+    SHIPPING_ADDRESS_SECTION_OPENED,
+    SHIPPING_OPTION_SECTION_OPENED,
     CREDIT_CARD_EDITOR_OPENED,
     SHIPPING_ADDRESS_EDITOR_OPENED,
     BACK_NAVIGATION,

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

@@ -184,7 +184,7 @@ void PaymentRequestDialogView::ShowShippingProfileSheet() {
           &controller_map_),
       /* animate = */ true);
   if (observer_for_testing_)
-    observer_for_testing_->OnShippingSectionOpened();
+    observer_for_testing_->OnShippingAddressSectionOpened();
 }
 
 void PaymentRequestDialogView::ShowShippingOptionSheet() {
@@ -193,6 +193,8 @@ void PaymentRequestDialogView::ShowShippingOptionSheet() {
                            request_->spec(), request_->state(), this),
                        &controller_map_),
                    /* animate = */ true);
+  if (observer_for_testing_)
+    observer_for_testing_->OnShippingOptionSectionOpened();
 }
 
 void PaymentRequestDialogView::ShowCvcUnmaskPrompt(

chrome/browser/ui/views/payments/payment_request_dialog_view.h

@@ -43,7 +43,9 @@ class PaymentRequestDialogView : public views::DialogDelegateView,
 
     virtual void OnPaymentMethodOpened() = 0;
 
-    virtual void OnShippingSectionOpened() = 0;
+    virtual void OnShippingAddressSectionOpened() = 0;
+
+    virtual void OnShippingOptionSectionOpened() = 0;
 
     virtual void OnCreditCardEditorOpened() = 0;
 

chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h

@@ -17,9 +17,9 @@ enum class DialogViewID : int {
 
   // The following are views::Button (clickable).
   PAYMENT_SHEET_CONTACT_INFO_SECTION,
-  PAYMENT_SHEET_PAYMENT_METHOD_SECTION,
-  PAYMENT_SHEET_SHIPPING_SECTION,
   PAYMENT_SHEET_SUMMARY_SECTION,
+  PAYMENT_SHEET_PAYMENT_METHOD_SECTION,
+  PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION,
   PAYMENT_SHEET_SHIPPING_OPTION_SECTION,
   PAYMENT_METHOD_ADD_CARD_BUTTON,
   PAYMENT_METHOD_ADD_SHIPPING_BUTTON,

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

@@ -417,7 +417,7 @@ std::unique_ptr<views::Button> PaymentSheetViewController::CreateShippingRow() {
   section->set_tag(
       static_cast<int>(PaymentSheetViewControllerTags::SHOW_SHIPPING_BUTTON));
   section->set_id(
-      static_cast<int>(DialogViewID::PAYMENT_SHEET_SHIPPING_SECTION));
+      static_cast<int>(DialogViewID::PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION));
   return section;
 }
 

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

@@ -62,8 +62,11 @@ IN_PROC_BROWSER_TEST_F(PaymentSheetViewControllerNoShippingTest,
                          DialogViewID::PAYMENT_SHEET_SUMMARY_SECTION)));
   EXPECT_NE(nullptr, dialog_view()->GetViewByID(static_cast<int>(
                          DialogViewID::PAYMENT_SHEET_PAYMENT_METHOD_SECTION)));
+  EXPECT_EQ(nullptr,
+            dialog_view()->GetViewByID(static_cast<int>(
+                DialogViewID::PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION)));
   EXPECT_EQ(nullptr, dialog_view()->GetViewByID(static_cast<int>(
-                         DialogViewID::PAYMENT_SHEET_SHIPPING_SECTION)));
+                         DialogViewID::PAYMENT_SHEET_SHIPPING_OPTION_SECTION)));
   EXPECT_EQ(nullptr, dialog_view()->GetViewByID(static_cast<int>(
                          DialogViewID::PAYMENT_SHEET_CONTACT_INFO_SECTION)));
 }
@@ -145,8 +148,11 @@ IN_PROC_BROWSER_TEST_F(PaymentSheetViewControllerContactDetailsTest,
                          DialogViewID::PAYMENT_SHEET_SUMMARY_SECTION)));
   EXPECT_NE(nullptr, dialog_view()->GetViewByID(static_cast<int>(
                          DialogViewID::PAYMENT_SHEET_PAYMENT_METHOD_SECTION)));
+  EXPECT_NE(nullptr,
+            dialog_view()->GetViewByID(static_cast<int>(
+                DialogViewID::PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION)));
   EXPECT_NE(nullptr, dialog_view()->GetViewByID(static_cast<int>(
-                         DialogViewID::PAYMENT_SHEET_SHIPPING_SECTION)));
+                         DialogViewID::PAYMENT_SHEET_SHIPPING_OPTION_SECTION)));
   EXPECT_NE(nullptr, dialog_view()->GetViewByID(static_cast<int>(
                          DialogViewID::PAYMENT_SHEET_CONTACT_INFO_SECTION)));
 }

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

@@ -75,6 +75,10 @@ ShippingOptionViewController::ShippingOptionViewController(
 
 ShippingOptionViewController::~ShippingOptionViewController() {}
 
+void ShippingOptionViewController::OnSpecUpdated() {
+  UpdateContentView();
+}
+
 base::string16 ShippingOptionViewController::GetSheetTitle() {
   return GetShippingOptionSectionString(spec()->shipping_type());
 }

chrome/browser/ui/views/payments/shipping_option_view_controller.h

@@ -8,19 +8,24 @@
 #include "base/macros.h"
 #include "chrome/browser/ui/views/payments/payment_request_item_list.h"
 #include "chrome/browser/ui/views/payments/payment_request_sheet_controller.h"
+#include "components/payments/content/payment_request_spec.h"
 
 namespace payments {
 
-class PaymentRequestSpec;
 class PaymentRequestState;
 
-class ShippingOptionViewController : public PaymentRequestSheetController {
+class ShippingOptionViewController : public PaymentRequestSheetController,
+                                     public PaymentRequestSpec::Observer {
  public:
   ShippingOptionViewController(PaymentRequestSpec* spec,
                                PaymentRequestState* state,
                                PaymentRequestDialogView* dialog);
   ~ShippingOptionViewController() override;
 
+  // PaymentRequestSpec::Observer:
+  void OnInvalidSpecProvided() override {}
+  void OnSpecUpdated() override;
+
  private:
   // PaymentRequestSheetController:
   base::string16 GetSheetTitle() override;

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

+// 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 <list>
+
+#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 "components/autofill/core/browser/autofill_profile.h"
+#include "components/autofill/core/browser/autofill_test_utils.h"
+#include "components/autofill/core/browser/field_types.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace payments {
+
+class PaymentRequestShippingOptionViewControllerTest
+    : public PaymentRequestBrowserTestBase {
+ protected:
+  PaymentRequestShippingOptionViewControllerTest()
+      : PaymentRequestBrowserTestBase(
+            "/payment_request_dynamic_shipping_test.html") {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PaymentRequestShippingOptionViewControllerTest);
+};
+
+IN_PROC_BROWSER_TEST_F(PaymentRequestShippingOptionViewControllerTest,
+                       OrderSummaryReflectsShippingOption) {
+  // In MI state, shipping is $5.00.
+  autofill::AutofillProfile michigan = autofill::test::GetFullProfile2();
+  michigan.set_use_count(100U);
+  AddAutofillProfile(michigan);
+  // A Canadian address will have no shipping options.
+  autofill::AutofillProfile canada = autofill::test::GetFullProfile();
+  canada.SetRawInfo(autofill::ADDRESS_HOME_COUNTRY, base::ASCIIToUTF16("CA"));
+  canada.set_use_count(50U);
+  AddAutofillProfile(canada);
+
+  InvokePaymentRequestUI();
+
+  // There is no shipping option section, because no address has been selected.
+  EXPECT_EQ(nullptr, dialog_view()->GetViewByID(static_cast<int>(
+                         DialogViewID::PAYMENT_SHEET_SHIPPING_OPTION_SECTION)));
+
+  // Go to the shipping address screen and select the first address (MI state).
+  OpenShippingAddressSectionScreen();
+  ResetEventObserverForSequence(std::list<DialogEvent>{
+      DialogEvent::BACK_NAVIGATION, DialogEvent::SPEC_DONE_UPDATING});
+  ClickOnChildInListViewAndWait(
+      /* child_index=*/0, /*total_num_children=*/2,
+      DialogViewID::SHIPPING_ADDRESS_SHEET_LIST_VIEW);
+
+  // Michigan address is selected and has standard shipping.
+  std::vector<base::string16> shipping_address_labels = GetThreeLineLabelValues(
+      DialogViewID::PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION);
+  EXPECT_EQ(base::ASCIIToUTF16("Jane A. Smith"), shipping_address_labels[0]);
+  EXPECT_EQ(
+      base::ASCIIToUTF16("ACME, 123 Main Street, Unit 1, Greensdale, MI 48838"),
+      shipping_address_labels[1]);
+  EXPECT_EQ(base::ASCIIToUTF16("13105557889"), shipping_address_labels[2]);
+
+  // The shipping option section exists, and the shipping option is shown.
+  std::vector<base::string16> shipping_option_labels =
+      GetShippingOptionLabelValues(
+          DialogViewID::PAYMENT_SHEET_SHIPPING_OPTION_SECTION);
+  EXPECT_EQ(base::ASCIIToUTF16("Standard shipping in US"),
+            shipping_option_labels[0]);
+  EXPECT_EQ(base::ASCIIToUTF16("$5.00"), shipping_option_labels[1]);
+
+  // Go to the shipping address screen and select the second address (Canada).
+  OpenShippingAddressSectionScreen();
+  ResetEventObserverForSequence(std::list<DialogEvent>{
+      DialogEvent::BACK_NAVIGATION, DialogEvent::SPEC_DONE_UPDATING});
+  ClickOnChildInListViewAndWait(
+      /* child_index=*/1, /*total_num_children=*/2,
+      DialogViewID::SHIPPING_ADDRESS_SHEET_LIST_VIEW);
+
+  // There is no longer shipping option section, because no shipping options are
+  // available for Canada.
+  EXPECT_EQ(nullptr, dialog_view()->GetViewByID(static_cast<int>(
+                         DialogViewID::PAYMENT_SHEET_SHIPPING_OPTION_SECTION)));
+}
+
+}  // namespace payments

chrome/test/BUILD.gn

@@ -2156,6 +2156,7 @@ test("browser_tests") {
         "../browser/ui/views/payments/payment_request_can_make_payment_browsertest.cc",
         "../browser/ui/views/payments/payment_request_payment_response_browsertest.cc",
         "../browser/ui/views/payments/payment_sheet_view_controller_browsertest.cc",
+        "../browser/ui/views/payments/shipping_option_view_controller_browsertest.cc",
         "../browser/ui/views/select_file_dialog_extension_browsertest.cc",
         "../browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc",
       ]

components/payments/content/payment_request_spec.cc

@@ -178,6 +178,10 @@ void PaymentRequestSpec::UpdateSelectedShippingOption() {
       });
   if (selected_shipping_option_it != details().shipping_options.rend()) {
     selected_shipping_option_ = selected_shipping_option_it->get();
+  } else {
+    // It's possible that there is no selected shipping option.
+    // TODO(crbug.com/710004): Show an error in this case.
+    selected_shipping_option_ = nullptr;
   }
 }