[Extensions Bindings] Add support for returns_async schema format

Changes the expected schema format that indicates promise support to no
longer specify a callback in the parameters, but instead supply a
"returns_async" key. This key species the format of the return value to
the promise, or to the callback if one is supplied. Also adjusts
associated tests to check for such.

Bug: 978538
Change-Id: Ifcb0b8e8152fd6b6c9049e2d856b10d577c955fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2253205Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Tim Judkins <tjudkins@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782031}

extensions/renderer/bindings/api_binding.cc

@@ -65,24 +65,32 @@ struct SignaturePair {
   std::unique_ptr<APISignature> callback_signature;
 };
 
-SignaturePair GetAPISignatureFromDictionary(const base::DictionaryValue* dict) {
-  const base::ListValue* params = nullptr;
-  CHECK(dict->GetList("parameters", &params));
+SignaturePair GetAPISignatureFromDictionary(const base::Value* dict) {
+  const base::Value* params =
+      dict->FindKeyOfType("parameters", base::Value::Type::LIST);
+  CHECK(params);
 
-  bool supports_promises = false;
-  dict->GetBoolean("supportsPromises", &supports_promises);
+  // The inclusion of the "returns_async" property indicates that an API
+  // supports promises.
+  const base::Value* returns_async =
+      dict->FindKeyOfType("returns_async", base::Value::Type::DICTIONARY);
 
   SignaturePair result;
-  result.method_signature = std::make_unique<APISignature>(*params);
+  result.method_signature =
+      std::make_unique<APISignature>(*params, returns_async);
   result.method_signature->set_promise_support(
-      supports_promises ? binding::PromiseSupport::kAllowed
-                        : binding::PromiseSupport::kDisallowed);
+      returns_async && APIBinding::enable_promise_support_for_testing
+          ? binding::PromiseSupport::kAllowed
+          : binding::PromiseSupport::kDisallowed);
   // If response validation is enabled, parse the callback signature. Otherwise,
   // there's no reason to, so don't bother.
   if (result.method_signature->has_callback() &&
       binding::IsResponseValidationEnabled()) {
-    const base::Value* callback_params = params->GetList().back().FindKeyOfType(
-        "parameters", base::Value::Type::LIST);
+    const base::Value* callback_params =
+        returns_async ? returns_async->FindKeyOfType("parameters",
+                                                     base::Value::Type::LIST)
+                      : params->GetList().back().FindKeyOfType(
+                            "parameters", base::Value::Type::LIST);
     if (callback_params) {
       const base::ListValue* params_as_list = nullptr;
       callback_params->GetAsList(&params_as_list);
@@ -531,6 +539,9 @@ void APIBinding::DecorateTemplateWithProperties(
   }
 }
 
+// static
+bool APIBinding::enable_promise_support_for_testing = false;
+
 // static
 void APIBinding::GetEventObject(
     v8::Local<v8::Name> property,

extensions/renderer/bindings/api_binding.h

@@ -83,6 +83,11 @@ class APIBinding {
 
   APIBindingHooks* hooks() { return binding_hooks_.get(); }
 
+  // Global bool to allow for testing of promise support.
+  // TODO(tjudkins): Replace this with a runtime determined condition gated on
+  // MV3.
+  static bool enable_promise_support_for_testing;
+
  private:
   // Initializes the object_template_ for this API. Called lazily when the
   // first instance is created.

extensions/renderer/bindings/api_binding_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "extensions/renderer/bindings/api_binding.h"
 
+#include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
@@ -1672,21 +1673,22 @@ TEST_F(APIBindingUnittest,
 
 // Tests promise-based APIs exposed on bindings.
 TEST_F(APIBindingUnittest, PromiseBasedAPIs) {
+  // TODO(tjudkins): Remove this once promise support is fully integrated into
+  // the API calling flow.
+  base::AutoReset<bool> auto_reset(
+      &APIBinding::enable_promise_support_for_testing, true);
+
   constexpr char kFunctions[] =
       R"([{
             'name': 'supportsPromises',
-            'supportsPromises': true,
             'parameters': [{
               'name': 'int',
               'type': 'integer'
-            }, {
-              'name': 'callback',
-              'type': 'function',
-              'parameters': [{
-                'name': 'strResult',
-                'type': 'string'
-              }]
-            }]
+            }],
+            "returns_async": {
+              'name': 'strResult',
+              'type': 'string'
+            }
           }])";
   SetFunctions(kFunctions);
 
@@ -1701,7 +1703,7 @@ TEST_F(APIBindingUnittest, PromiseBasedAPIs) {
         R"((function(api) {
              this.apiResult = api.supportsPromises(3);
              this.apiResult.then((strResult) => {
-               this.strResult = strResult;
+               this.promiseResult = strResult;
              });
            }))";
     v8::Local<v8::Function> promise_api_call =
@@ -1723,8 +1725,29 @@ TEST_F(APIBindingUnittest, PromiseBasedAPIs) {
 
     EXPECT_EQ(v8::Promise::kFulfilled, promise->State());
     EXPECT_EQ(R"("foo")", V8ToString(promise->Result(), context));
-    EXPECT_EQ(R"("foo")", GetStringPropertyFromObject(context->Global(),
-                                                      context, "strResult"));
+    EXPECT_EQ(R"("foo")", GetStringPropertyFromObject(
+                              context->Global(), context, "promiseResult"));
+  }
+  // Also test that promise-based APIs still support passing a callback.
+  {
+    constexpr char kFunctionCall[] =
+        R"((function(api) {
+             api.supportsPromises(3, (strResult) => {
+               this.callbackResult = strResult
+             });
+           }))";
+    v8::Local<v8::Function> promise_api_call =
+        FunctionFromString(context, kFunctionCall);
+    v8::Local<v8::Value> args[] = {binding_object};
+    RunFunctionOnGlobal(promise_api_call, context, base::size(args), args);
+
+    ASSERT_TRUE(last_request());
+    request_handler()->CompleteRequest(last_request()->request_id,
+                                       *ListValueFromString(R"(["bar"])"),
+                                       std::string());
+
+    EXPECT_EQ(R"("bar")", GetStringPropertyFromObject(
+                              context->Global(), context, "callbackResult"));
   }
 }
 

extensions/renderer/bindings/api_signature.cc

@@ -19,10 +19,9 @@ namespace extensions {
 namespace {
 
 bool HasCallback(const std::vector<std::unique_ptr<ArgumentSpec>>& signature) {
-  // TODO(devlin): This is how extension APIs have always determined if a
-  // function has a callback, but it seems a little silly. In the long run (once
-  // signatures are generated), it probably makes sense to indicate this
-  // differently.
+  // TODO(tjudkins): Once we change the APISignature to represent the whole
+  // signature including any asynchronous return, we should replace this with a
+  // method on the APISignature object itself.
   return !signature.empty() &&
          signature.back()->type() == ArgumentType::FUNCTION;
 }
@@ -409,6 +408,27 @@ APISignature::APISignature(const base::ListValue& specification) {
   has_callback_ = HasCallback(signature_);
 }
 
+APISignature::APISignature(const base::Value& specification_list,
+                           bool supports_promises) {
+  auto size = specification_list.GetList().size() + (supports_promises ? 1 : 0);
+  signature_.reserve(size);
+  for (const auto& value : specification_list.GetList()) {
+    CHECK(value.is_dict());
+    signature_.push_back(std::make_unique<ArgumentSpec>(value));
+  }
+  // To allow promise supporting APIs to instead take a callback, we add an
+  // allowed function to the end of the signature.
+  if (supports_promises) {
+    auto callback = std::make_unique<ArgumentSpec>(ArgumentType::FUNCTION);
+    callback->set_name("callback");
+    signature_.push_back(std::move(callback));
+  }
+
+  has_callback_ = HasCallback(signature_);
+  DCHECK(!supports_promises || has_callback_)
+      << "If an API supports promises, it must also support callbacks";
+}
+
 APISignature::APISignature(std::vector<std::unique_ptr<ArgumentSpec>> signature)
     : signature_(std::move(signature)),
       has_callback_(HasCallback(signature_)) {}

extensions/renderer/bindings/api_signature.h

@@ -28,6 +28,7 @@ class ArgumentSpec;
 class APISignature {
  public:
   explicit APISignature(const base::ListValue& specification);
+  APISignature(const base::Value& specification_list, bool supports_promises);
   explicit APISignature(std::vector<std::unique_ptr<ArgumentSpec>> signature);
   ~APISignature();