Bug 160926 - Merge toNativeArray and toRefPtrNativeArray
Summary: Merge toNativeArray and toRefPtrNativeArray
Status: RESOLVED DUPLICATE of bug 163247
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 161030
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-16 17:32 PDT by Ryosuke Niwa
Modified: 2020-08-07 12:39 PDT (History)
6 users (show)

See Also:


Attachments
Merges the functions (10.81 KB, patch)
2016-08-16 23:46 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Rebaselined binding tests (38.44 KB, patch)
2016-08-17 14:36 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated for ToT (37.04 KB, patch)
2016-08-17 15:48 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-08-16 17:32:09 PDT
Merge these two functions to reduce the code duplication.
Comment 1 Ryosuke Niwa 2016-08-16 23:46:07 PDT
Created attachment 286282 [details]
Merges the functions
Comment 2 Ryosuke Niwa 2016-08-17 14:36:45 PDT
Created attachment 286330 [details]
Rebaselined binding tests
Comment 3 Ryosuke Niwa 2016-08-17 15:48:06 PDT
Created attachment 286334 [details]
Updated for ToT
Comment 4 Sam Weinig 2016-08-17 20:45:37 PDT
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).
Comment 5 Ryosuke Niwa 2016-08-18 13:03:20 PDT
(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.
Comment 6 Ryosuke Niwa 2016-08-18 13:07:44 PDT
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.
Comment 7 Sam Weinig 2016-08-18 15:26:28 PDT
(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?
Comment 8 Sam Weinig 2016-08-18 15:28:59 PDT
(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.
Comment 9 Ryosuke Niwa 2016-08-18 22:44:08 PDT
Ugh... a bunch of inspector code relies on the fact JSNode::toWrapped doesn’t require ExecState.
Comment 10 Ryosuke Niwa 2016-08-18 22:52:26 PDT
(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.
Comment 11 Ryosuke Niwa 2016-08-18 23:36:06 PDT
Nope :(. SerializedScriptValue also assumes you can call toWrapped without ExecState.
Comment 12 Ryosuke Niwa 2016-08-19 00:01:16 PDT
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);
}
Comment 13 Ryosuke Niwa 2016-08-19 00:08:35 PDT
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 14 Ryosuke Niwa 2016-08-31 10:39:36 PDT
Comment on attachment 286334 [details]
Updated for ToT

I'm gonna also give up on this patch after bug 161030.
Comment 15 Sam Weinig 2016-09-02 13:48:09 PDT
(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.
Comment 16 Alexey Shvayka 2020-08-07 12:39:33 PDT
to[RefPtr]NativeArray() were merged to Converter<IDLSequence<T>> in r207150.

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