Make OSObjectPtr a bit more like RefPtr

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Make OSObjectPtr a bit more like RefPtr

[Sam Weinig]

Created attachment 237757 [details]
Patch

[Sam Weinig]

This bug is for addressing the feedback in https://bugs.webkit.org/show_bug.cgi?id=136613.

[Sam Weinig]

Two things I didn't do were:
- Remove the specialization for nullptr as I don't trust compilers :).
- Made it share code with RefPtr, as that would be a much bigger task.

[Darin Adler]

(In reply to comment #3)
> - Remove the specialization for nullptr as I don't trust compilers :).

But you didn’t add one for RefPtr. You trust compilers only with a class that we use extensively across the entire project, not with a class that’s far less often used?

[Darin Adler]

Comment on attachment 237757 [details]
Patch

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

> Source/WTF/wtf/OSObjectPtr.h:52
>  struct AdoptOSObject { };

Now that this is not part of the public API of OSObjectPtr, could make this struct a private member of the OSObjectPtr class. Would be tidier.

> Source/WTF/wtf/OSObjectPtr.h:122
> +    OSObjectPtr& operator=(const OSObjectPtr&& other)

Unwanted const here prevents the patch from compiling.

[Sam Weinig]

(In reply to comment #4)
> (In reply to comment #3)
> > - Remove the specialization for nullptr as I don't trust compilers :).
> 
> But you didn’t add one for RefPtr. You trust compilers only with a class that we use extensively across the entire project, not with a class that’s far less often used?

I'm writing some test code to figure out if it really is the same. When I finish that experiment, I will either add the code to RefPtr, or remove it from OSObjectPtr.

[Sam Weinig]

(In reply to comment #5)
> (From update of attachment 237757 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237757&action=review
> 
> > Source/WTF/wtf/OSObjectPtr.h:52
> >  struct AdoptOSObject { };
> 
> Now that this is not part of the public API of OSObjectPtr, could make this struct a private member of the OSObjectPtr class. Would be tidier.

Will fix.

> 
> > Source/WTF/wtf/OSObjectPtr.h:122
> > +    OSObjectPtr& operator=(const OSObjectPtr&& other)
> 
> Unwanted const here prevents the patch from compiling.

Do'h.

[Sam Weinig]

Committed r173383: <http://trac.webkit.org/changeset/173383>

[Tim Horton]

Hopeful build fix in http://trac.webkit.org/changeset/173384

[WebKit Commit Bot]

Re-opened since this is blocked by bug 136636

[Antti Koivisto]

How is this better than just having adoptOSObject() function for RetainPtr? That's what I was doing before discovering this and it seemed to work fine.

[Sam Weinig]

(In reply to comment #11)
> How is this better than just having adoptOSObject() function for RetainPtr? That's what I was doing before discovering this and it seemed to work fine.

It's not. And I will be rolling this out pretty soon.

[Antti Koivisto]

This seemed to work on all platforms:

+    template<typename T> inline RetainPtr<T> adoptSystem(T ptr)
+    {
+#if OS_OBJECT_USE_OBJC
+        return adoptNS(ptr);
+#else
+        return adoptCF(ptr);
+#endif
+    }
+

[Sam Weinig]

(In reply to comment #13)
> This seemed to work on all platforms:
> 
> +    template<typename T> inline RetainPtr<T> adoptSystem(T ptr)
> +    {
> +#if OS_OBJECT_USE_OBJC
> +        return adoptNS(ptr);
> +#else
> +        return adoptCF(ptr);
> +#endif
> +    }
> +

Actually, I don't believe this actually works.  In 32-bit, OS-objects are not objective-c objects, but also not CFObjects, so they can't be CFRetain()/CFReleased().  I think we are going to need to stick with OSObjectPtr for now.