bindings: [NewObject] does not work with Promise return types |
||
Issue descriptionhttps://fetch.spec.whatwg.org/#fetch-method specifies the global fetch() operation as partial interface WindowOrWorkerGlobalScope { [NewObject] Promise<Response> fetch(RequestInfo input, optional RequestInit init); }; our IDL file in //third_party/WebKit/Source/modules/fetch/WindowFetch.idl has a few other extended attributes, but not [NewObject]. If I try to add it, the generated .cpp file does not build: gen/blink/bindings/modules/v8/V8WindowPartial.cpp:719:10: error: invalid argument type 'blink::ScriptPromise' to unary expression DCHECK(!result || DOMDataStore::GetWrapper(result, info.GetIsolate()).IsEmpty()); the code actually looks like this: ScriptPromise result = GlobalFetch::fetch(scriptState, *impl, input, init, exceptionState); if (exceptionState.HadException()) { return; } // [NewObject] must always create a new wrapper. Check that a wrapper // does not exist yet. DCHECK(!result || DOMDataStore::GetWrapper(result, info.GetIsolate()).IsEmpty()); which fails because "!result" doesn't work with ScriptPromises, only interface types (which are pointers in this context).
,
Sep 26 2017
Your point makes sense to me. Ideally, I'd like to guarantee that, when [NewObject] specified, the returned object is always a new object. Theoretically it's possible even in the case of Promises. Having said that, considering the current implementation of Promises and bindings, it seems not easy to perform such a check. I agree to not add a DCHECK in case of promises. The DCHECK is a kind of nice-to-have and not mandatory.
,
Sep 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9fdf626bf71aadde6290026810dc7438e3f50886 commit 9fdf626bf71aadde6290026810dc7438e3f50886 Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Date: Wed Sep 27 07:51:42 2017 bindings: Make [NewObject] a no-op when an operation returns a Promise. WebIDL commit f0e58c36c912b1 ("Allow [NewObject] to be used on promise typed things") made it possible to use the [NewObject] extended attribute on methods that return promises in addition to methods that return interfaces. Doing so did not work at the moment, as we would add a DCHECK to the generated code that only made sense for interface types: we check a pointer and call DOMDataStore::GetWrapper(), whereas promises are returned as ScriptPromise types and we call V8SetReturnValue() with a v8::Value directly. For now, just do not add the DCHECK for promises. Bug: 462913 , 767442 Change-Id: I171ffd543daa39a4c334315448979d75c683f828 Reviewed-on: https://chromium-review.googlesource.com/684836 Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com> Cr-Commit-Position: refs/heads/master@{#504597} [modify] https://crrev.com/9fdf626bf71aadde6290026810dc7438e3f50886/third_party/WebKit/Source/bindings/IDLExtendedAttributes.md [modify] https://crrev.com/9fdf626bf71aadde6290026810dc7438e3f50886/third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl [modify] https://crrev.com/9fdf626bf71aadde6290026810dc7438e3f50886/third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl [modify] https://crrev.com/9fdf626bf71aadde6290026810dc7438e3f50886/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp [modify] https://crrev.com/9fdf626bf71aadde6290026810dc7438e3f50886/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.h
,
Sep 27 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by raphael....@intel.com
, Sep 26 2017