Putting error message when skipping sw-apps for partial delegation

Change:
* Devtool Warning. Before - when enforcing full delegation, those
sw-apps that are skipped for partial delegation are skipped silently.
After - under the same situation, the apps are skipped with a warning
message in devtool console.
* Show Rejection. Before - when enforcing full delegation, if all
sw-apps are skipped, no error would be given to the developer.
After - under the same condition, the developer would receive an error
in the form of a show() promise rejection. Note that if there has
already been an error, do not override the error.
* Test Addition. Add a test for the multiple-app scenario.
* Test Fix.

Bug: 1100656, 1103695

Change-Id: Ic6c8a1e222069a0eadbbcad7de2d358489dec8ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2316799
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Sahel Sharify <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795112}

chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinder.java

@@ -665,7 +665,7 @@ public class AndroidPaymentAppFinder implements ManifestVerifyCallback {
                 || methodName.equals(MethodStrings.GOOGLE_PLAY_BILLING)) {
             if (!appSupportedDelegations.providesAll(
                         mFactoryDelegate.getParams().getPaymentOptions())) {
-                Log.e(TAG, ErrorStrings.SKIP_APP_FOR_PARTIAL_DELEGATION.replace("$1", packageName));
+                Log.e(TAG, ErrorStrings.SKIP_APP_FOR_PARTIAL_DELEGATION.replace("$", packageName));
                 return;
             }
         }

chrome/browser/payments/payment_handler_enforce_full_delegation_browsertest.cc

@@ -12,8 +12,8 @@ namespace payments {
 namespace {
 
 enum EnforceFullDelegationFlag {
+  DISABLED = 0,
   ENABLED,
-  DISABLED,
 };
 
 class PaymentHandlerEnforceFullDelegationTest
@@ -33,7 +33,6 @@ class PaymentHandlerEnforceFullDelegationTest
 
   void SetUpOnMainThread() override {
     PaymentRequestPlatformBrowserTestBase::SetUpOnMainThread();
-    NavigateTo("/enforce_full_delegation.com/index.html");
   }
 
  private:
@@ -41,7 +40,56 @@ class PaymentHandlerEnforceFullDelegationTest
 };
 
 IN_PROC_BROWSER_TEST_P(PaymentHandlerEnforceFullDelegationTest,
-                       ShowPaymentSheetWhenEnabledRejectWhenDisabled) {
+                       ShowPaymentSheetWhenOnlySomeAppsAreSkipped) {
+  std::string expected = "success";
+
+  std::string method_name1 =
+      https_server()->GetURL("a.com", "/method_manifest.json").spec();
+  NavigateTo("a.com", "/enforce_full_delegation.com/index.html");
+  EXPECT_EQ(expected,
+            content::EvalJs(GetActiveWebContents(),
+                            content::JsReplace("install($1)", method_name1)));
+  EXPECT_EQ(expected, content::EvalJs(GetActiveWebContents(),
+                                      "enableDelegations(['payerName'])"));
+
+  std::string method_name2 =
+      https_server()->GetURL("b.com", "/method_manifest.json").spec();
+  NavigateTo("b.com", "/enforce_full_delegation.com/index.html");
+  EXPECT_EQ(expected,
+            content::EvalJs(GetActiveWebContents(),
+                            content::JsReplace("install($1)", method_name2)));
+  EXPECT_EQ(expected,
+            content::EvalJs(GetActiveWebContents(), "enableDelegations([])"));
+
+  EXPECT_EQ(expected,
+            content::EvalJs(GetActiveWebContents(),
+                            content::JsReplace("addSupportedMethods([$1, $2])",
+                                               method_name1, method_name2)));
+  EXPECT_EQ(expected,
+            content::EvalJs(
+                GetActiveWebContents(),
+                "createPaymentRequestWithOptions({requestPayerName: true})"));
+
+  // When enforcing full delegation: although b.com app is skipped for partial
+  // delegation, a.com app is still expected to appear in the payment sheet.
+  // When not enforcing: both apps are expected to appear in the sheet. So the
+  // sheet appears in both enabled and disabled cases.
+  ResetEventWaiterForSingleEvent(TestEvent::kAppListReady);
+
+  EXPECT_EQ(expected, content::EvalJs(GetActiveWebContents(), "show()"));
+  WaitForObservedEvent();
+
+  if (GetParam() == ENABLED) {
+    EXPECT_EQ(1u, test_controller()->app_descriptions().size());
+  } else {
+    EXPECT_EQ(2u, test_controller()->app_descriptions().size());
+  }
+}
+
+IN_PROC_BROWSER_TEST_P(PaymentHandlerEnforceFullDelegationTest,
+                       WhenEnabled_ShowPaymentSheet_WhenDisabled_Reject) {
+  NavigateTo("/enforce_full_delegation.com/index.html");
+
   std::string expected = "success";
   EXPECT_EQ(expected, content::EvalJs(GetActiveWebContents(), "install()"));
   EXPECT_EQ(expected, content::EvalJs(GetActiveWebContents(),
@@ -63,7 +111,12 @@ IN_PROC_BROWSER_TEST_P(PaymentHandlerEnforceFullDelegationTest,
   WaitForObservedEvent();
 
   if (GetParam() == ENABLED) {
-    EXPECT_GE(1u, test_controller()->app_descriptions().size());
+    EXPECT_EQ(0u, test_controller()->app_descriptions().size());
+    ExpectBodyContains(
+        "Skipping \"MaxPay\" for not providing all of the requested "
+        "PaymentOptions.");
+  } else {
+    EXPECT_EQ(1u, test_controller()->app_descriptions().size());
   }
 }
 
@@ -71,6 +124,6 @@ IN_PROC_BROWSER_TEST_P(PaymentHandlerEnforceFullDelegationTest,
 // features::kEnforceFullDelegation.
 INSTANTIATE_TEST_SUITE_P(All,
                          PaymentHandlerEnforceFullDelegationTest,
-                         ::testing::Values(ENABLED, DISABLED));
+                         ::testing::Values(DISABLED, ENABLED));
 }  // namespace
 }  // namespace payments

components/payments/content/service_worker_payment_app_factory.cc

@@ -10,9 +10,11 @@
 #include "base/bind.h"
 #include "base/check_op.h"
 #include "base/memory/weak_ptr.h"
+#include "components/payments/content/developer_console_logger.h"
 #include "components/payments/content/payment_manifest_web_data_service.h"
 #include "components/payments/content/service_worker_payment_app.h"
 #include "components/payments/content/service_worker_payment_app_finder.h"
+#include "components/payments/core/error_message_util.h"
 #include "components/payments/core/features.h"
 #include "components/payments/core/method_strings.h"
 #include "content/public/browser/stored_payment_app.h"
@@ -38,7 +40,7 @@ class ServiceWorkerPaymentAppCreator {
   ServiceWorkerPaymentAppCreator(
       ServiceWorkerPaymentAppFactory* owner,
       base::WeakPtr<PaymentAppFactory::Delegate> delegate)
-      : owner_(owner), delegate_(delegate) {}
+      : owner_(owner), delegate_(delegate), log_(delegate->GetWebContents()) {}
 
   ~ServiceWorkerPaymentAppCreator() {}
 
@@ -56,7 +58,7 @@ class ServiceWorkerPaymentAppCreator {
 
     base::RepeatingClosure show_processing_spinner = base::BindRepeating(
         &PaymentAppFactory::Delegate::ShowProcessingSpinner, delegate_);
-
+    std::vector<std::string> skipped_app_names;
     for (auto& installed_app : apps) {
       std::vector<std::string> enabled_methods =
           installed_app.second->enabled_methods;
@@ -67,7 +69,7 @@ class ServiceWorkerPaymentAppCreator {
       if (ShouldSkipAppForPartialDelegation(
               installed_app.second->supported_delegations, delegate_,
               has_app_store_billing_method)) {
-        // TODO(crbug.com/1100656): give the developer an error message.
+        skipped_app_names.emplace_back(installed_app.second->name);
         continue;
       }
       auto app = std::make_unique<ServiceWorkerPaymentApp>(
@@ -89,7 +91,7 @@ class ServiceWorkerPaymentAppCreator {
       if (ShouldSkipAppForPartialDelegation(
               installable_app.second->supported_delegations, delegate_,
               is_app_store_billing_method)) {
-        // TODO(crbug.com/1100656): give the developer an error message.
+        skipped_app_names.emplace_back(installable_app.second->name);
         continue;
       }
       auto app = std::make_unique<ServiceWorkerPaymentApp>(
@@ -105,8 +107,20 @@ class ServiceWorkerPaymentAppCreator {
       number_of_pending_sw_payment_apps_++;
     }
 
-    if (number_of_pending_sw_payment_apps_ == 0U)
+    if (!skipped_app_names.empty()) {
+      std::string warning_message =
+          GetAppsSkippedForPartialDelegationErrorMessage(skipped_app_names);
+      log_.Warn(warning_message);
+    }
+
+    if (number_of_pending_sw_payment_apps_ == 0U) {
+      if (error_message.empty() && !skipped_app_names.empty()) {
+        std::string new_error_message =
+            GetAppsSkippedForPartialDelegationErrorMessage(skipped_app_names);
+        delegate_->OnPaymentAppCreationError(new_error_message);
+      }
       FinishAndCleanup();
+    }
   }
 
   bool ShouldSkipAppForPartialDelegation(
@@ -150,6 +164,7 @@ class ServiceWorkerPaymentAppCreator {
   ServiceWorkerPaymentAppFactory* owner_;
   base::WeakPtr<PaymentAppFactory::Delegate> delegate_;
   std::map<PaymentApp*, std::unique_ptr<PaymentApp>> available_apps_;
+  DeveloperConsoleLogger log_;
   int number_of_pending_sw_payment_apps_ = 0;
 
   base::WeakPtrFactory<ServiceWorkerPaymentAppCreator> weak_ptr_factory_{this};

components/payments/core/error_message_util.cc

@@ -9,27 +9,46 @@
 
 #include "base/check.h"
 #include "base/strings/string_util.h"
+#include "components/payments/core/error_strings.h"
 #include "components/payments/core/native_error_strings.h"
 
 namespace payments {
 
-std::string GetNotSupportedErrorMessage(const std::set<std::string>& methods) {
-  if (methods.empty())
-    return errors::kGenericPaymentMethodNotSupportedMessage;
+namespace {
 
-  std::vector<std::string> with_quotes(methods.size());
+template <class Collection>
+std::string concatNamesWithQuotesAndCommma(const Collection& names) {
+  std::vector<std::string> with_quotes(names.size());
   std::transform(
-      methods.begin(), methods.end(), with_quotes.begin(),
+      names.begin(), names.end(), with_quotes.begin(),
       [](const std::string& method_name) { return "\"" + method_name + "\""; });
+  std::string result = base::JoinString(with_quotes, ", ");
+  return result;
+}
+
+}  // namespace
+
+std::string GetNotSupportedErrorMessage(const std::set<std::string>& methods) {
+  if (methods.empty())
+    return errors::kGenericPaymentMethodNotSupportedMessage;
 
   std::string output;
   bool replaced = base::ReplaceChars(
-      with_quotes.size() == 1
-          ? errors::kSinglePaymentMethodNotSupportedFormat
-          : errors::kMultiplePaymentMethodsNotSupportedFormat,
-      "$", base::JoinString(with_quotes, ", "), &output);
+      methods.size() == 1 ? errors::kSinglePaymentMethodNotSupportedFormat
+                          : errors::kMultiplePaymentMethodsNotSupportedFormat,
+      "$", concatNamesWithQuotesAndCommma(methods), &output);
   DCHECK(replaced);
   return output;
 }
 
+std::string GetAppsSkippedForPartialDelegationErrorMessage(
+    const std::vector<std::string>& skipped_app_names) {
+  std::string output;
+  bool replaced = base::ReplaceChars(
+      errors::kSkipAppForPartialDelegation, "$",
+      concatNamesWithQuotesAndCommma(skipped_app_names), &output);
+
+  DCHECK(replaced);
+  return output;
+}
 }  // namespace payments

components/payments/core/error_message_util.h

@@ -14,6 +14,11 @@ namespace payments {
 // not supported.
 std::string GetNotSupportedErrorMessage(const std::set<std::string>& methods);
 
+// Returns a developer-facing error message that the apps are skipped because
+// they do not support full delegation.
+std::string GetAppsSkippedForPartialDelegationErrorMessage(
+    const std::vector<std::string>& skipped_apps);
+
 }  // namespace payments
 
 #endif  // COMPONENTS_PAYMENTS_CORE_ERROR_MESSAGE_UTIL_H_

components/payments/core/error_strings.cc

@@ -32,7 +32,7 @@ const char kProhibitedOriginOrInvalidSslExplanation[] = "No UI will be shown. Ca
 const char kShippingAddressInvalid[] = "Payment app returned invalid shipping address in response.";
 const char kShippingOptionEmpty[] = "Payment app returned invalid response. Missing field \"shipping option\".";
 const char kShippingOptionIdRequired[] = "Shipping option identifier required.";
-const char kSkipAppForPartialDelegation[] = "Skipping \"$1\" because it does not provide all of the requested PaymentOptions.";
+const char kSkipAppForPartialDelegation[] = "Skipping $ for not providing all of the requested PaymentOptions.";
 const char kStrictBasicCardShowReject[] = "User does not have valid information on file.";
 const char kTotalRequired[] = "Total required.";
 const char kUserCancelled[] = "User closed the Payment Request UI.";

components/test/data/payments/enforce_full_delegation.com/index.js

@@ -4,35 +4,59 @@
  * found in the LICENSE file.
  */
 
-const methodName = window.location.origin + '/method_manifest.json';
-const swSrcUrl = 'app.js';
+const CURRENT_URL = window.location.origin + window.location.pathname;
+const METHOD_NAME = CURRENT_URL.substring(0, CURRENT_URL.lastIndexOf('/')) +
+    '/method_manifest.json';
+const SW_SRC_URL = 'app.js';
 let request;
 let supportedInstruments = [];
 
 /**
- * Install a payment app.
- * @return {string} - a message indicating whether the installation is
- *  successful.
+ * Installs the given payment handler with the given payment method.
+ * @param {string} method - The payment method that this service worker
+ *    supports.
+ * @return {Promise<string>} - 'success' or error message on failure.
  */
-async function install() { // eslint-disable-line no-unused-vars
+async function install(method=METHOD_NAME) { // eslint-disable-line no-unused-vars, max-len
   info('installing');
-
-  await navigator.serviceWorker.register(swSrcUrl);
-  const registration = await navigator.serviceWorker.ready;
-  if (!registration.paymentManager) {
-    return 'No payment handler capability in this browser. Is' +
-        'chrome://flags/#service-worker-payment-apps enabled?';
+  try {
+    const registration = await navigator.serviceWorker.register(SW_SRC_URL);
+    await activation(registration);
+    await registration.paymentManager.instruments.set(
+        'instrument-for-' + method, {name: 'Instrument Name', method});
+    return 'success';
+  } catch (e) {
+    return e.message;
   }
+}
 
-  if (!registration.paymentManager.instruments) {
-    return 'Payment handler is not fully implemented. ' +
-        'Cannot set the instruments.';
-  }
-  await registration.paymentManager.instruments.set('instrument-key', {
-    name: 'MaxPay',
-    method: methodName,
+/**
+ * Returns a promise that resolves when the service worker of the given
+ * registration has activated.
+ * @param {ServiceWorkerRegistration} registration - A service worker
+ * registration.
+ * @return {Promise<void>} - A promise that resolves when the service worker
+ * has activated.
+ */
+async function activation(registration) {
+  return new Promise((resolve) => {
+    if (registration.active) {
+      resolve();
+      return;
+    }
+    registration.addEventListener('updatefound', () => {
+      const newWorker = registration.installing;
+      if (newWorker.state == 'activated') {
+        resolve();
+        return;
+      }
+      newWorker.addEventListener('statechange', () => {
+        if (newWorker.state == 'activated') {
+          resolve();
+        }
+      });
+    });
   });
-  return 'success';
 }
 
 /**
@@ -41,7 +65,7 @@ async function install() { // eslint-disable-line no-unused-vars
  */
 async function uninstall() { // eslint-disable-line no-unused-vars
   info('uninstall');
-  let registration = await navigator.serviceWorker.getRegistration(swSrcUrl);
+  let registration = await navigator.serviceWorker.getRegistration(SW_SRC_URL);
   if (!registration) {
     return 'The Payment handler has not been installed yet.';
   }
@@ -59,7 +83,7 @@ async function enableDelegations(delegations) { // eslint-disable-line no-unused
   try {
     await navigator.serviceWorker.ready;
     let registration =
-        await navigator.serviceWorker.getRegistration(swSrcUrl);
+        await navigator.serviceWorker.getRegistration(SW_SRC_URL);
     if (!registration) {
       return 'The payment handler is not installed.';
     }
@@ -78,16 +102,18 @@ async function enableDelegations(delegations) { // eslint-disable-line no-unused
 }
 
 /**
- * Add a payment method to the payment request.
- * @param {string} method - the payment method.
+ * Add payment methods to the payment request.
+ * @param {string[]} methods - the payment methods.
  * @return {string} - a message indicating whether the operation is successful.
  */
-function addSupportedMethod(method) { // eslint-disable-line no-unused-vars
-  info('addSupportedMethod: ' + method);
-  supportedInstruments.push({
-    supportedMethods: [
-      method,
-    ],
+function addSupportedMethods(methods) { // eslint-disable-line no-unused-vars
+  info('addSupportedMethods: ' + JSON.stringify(methods));
+  methods.forEach((method)=>{
+    supportedInstruments.push({
+      supportedMethods: [
+        method,
+      ],
+    });
   });
   return 'success';
 }
@@ -97,7 +123,7 @@ function addSupportedMethod(method) { // eslint-disable-line no-unused-vars
  * @return {string} - a message indicating whether the operation is successful.
  */
 function addDefaultSupportedMethod() { // eslint-disable-line no-unused-vars
-  return addSupportedMethod(methodName);
+  return addSupportedMethods([METHOD_NAME]);
 }
 
 /**