It might be better to move m_impl handling of JS DOM class wrappers outside binding generator
Created attachment 262153 [details] Patch
Comment on attachment 262153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262153&action=review This is OK, but I think we can do a little better. See comments below. Also, since we’re going to touch every call site, I think it’s useful to think about the fact that impl() is not a great name. The thing that’s wrapped by a wrapper doesn’t need to be named “impl” nor is “implementation object” and ideal term for it. I think I’d call it the “wrapped object” or “underlying object”. > Source/WebCore/bindings/js/JSDOMWrapper.h:51 > +template<typename ImplementationClass> class JSDOMWrapperWithImplementation : public JSDOMWrapper { Why such a long name? Is there code uses JSDOMWrapper directly? Can’t this class template have the shorter name? > Source/WebCore/bindings/js/JSDOMWrapper.h:53 > + typedef JSDOMWrapper Base; These kinds of typedefs can be dangerous, because if I derive from this class template, I will inherit an inaccurate typedef “Base” that is actually my grandparent base class, not my immediate base class. Does the benefit outweigh the risk here? I suppose we were using this already. > Source/WebCore/bindings/js/JSDOMWrapper.h:55 > + ImplementationClass& impl() const { return *m_impl; } This isn’t safe to call once releaseImpl() is called. I don’t understand why we have releaseImpl(). > Source/WebCore/bindings/js/JSDOMWrapper.h:56 > + ~JSDOMWrapperWithImplementation() { releaseImpl(); } This is only needed because we use a raw pointer rather than a Ref or RefPtr. Why do we do that? > Source/WebCore/bindings/js/JSDOMWrapper.h:59 > + JSDOMWrapperWithImplementation(JSC::Structure* structure, JSC::JSGlobalObject* globalObject, Ref<ImplementationClass>&& impl) The structure and global object arguments should probably be references instead of pointers, longer term. Don’t need to change that at this time. > Source/WebCore/bindings/js/JSDOMWrapper.h:63 > + void releaseImpl() { std::exchange(m_impl, nullptr)->deref(); } What call site needs releaseImpl()? I ask because it forces us to use RefPtr instead of Ref. > Source/WebCore/bindings/js/JSDOMWrapper.h:66 > + ImplementationClass* m_impl; It seems strange to use a raw pointer here instead of a Ref or RefPtr. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1033 > + push(@headerContent, " ~${className}() { }\n"); I think that "= default" would be better. But also omitting any explicit destructor declaration entirely would be even better.
Comment on attachment 262153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262153&action=review >> Source/WebCore/bindings/js/JSDOMWrapper.h:53 >> + typedef JSDOMWrapper Base; > > These kinds of typedefs can be dangerous, because if I derive from this class template, I will inherit an inaccurate typedef “Base” that is actually my grandparent base class, not my immediate base class. Does the benefit outweigh the risk here? I suppose we were using this already. It is not that much shorter. I personally would just get rid of this typedef.
Created attachment 262247 [details] Patch for landing
Comment on attachment 262247 [details] Patch for landing Clearing flags on attachment: 262247 Committed r190403: <http://trac.webkit.org/changeset/190403>
All reviewed patches have been landed. Closing bug.
Thanks for the reviews. I agree with most of the points. It is difficult though to integrate some of them as that would touch other files as well. > Also, since we’re going to touch every call site, I think it’s useful to > think about the fact that impl() is not a great name. The thing that’s > wrapped by a wrapper doesn’t need to be named “impl” nor is “implementation > object” and ideal term for it. I think I’d call it the “wrapped object” or > “underlying object”. wrapped() is good to me. We already have toWrapped for instance. impl() is already in use in other parts of the code. It might be better to change the name as a follow-up patch. > > Source/WebCore/bindings/js/JSDOMWrapper.h:51 > > +template<typename ImplementationClass> class JSDOMWrapperWithImplementation : public JSDOMWrapper { > > Why such a long name? Is there code uses JSDOMWrapper directly? Can’t this > class template have the shorter name? How about moving JSDOMWrapper to JSDOMObject and JSDOMWrapperWithImplementation to JSDOMWrapper? > > > Source/WebCore/bindings/js/JSDOMWrapper.h:53 > > + typedef JSDOMWrapper Base; > > These kinds of typedefs can be dangerous, because if I derive from this > class template, I will inherit an inaccurate typedef “Base” that is actually > my grandparent base class, not my immediate base class. Does the benefit > outweigh the risk here? I suppose we were using this already. Yes, it is in use in JSC. It is also nice when generating code, see Base::impl() for instance in CodeGeneratorJS.pm. Whatever is done, Base will be defined for all JS DOM wrappers: it is already defined by JSDOMWrapper and JSDestructibleObject for instance. > It is not that much shorter. I personally would just get rid of this typedef. I will fix that when renaming m_impl -> m_wrapped. > > > Source/WebCore/bindings/js/JSDOMWrapper.h:55 > > + ImplementationClass& impl() const { return *m_impl; } > > This isn’t safe to call once releaseImpl() is called. I don’t understand why > we have releaseImpl(). I removed it in the patch. > > Source/WebCore/bindings/js/JSDOMWrapper.h:56 > > + ~JSDOMWrapperWithImplementation() { releaseImpl(); } > > This is only needed because we use a raw pointer rather than a Ref or > RefPtr. Why do we do that? > > > Source/WebCore/bindings/js/JSDOMWrapper.h:59 > > + JSDOMWrapperWithImplementation(JSC::Structure* structure, JSC::JSGlobalObject* globalObject, Ref<ImplementationClass>&& impl) > > The structure and global object arguments should probably be references > instead of pointers, longer term. Don’t need to change that at this time. Time permitting, I would like to tackle this one. > > Source/WebCore/bindings/js/JSDOMWrapper.h:63 > > + void releaseImpl() { std::exchange(m_impl, nullptr)->deref(); } > > What call site needs releaseImpl()? I ask because it forces us to use RefPtr > instead of Ref. > > > Source/WebCore/bindings/js/JSDOMWrapper.h:66 > > + ImplementationClass* m_impl; > > It seems strange to use a raw pointer here instead of a Ref or RefPtr. I think it is ok to move to Ref<> It has some consequences related to const, so better do that as a follow-up patch. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1033 > > + push(@headerContent, " ~${className}() { }\n"); > > I think that "= default" would be better. But also omitting any explicit > destructor declaration entirely would be even better. OK, I removed it.