We should remove or replace toJS template methods taking const Ref and const RefPtr.
Created attachment 279119 [details] Patch
I kept using toJS in the JS constructor. But I am wondering whether toJSNewlyCreated could always be used instead.
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 :/
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.
(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>&&).
(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.
> 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?
(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.
(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.
Created attachment 279144 [details] Patch
Comment on attachment 279144 [details] Patch r=me, thanks.
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.
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
Comment on attachment 279144 [details] Patch Clearing flags on attachment: 279144 Committed r201070: <http://trac.webkit.org/changeset/201070>
All reviewed patches have been landed. Closing bug.