Summary: | Merge toNativeArray and toRefPtrNativeArray | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
Component: | Bindings | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED DUPLICATE | ||||||||||
Severity: | Normal | CC: | ashvayka, cdumez, cgarcia, commit-queue, darin, sam | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 161030 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2016-08-16 17:32:09 PDT
Created attachment 286282 [details]
Merges the functions
Created attachment 286330 [details]
Rebaselined binding tests
Created attachment 286334 [details]
Updated for ToT
I wonder if we can do this without adding separate toWrapped overload for each wrapper type. Instead, maybe do something like: template<> struct JSDOMWrapperConverterTraits<Element> { typedef JSElement WrapperType; }; And then, you could have a single toWrapped in JSDOMBindings.h that looks something like: template<typename NativeType> NativeType* toWrapped(JSC::ExecState& state, JSC::JSValue jsValue) { typedef typename JSDOMWrapperConverterTraits<NativeType>::WrapperType WrapperType; return WrapperType::toWrapped(value) } (As usual, C++ typed into a comment is not guaranteed to work). (In reply to comment #4) > I wonder if we can do this without adding separate toWrapped overload for > each wrapper type. Instead, maybe do something like: > > template<> struct JSDOMWrapperConverterTraits<Element> { > typedef JSElement WrapperType; > }; > > And then, you could have a single toWrapped in JSDOMBindings.h that looks > something like: > > template<typename NativeType> > NativeType* toWrapped(JSC::ExecState& state, JSC::JSValue jsValue) > { > typedef typename JSDOMWrapperConverterTraits<NativeType>::WrapperType > WrapperType; > return WrapperType::toWrapped(value) > } > > (As usual, C++ typed into a comment is not guaranteed to work). We’d have to define both WrapperType and NativeType (because some wrappers use RefPtr instead of raw pointer) but sure. Actually, no quite because toWrapped takes ExecState in addition to JSValue when JSCustomToNativeObject is set :( We could change it so that it always takes two arguments, or add yet another trait which tells us whether the function takes one or two arguments. (In reply to comment #5) > We’d have to define both WrapperType and NativeType (because some wrappers > use RefPtr instead of raw pointer) but sure. Really? Which ones? (In reply to comment #6) > Actually, no quite because toWrapped takes ExecState in addition to JSValue > when JSCustomToNativeObject is set :( > > We could change it so that it always takes two arguments, or add yet another > trait which tells us whether the function takes one or two arguments. I think changing toWrapped to always take both makes sense. Having different signatures is really weird here. Ugh... a bunch of inspector code relies on the fact JSNode::toWrapped doesn’t require ExecState. (In reply to comment #9) > Ugh... a bunch of inspector code relies on the fact JSNode::toWrapped > doesn’t require ExecState. Oh, I can just call jsNodeCast for that. Nope :(. SerializedScriptValue also assumes you can call toWrapped without ExecState. Actually, getting rid of ExecState is easier. Only two toWrapped functions use ExecState, and they’re probably just written by someone who didn’t understand how toWrapped function is used: RefPtr<DOMStringList> JSDOMStringList::toWrapped(ExecState& state, JSValue value) { if (value.inherits(JSDOMStringList::info())) return &jsCast<JSDOMStringList*>(asObject(value))->wrapped(); if (!isJSArray(value)) return nullptr; JSArray* array = asArray(value); RefPtr<DOMStringList> stringList = DOMStringList::create(); for (unsigned i = 0; i < array->length(); ++i) stringList->append(array->getIndex(&state, i).toString(&state)->value(&state)); return stringList; } RefPtr<XPathNSResolver> JSXPathNSResolver::toWrapped(ExecState& state, JSValue value) { if (value.inherits(JSXPathNSResolver::info())) return &jsCast<JSXPathNSResolver*>(asObject(value))->wrapped(); return JSCustomXPathNSResolver::create(&state, value); } Hm... actually, the one for XPathNSResolver was added recently in https://trac.webkit.org/changeset/200550. Chris, why does toWrapped need to call XPathNSResolver::create when the wrapped JSValue isn’t XPathNSResolver? Comment on attachment 286334 [details] Updated for ToT I'm gonna also give up on this patch after bug 161030. (In reply to comment #14) > Comment on attachment 286334 [details] > Updated for ToT > > I'm gonna also give up on this patch after bug 161030. :(. Re-opening as we really should do this. to[RefPtr]NativeArray() were merged to Converter<IDLSequence<T>> in r207150. *** This bug has been marked as a duplicate of bug 163247 *** |