Bug 160926 – Merge toNativeArray and toRefPtrNativeArray

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Bindings

[Ryosuke Niwa]

Merge these two functions to reduce the code duplication.

[Ryosuke Niwa]

Created attachment 286282 [details]
Merges the functions

[Ryosuke Niwa]

Created attachment 286330 [details]
Rebaselined binding tests

[Ryosuke Niwa]

Created attachment 286334 [details]
Updated for ToT

[Sam Weinig]

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).

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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.

[Sam Weinig]

(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?

[Sam Weinig]

(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.

[Ryosuke Niwa]

Ugh... a bunch of inspector code relies on the fact JSNode::toWrapped doesn’t require ExecState.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Nope :(. SerializedScriptValue also assumes you can call toWrapped without ExecState.

[Ryosuke Niwa]

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);
}

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

Comment on attachment 286334 [details]
Updated for ToT

I'm gonna also give up on this patch after bug 161030.

[Sam Weinig]

(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.

[Alexey Shvayka]

to[RefPtr]NativeArray() were merged to Converter<IDLSequence<T>> in r207150.

*** This bug has been marked as a duplicate of bug 163247 ***