Created attachment 403943 [details]
Patch

Address comments by Darin in bug 214144

Created attachment 403968 [details]
Patch

I also changed a call to createBoxPtr that was already in trunk.

Comment on attachment 403968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403968&action=review

> Source/WTF/wtf/BoxPtr.h:49
> +template<typename T> BoxPtr<T> adoptBoxPtr(T* ptr)

I don’t think adoptBoxPtr is quite the right name. The return value is a BoxPtr, the thing being adopted is not a BoxPtr. Maybe the name needs both the words create (for the BoxPtr) and adopt (for the thing it’s taking)?

> Source/WTF/wtf/BoxPtr.h:51
>      return BoxPtr<T>::create(ptr);

I think Box::create also needs to be renamed for the same reason; as with this function, it takes ownership of a raw pointer and so needs a special name making that clear.

(In reply to Darin Adler from comment #3)
> Comment on attachment 403968 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403968&action=review
> 
> > Source/WTF/wtf/BoxPtr.h:49
> > +template<typename T> BoxPtr<T> adoptBoxPtr(T* ptr)
> 
> I don’t think adoptBoxPtr is quite the right name. The return value is a
> BoxPtr, the thing being adopted is not a BoxPtr. Maybe the name needs both
> the words create (for the BoxPtr) and adopt (for the thing it’s taking)?

I guess it could be do something like adoptPointerIntoBoxPtr ? I cannot get a better name, but I guess we should iron it out before writing another patch, wdyt?

> > Source/WTF/wtf/BoxPtr.h:51
> >      return BoxPtr<T>::create(ptr);
> 
> I think Box::create also needs to be renamed for the same reason; as with
> this function, it takes ownership of a raw pointer and so needs a special
> name making that clear.

This create comes from Box class, something I don't think we should rename.

(In reply to Xabier Rodríguez Calvar from comment #4)
> (In reply to Darin Adler from comment #3)
> > I think Box::create also needs to be renamed for the same reason; as with
> > this function, it takes ownership of a raw pointer and so needs a special
> > name making that clear.
> 
> This create comes from Box class, something I don't think we should rename.

Why? What makes the Box class immune to naming considerations?

(In reply to Darin Adler from comment #5)
> (In reply to Xabier Rodríguez Calvar from comment #4)
> > (In reply to Darin Adler from comment #3)
> > > I think Box::create also needs to be renamed for the same reason; as with
> > > this function, it takes ownership of a raw pointer and so needs a special
> > > name making that clear.
> > 
> > This create comes from Box class, something I don't think we should rename.
> 
> Why? What makes the Box class immune to naming considerations?

Well, yes, we could, but wouldn't recommend it here since Box does create objects in place.

(In reply to Xabier Rodríguez Calvar from comment #6)
> Well, yes, we could, but wouldn't recommend it here since Box does create
> objects in place.

Doesn’t that function take ownership of a pointer? How is that "creating an object in place"?

(In reply to Darin Adler from comment #7)
> (In reply to Xabier Rodríguez Calvar from comment #6)
> > Well, yes, we could, but wouldn't recommend it here since Box does create
> > objects in place.
> 
> Doesn’t that function take ownership of a pointer? How is that "creating an
> object in place"?

Oh, OK, I guess not. My bad. I misunderstood.

Created attachment 404453 [details]
Patch

Landing this tomorrow morning CEST.

Committed r264499: <https://trac.webkit.org/changeset/264499>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404453 [details].

<rdar://problem/65707686>