Avoid unnecessary null checks in toJS() when the implementation returns a reference or Ref<>.
Created attachment 278694 [details] Patch
Created attachment 278695 [details] Patch
Comment on attachment 278694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278694&action=review Should have renamed this to convert and changed all the ExecState* to ExecState&. OK, just kidding. A few comments: > Source/WebCore/bindings/js/JSDOMBinding.h:255 > template<typename T> JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, PassRefPtr<T>); Why not change this to const PassRefPtr<T>&, at least until we can delete it. > Source/WebCore/bindings/js/JSDOMBinding.h:559 > + return toJS(exec, globalObject, const_cast<Ref<T>&>(ptr).get()); Why not do the const cast on the result of get() instead? > Source/WebCore/bindings/js/JSDOMBinding.h:574 > + return toJSNewlyCreated(exec, globalObject, const_cast<Ref<T>&>(ptr).get()); Ditto. > Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp:43 > + return wrap<JSAllVideoCapabilities>(globalObject, static_cast<AllVideoCapabilities>(object)); Pre-existing: This seems like a peculiar cast. Shouldn’t it be AllVideoCapabilities&? Do we really want to copy the object? > Source/WebCore/bindings/js/JSMediaStreamCapabilitiesCustom.cpp:45 > + return wrap<JSAllAudioCapabilities>(globalObject, static_cast<AllAudioCapabilities>(object)); Ditto.
Comment on attachment 278695 [details] Patch Attachment 278695 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1307533 Number of test failures exceeded the failure limit.
Created attachment 278698 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 278699 [details] Patch
Comment on attachment 278699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278699&action=review > Source/WebCore/bindings/js/JSDOMBinding.h:559 > + return toJS(exec, globalObject, const_cast<T&>(ptr.get())); Do we really need to add a const_cast? Couldn't we make the parameter a Ref<T>& or make toJS take a const T&? > Source/WebCore/bindings/js/JSDOMBinding.h:574 > + return toJSNewlyCreated(exec, globalObject, const_cast<T&>(ptr.get())); ditto.
Comment on attachment 278699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278699&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3414 > + void* actualVTablePointer = *(reinterpret_cast<void**>(&impl)); Extra unneeded parentheses here around the reinterpret_cast.
Comment on attachment 278699 [details] Patch Clearing flags on attachment: 278699 Committed r200775: <http://trac.webkit.org/changeset/200775>
All reviewed patches have been landed. Closing bug.
Comment on attachment 278699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278699&action=review >> Source/WebCore/bindings/js/JSDOMBinding.h:559 >> + return toJS(exec, globalObject, const_cast<T&>(ptr.get())); > > Do we really need to add a const_cast? Couldn't we make the parameter a Ref<T>& or make toJS take a const T&? If the parameter would be a Ref<T>&, then it wouldn't be able to bind to a temporary. E.g. return toJS(state, globalObject, impl.createObject()); would not work if impl.createObject() returns a Ref<>
Looks like a 2% progression on Speedometer on iOS.
(In reply to comment #12) > Looks like a 2% progression on Speedometer on iOS. And 3% on JSBench on iOS.
!!!
(In reply to comment #13) > (In reply to comment #12) > > Looks like a 2% progression on Speedometer on iOS. > > And 3% on JSBench on iOS. And it looks like 2% progression on JetStream / iOS as well !? I did not know JSBench / JetStream used our JS bindings at all.
(In reply to comment #11) > Comment on attachment 278699 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278699&action=review > > >> Source/WebCore/bindings/js/JSDOMBinding.h:559 > >> + return toJS(exec, globalObject, const_cast<T&>(ptr.get())); > > > > Do we really need to add a const_cast? Couldn't we make the parameter a Ref<T>& or make toJS take a const T&? > > If the parameter would be a Ref<T>&, then it wouldn't be able to bind to a > temporary. E.g. > return toJS(state, globalObject, impl.createObject()); > > would not work if impl.createObject() returns a Ref<> I updated JSDOMBinding.h in bug 157307 to cope with that. It might be good also to make toJSNewlyCreated() take Ref<>/RefPtr<> or Ref<>&&/RefPtr<>&& and not raw pointers/references. That may help removing some count churnning. I'll look at that, hopefully next week.
(In reply to comment #16) > (In reply to comment #11) > > Comment on attachment 278699 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=278699&action=review > > > > >> Source/WebCore/bindings/js/JSDOMBinding.h:559 > > >> + return toJS(exec, globalObject, const_cast<T&>(ptr.get())); > > > > > > Do we really need to add a const_cast? Couldn't we make the parameter a Ref<T>& or make toJS take a const T&? > > > > If the parameter would be a Ref<T>&, then it wouldn't be able to bind to a > > temporary. E.g. > > return toJS(state, globalObject, impl.createObject()); > > > > would not work if impl.createObject() returns a Ref<> > > I updated JSDOMBinding.h in bug 157307 to cope with that. > > It might be good also to make toJSNewlyCreated() take Ref<>/RefPtr<> or > Ref<>&&/RefPtr<>&& and not raw pointers/references. That may help removing > some count churnning. > I'll look at that, hopefully next week. I believe this patch did this as well unless I misunderstood you.
> > It might be good also to make toJSNewlyCreated() take Ref<>/RefPtr<> or > > Ref<>&&/RefPtr<>&& and not raw pointers/references. That may help removing > > some count churnning. > > I'll look at that, hopefully next week. > > I believe this patch did this as well unless I misunderstood you. IIANM, toJSNewlyCreated is basically calling createWrapper. Currently, createWrapper is taking a raw pointer. It then creates a Ref<> when calling JSXX::create, thus incrementing the DOM object counter. It might be good to have createWrapper take a Ref<> or Ref<>&&. That way, when toJSNewlyCreated is passed a Ref<>&&, it would be moved to createWrapper and there would be no counter modification. toJS may be in scope also but I am less sure of its usefulness.