PaymentRequest: Add a use counter for updateWith() w/o shippingOptions

This patch adds a use counter to number of calling updateWith() without
shippingOptions dictionary member on ShippingAddressChange and
ShippingOptionChange events.

Bug: 902291
Change-Id: Iac779040dc1a7245dcfe4db795e09d4ecbca9b90
Reviewed-on: https://chromium-review.googlesource.com/c/1319653
Commit-Queue: Jinho Bang <jinho.bang@samsung.com>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606376}

chrome/browser/page_load_metrics/observers/use_counter/ukm_features.cc

@@ -62,6 +62,8 @@ bool IsAllowedUkmFeature(blink::mojom::WebFeature feature) {
           WebFeature::kCursorImageGT32x32, WebFeature::kCursorImageLE32x32,
           WebFeature::kHistoryPushState, WebFeature::kHistoryReplaceState,
           WebFeature::kCursorImageGT64x64, WebFeature::kAdClick,
+          WebFeature::kUpdateWithoutShippingOptionOnShippingAddressChange,
+          WebFeature::kUpdateWithoutShippingOptionOnShippingOptionChange,
       }));
   return opt_in_features->count(feature);
 }

third_party/blink/public/platform/web_feature.mojom

@@ -2071,6 +2071,8 @@ enum WebFeature {
   kGetDisplayMedia = 2619,
   kCursorImageGT64x64 = 2620,
   kAdClick = 2621,
+  kUpdateWithoutShippingOptionOnShippingAddressChange = 2622,
+  kUpdateWithoutShippingOptionOnShippingOptionChange = 2623,
   // Add new features immediately above this line. Don't change assigned
   // numbers of any item, and don't reuse removed slots.
   // Also, run update_use_counter_feature_enum.py in

third_party/blink/renderer/modules/payments/payment_request.cc

@@ -980,6 +980,7 @@ ScriptPromise PaymentRequest::Complete(ScriptState* script_state,
 }
 
 void PaymentRequest::OnUpdatePaymentDetails(
+    const AtomicString& event_type,
     const ScriptValue& details_script_value) {
   if (!GetPendingAcceptPromiseResolver() || !payment_provider_)
     return;
@@ -1016,6 +1017,20 @@ void PaymentRequest::OnUpdatePaymentDetails(
     return;
   }
 
+  // TODO(https://crbug.com/902291): We should make shippingOptions optional.
+  if (options_->requestShipping() && !details->hasShippingOptions()) {
+    if (event_type == event_type_names::kShippingaddresschange) {
+      UseCounter::Count(
+          GetExecutionContext(),
+          WebFeature::kUpdateWithoutShippingOptionOnShippingAddressChange);
+    }
+    if (event_type == event_type_names::kShippingoptionchange) {
+      UseCounter::Count(
+          GetExecutionContext(),
+          WebFeature::kUpdateWithoutShippingOptionOnShippingOptionChange);
+    }
+  }
+
   if (!options_->requestShipping())
     validated_details->shipping_options.clear();
 

third_party/blink/renderer/modules/payments/payment_request.h

@@ -88,7 +88,8 @@ class MODULES_EXPORT PaymentRequest final
   ScriptPromise Retry(ScriptState*, const PaymentValidationErrors*) override;
 
   // PaymentUpdater:
-  void OnUpdatePaymentDetails(const ScriptValue& details_script_value) override;
+  void OnUpdatePaymentDetails(const AtomicString& event_type,
+                              const ScriptValue& details_script_value) override;
   void OnUpdatePaymentDetailsFailure(const String& error) override;
 
   void Trace(blink::Visitor*) override;

third_party/blink/renderer/modules/payments/payment_request_test.cc

@@ -7,6 +7,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_testing.h"
 #include "third_party/blink/renderer/core/dom/document.h"
+#include "third_party/blink/renderer/core/event_type_names.h"
 #include "third_party/blink/renderer/modules/payments/payment_test_helper.h"
 #include "third_party/blink/renderer/platform/heap/heap_allocator.h"
 
@@ -389,6 +390,7 @@ TEST(PaymentRequestTest, IgnoreUpdatePaymentDetailsAfterShowPromiseResolved) {
       ->OnPaymentResponse(BuildPaymentResponseForTest());
 
   request->OnUpdatePaymentDetails(
+      event_type_names::kShippingaddresschange,
       ScriptValue::From(scope.GetScriptState(), "foo"));
 }
 
@@ -405,6 +407,7 @@ TEST(PaymentRequestTest, RejectShowPromiseOnNonPaymentDetailsUpdate) {
       .Then(funcs.ExpectNoCall(), funcs.ExpectCall());
 
   request->OnUpdatePaymentDetails(
+      event_type_names::kShippingaddresschange,
       ScriptValue::From(scope.GetScriptState(), "NotPaymentDetails"));
 }
 
@@ -420,11 +423,13 @@ TEST(PaymentRequestTest, RejectShowPromiseOnInvalidPaymentDetailsUpdate) {
   request->show(scope.GetScriptState())
       .Then(funcs.ExpectNoCall(), funcs.ExpectCall());
 
-  request->OnUpdatePaymentDetails(ScriptValue::From(
-      scope.GetScriptState(),
-      FromJSONString(scope.GetScriptState()->GetIsolate(),
-                     scope.GetScriptState()->GetContext(), "{\"total\": {}}",
-                     scope.GetExceptionState())));
+  request->OnUpdatePaymentDetails(
+      event_type_names::kShippingaddresschange,
+      ScriptValue::From(
+          scope.GetScriptState(),
+          FromJSONString(scope.GetScriptState()->GetIsolate(),
+                         scope.GetScriptState()->GetContext(),
+                         "{\"total\": {}}", scope.GetExceptionState())));
   EXPECT_FALSE(scope.GetExceptionState().HadException());
 }
 
@@ -450,11 +455,13 @@ TEST(PaymentRequestTest,
       "\"shippingOptions\": [{\"id\": \"standardShippingOption\", \"label\": "
       "\"Standard shipping\", \"amount\": {\"currency\": \"USD\", \"value\": "
       "\"5.00\"}, \"selected\": true}]}";
-  request->OnUpdatePaymentDetails(ScriptValue::From(
-      scope.GetScriptState(),
-      FromJSONString(scope.GetScriptState()->GetIsolate(),
-                     scope.GetScriptState()->GetContext(),
-                     detail_with_shipping_options, scope.GetExceptionState())));
+  request->OnUpdatePaymentDetails(
+      event_type_names::kShippingaddresschange,
+      ScriptValue::From(scope.GetScriptState(),
+                        FromJSONString(scope.GetScriptState()->GetIsolate(),
+                                       scope.GetScriptState()->GetContext(),
+                                       detail_with_shipping_options,
+                                       scope.GetExceptionState())));
   EXPECT_FALSE(scope.GetExceptionState().HadException());
   EXPECT_EQ("standardShippingOption", request->shippingOption());
   String detail_without_shipping_options =
@@ -462,6 +469,7 @@ TEST(PaymentRequestTest,
       "\"value\": \"5.00\"}}}";
 
   request->OnUpdatePaymentDetails(
+      event_type_names::kShippingaddresschange,
       ScriptValue::From(scope.GetScriptState(),
                         FromJSONString(scope.GetScriptState()->GetIsolate(),
                                        scope.GetScriptState()->GetContext(),
@@ -495,6 +503,7 @@ TEST(
       "\"USD\", \"value\": \"50.00\"}}]}";
 
   request->OnUpdatePaymentDetails(
+      event_type_names::kShippingaddresschange,
       ScriptValue::From(scope.GetScriptState(),
                         FromJSONString(scope.GetScriptState()->GetIsolate(),
                                        scope.GetScriptState()->GetContext(),
@@ -525,6 +534,7 @@ TEST(PaymentRequestTest, UseTheSelectedShippingOptionFromPaymentDetailsUpdate) {
       "\"USD\", \"value\": \"50.00\"}, \"selected\": true}]}";
 
   request->OnUpdatePaymentDetails(
+      event_type_names::kShippingaddresschange,
       ScriptValue::From(scope.GetScriptState(),
                         FromJSONString(scope.GetScriptState()->GetIsolate(),
                                        scope.GetScriptState()->GetContext(),
@@ -550,11 +560,13 @@ TEST(PaymentRequestTest, NoExceptionWithErrorMessageInUpdate) {
       "\"value\": \"5.00\"}},"
       "\"error\": \"This is an error message.\"}";
 
-  request->OnUpdatePaymentDetails(ScriptValue::From(
-      scope.GetScriptState(),
-      FromJSONString(scope.GetScriptState()->GetIsolate(),
-                     scope.GetScriptState()->GetContext(),
-                     detail_with_error_msg, scope.GetExceptionState())));
+  request->OnUpdatePaymentDetails(
+      event_type_names::kShippingaddresschange,
+      ScriptValue::From(
+          scope.GetScriptState(),
+          FromJSONString(scope.GetScriptState()->GetIsolate(),
+                         scope.GetScriptState()->GetContext(),
+                         detail_with_error_msg, scope.GetExceptionState())));
   EXPECT_FALSE(scope.GetExceptionState().HadException());
 }
 

third_party/blink/renderer/modules/payments/payment_request_update_event.cc

@@ -23,31 +23,32 @@ constexpr TimeDelta kAbortTimeout = TimeDelta::FromSeconds(60);
 
 class UpdatePaymentDetailsFunction : public ScriptFunction {
  public:
-  static v8::Local<v8::Function> CreateFunction(ScriptState* script_state,
-                                                PaymentUpdater* updater) {
+  static v8::Local<v8::Function> CreateFunction(
+      ScriptState* script_state,
+      PaymentRequestUpdateEvent* update_event) {
     UpdatePaymentDetailsFunction* self =
-        new UpdatePaymentDetailsFunction(script_state, updater);
+        new UpdatePaymentDetailsFunction(script_state, update_event);
     return self->BindToV8Function();
   }
 
   void Trace(blink::Visitor* visitor) override {
-    visitor->Trace(updater_);
+    visitor->Trace(update_event_);
     ScriptFunction::Trace(visitor);
   }
 
  private:
   UpdatePaymentDetailsFunction(ScriptState* script_state,
-                               PaymentUpdater* updater)
-      : ScriptFunction(script_state), updater_(updater) {
-    DCHECK(updater_);
+                               PaymentRequestUpdateEvent* update_event)
+      : ScriptFunction(script_state), update_event_(update_event) {
+    DCHECK(update_event_);
   }
 
   ScriptValue Call(ScriptValue value) override {
-    updater_->OnUpdatePaymentDetails(value);
+    update_event_->OnUpdatePaymentDetails(update_event_->type(), value);
     return ScriptValue();
   }
 
-  Member<PaymentUpdater> updater_;
+  Member<PaymentRequestUpdateEvent> update_event_;
 };
 
 class UpdatePaymentDetailsErrorFunction : public ScriptFunction {
@@ -130,11 +131,12 @@ void PaymentRequestUpdateEvent::updateWith(ScriptState* script_state,
 }
 
 void PaymentRequestUpdateEvent::OnUpdatePaymentDetails(
+    const AtomicString& event_type,
     const ScriptValue& details_script_value) {
   if (!updater_)
     return;
   abort_timer_.Stop();
-  updater_->OnUpdatePaymentDetails(details_script_value);
+  updater_->OnUpdatePaymentDetails(event_type, details_script_value);
   updater_ = nullptr;
 }
 

third_party/blink/renderer/modules/payments/payment_request_update_event.h

@@ -41,7 +41,8 @@ class MODULES_EXPORT PaymentRequestUpdateEvent : public Event,
   bool is_waiting_for_update() const { return wait_for_update_; }
 
   // PaymentUpdater:
-  void OnUpdatePaymentDetails(const ScriptValue& details_script_value) override;
+  void OnUpdatePaymentDetails(const AtomicString& event_type,
+                              const ScriptValue& details_script_value) override;
   void OnUpdatePaymentDetailsFailure(const String& error) override;
 
   void Trace(blink::Visitor*) override;

third_party/blink/renderer/modules/payments/payment_request_update_event_test.cc

@@ -28,8 +28,9 @@ class MockPaymentUpdater : public GarbageCollectedFinalized<MockPaymentUpdater>,
   MockPaymentUpdater() = default;
   ~MockPaymentUpdater() override = default;
 
-  MOCK_METHOD1(OnUpdatePaymentDetails,
-               void(const ScriptValue& detailsScriptValue));
+  MOCK_METHOD2(OnUpdatePaymentDetails,
+               void(const AtomicString& event_type,
+                    const ScriptValue& detailsScriptValue));
   MOCK_METHOD1(OnUpdatePaymentDetailsFailure, void(const String& error));
 
   void Trace(blink::Visitor* visitor) override {}
@@ -49,7 +50,9 @@ TEST(PaymentRequestUpdateEventTest, OnUpdatePaymentDetailsCalled) {
                     scope.GetExceptionState());
   EXPECT_FALSE(scope.GetExceptionState().HadException());
 
-  EXPECT_CALL(*updater, OnUpdatePaymentDetails(testing::_));
+  EXPECT_CALL(*updater,
+              OnUpdatePaymentDetails(event_type_names::kShippingaddresschange,
+                                     testing::_));
   EXPECT_CALL(*updater, OnUpdatePaymentDetailsFailure(testing::_)).Times(0);
 
   payment_details->Resolve("foo");
@@ -69,7 +72,10 @@ TEST(PaymentRequestUpdateEventTest, OnUpdatePaymentDetailsFailureCalled) {
                     scope.GetExceptionState());
   EXPECT_FALSE(scope.GetExceptionState().HadException());
 
-  EXPECT_CALL(*updater, OnUpdatePaymentDetails(testing::_)).Times(0);
+  EXPECT_CALL(*updater,
+              OnUpdatePaymentDetails(event_type_names::kShippingaddresschange,
+                                     testing::_))
+      .Times(0);
   EXPECT_CALL(*updater, OnUpdatePaymentDetailsFailure(testing::_));
 
   payment_details->Reject("oops");

third_party/blink/renderer/modules/payments/payment_updater.h

@@ -16,6 +16,7 @@ class ScriptValue;
 class MODULES_EXPORT PaymentUpdater : public GarbageCollectedMixin {
  public:
   virtual void OnUpdatePaymentDetails(
+      const AtomicString& event_type,
       const ScriptValue& details_script_value) = 0;
   virtual void OnUpdatePaymentDetailsFailure(const String& error) = 0;
 

tools/metrics/histograms/enums.xml

@@ -20678,6 +20678,8 @@ Called by update_net_error_codes.py.-->
   <int value="2619" label="GetDisplayMedia"/>
   <int value="2620" label="CursorImageGT64x64"/>
   <int value="2621" label="AdClick"/>
+  <int value="2622" label="UpdateWithoutShippingOptionOnShippingAddressChange"/>
+  <int value="2623" label="UpdateWithoutShippingOptionOnShippingOptionChange"/>
 </enum>
 
 <enum name="FeaturePolicyFeature">