[webkit-reviews] review granted: [Bug 47688] Add a way to detect that a RenderObject* is being used across methods that might cause it to be destroyed : [Attachment 73842] added copy constructor and removed erroneous const

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 47688: Add a way to detect that a RenderObject* is being used across
methods that might cause it to be destroyed
https://bugs.webkit.org/show_bug.cgi?id=47688

Attachment 73842: added copy constructor and removed erroneous const
https://bugs.webkit.org/attachment.cgi?id=73842&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=73842&action=review

While this is not great, it’s good as is, so r=me.

I am not comfortable with the rules about where to use this smart pointer
class. It’s going to be tricky to teach people how to use it.

> WebCore/dom/Document.cpp:1530
> +   
ASSERT(!RenderObjectPointer<RenderObject>::rendererDestructionIsForbidden());

It’s strange to be calling this function on one particular class expanded from
the template rather than on the entire template. I think because of that, it
should be a non-member function.

> WebCore/rendering/RenderObject.cpp:235
> +    RenderObjectPointer<const RenderObject> forbidDestructionOf(this);

I do not think the name forbidDestructionOf is all that cute. Could we come up
with a less cute way to do this.

I still don’t think it makes sense to add this in all the places we add it in
this patch. For example, there is definitely no need for it in this one-line
function!

> WebCore/rendering/RenderObject.h:864
> +class RenderObjectPointer : public FastAllocBase {

We traditionally use the abbreviation Ptr in all our smart pointer classes. I
think RenderPtr would be a better name for this class template than
RenderObjectPointer.

This class template should have its own header file.

It would be better to say:

    template<typename T>

rather than:
    template<class T>

> WebCore/rendering/RenderObject.h:867
> +    : m_renderer(renderer)

We indent these.

> WebCore/rendering/RenderObject.h:875
> +    : m_renderer(o.m_renderer)

We indent these.

> WebCore/rendering/RenderObject.h:889
> +    T* operator*() const { return m_renderer; }

We also need to override operator-> so we can use these as normal pointers.

We’ll also need either a get() function or implicit conversion to T*.