[webkit-reviews] review canceled: [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]

Ryosuke Niwa <rniwa at webkit.org> has canceled  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 Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73842&action=review

Because these are major changes, I'll re-submit patch for another review.

>> 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.

Will do.

>> 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!

Protector?  Right, this and the one below are safe.  Will remove.

>> 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>

Will do.

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

Will do.

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

Ditto.

>> 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*.

Oops, I meant to say "operator T*()" instead of "T* operator*()" there. 
Obviously, operator*() should return the reference...
I'm adding implicit conversion to T*.