Bug 157791 – Remove toJS template methods taking const Ref and const RefPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

We should remove or replace toJS template methods taking const Ref and const RefPtr.

[youenn fablet]

Created attachment 279119 [details]
Patch

[youenn fablet]

I kept using toJS in the JS constructor.
But I am wondering whether toJSNewlyCreated could always be used instead.

[Chris Dumez]

Comment on attachment 279119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279119&action=review

> Source/WebCore/bindings/js/JSDOMBinding.h:-259
> -template<typename T> JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, const Ref<T>&);

I believe you need one that takes a Ref<>&& then. Otherwise, we end up calling toJS(JSC::ExecState*, JSDOMGlobalObject*, RefPtr<T>&&) if someone gives us a Ref<>&& and we end up doing an unnecessary null check :/

[Chris Dumez]

Comment on attachment 279119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279119&action=review

> Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:243
> +    return JSValue::encode(asObject(toJS(state, castedThis->globalObject(), WTFMove(object))));

I believe you are right that we could use toJSNewlyCreated() in constructors but let's do this separately in a follow-up patch.

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 279119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279119&action=review
> 
> > Source/WebCore/bindings/js/JSDOMBinding.h:-259
> > -template<typename T> JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, const Ref<T>&);
> 
> I believe you need one that takes a Ref<>&& then. Otherwise, we end up
> calling toJS(JSC::ExecState*, JSDOMGlobalObject*, RefPtr<T>&&) if someone
> gives us a Ref<>&& and we end up doing an unnecessary null check :/

It seems to me that Ref<T>-to-T& implict cast should take precedence here, at least in the case where there is a toJS that takes a T&. I will check this.

That said, maybe we should indeed stop using this implicit-cast (that RefPtr<> doesn't have) and add a toJS(Ref<T>&&).

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #3)
> > Comment on attachment 279119 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=279119&action=review
> > 
> > > Source/WebCore/bindings/js/JSDOMBinding.h:-259
> > > -template<typename T> JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, const Ref<T>&);
> > 
> > I believe you need one that takes a Ref<>&& then. Otherwise, we end up
> > calling toJS(JSC::ExecState*, JSDOMGlobalObject*, RefPtr<T>&&) if someone
> > gives us a Ref<>&& and we end up doing an unnecessary null check :/
> 
> It seems to me that Ref<T>-to-T& implict cast should take precedence here,
> at least in the case where there is a toJS that takes a T&. I will check
> this.
> 
> That said, maybe we should indeed stop using this implicit-cast (that
> RefPtr<> doesn't have) and add a toJS(Ref<T>&&).

I think that would be safer.

[youenn fablet]

> It seems to me that Ref<T>-to-T& implict cast should take precedence here,
> at least in the case where there is a toJS that takes a T&. I will check
> this.

I have not found any case where implicit cast is not picked.
I do not have an explanation of the performance loss.

I would be interested to see whether the current patch would actually create a performance loss. Is there a way to do so?

[Chris Dumez]

(In reply to comment #7)
> > It seems to me that Ref<T>-to-T& implict cast should take precedence here,
> > at least in the case where there is a toJS that takes a T&. I will check
> > this.
> 
> I have not found any case where implicit cast is not picked.
> I do not have an explanation of the performance loss.

What if you have a Ref<Text>&&? Are you sure it would use:
toJS(Node&) // note that Text does not have its own toJS() so we use the base class' one.
and not
toJS(RefPtr<Text>&&) // Which would end up calling toJS(Node*).
?

> I would be interested to see whether the current patch would actually create
> a performance loss. Is there a way to do so?

The easiest would be to land the patch and monitor the bots. However, I would still suggest keeping the Ref<>&& overload as it is just one line and it is clearer that the compiler will do the right thing always.

[youenn fablet]

(In reply to comment #8)
> (In reply to comment #7)
> > > It seems to me that Ref<T>-to-T& implict cast should take precedence here,
> > > at least in the case where there is a toJS that takes a T&. I will check
> > > this.
> > 
> > I have not found any case where implicit cast is not picked.
> > I do not have an explanation of the performance loss.
> 
> What if you have a Ref<Text>&&? Are you sure it would use:
> toJS(Node&) // note that Text does not have its own toJS() so we use the
> base class' one.
> and not
> toJS(RefPtr<Text>&&) // Which would end up calling toJS(Node*).
> ?

I tried this by tweaking Document.idl/createTextNode.

If the reason of the performance loss is the missing toJS, that may be an indication we should remove Ref implicit cast.

> > I would be interested to see whether the current patch would actually create
> > a performance loss. Is there a way to do so?
> 
> The easiest would be to land the patch and monitor the bots. However, I
> would still suggest keeping the Ref<>&& overload as it is just one line and
> it is clearer that the compiler will do the right thing always.

Is there a way to locally run those tests?
I'll update the patch with the additional Ref<>&& anyway.

[youenn fablet]

Created attachment 279144 [details]
Patch

[Chris Dumez]

Comment on attachment 279144 [details]
Patch

r=me, thanks.

[Build Bot]

Comment on attachment 279144 [details]
Patch

Attachment 279144 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1337812

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279154 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[WebKit Commit Bot]

Comment on attachment 279144 [details]
Patch

Clearing flags on attachment: 279144

Committed r201070: <http://trac.webkit.org/changeset/201070>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.