Bug 129437 – [iOS] selectionImageForcingBlackText should return autoreleased object

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Pratik Solanki]

[WebHTMLView(WebDocumentPrivateProtocols) selectionImageForcingBlackText:] returns a retained CGImageRef on iOS. It should return an autoreleased object like we do on OS X.

[Pratik Solanki]

<rdar://problem/15810384>

[Pratik Solanki]

Created attachment 225393 [details]
Patch

[Pratik Solanki]

Comment on attachment 225393 [details]
Patch

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

> Source/WebKit/mac/WebView/WebHTMLView.mm:6572
> +    return (CGImageRef)CFBridgingRelease(createDragImageForSelection(*coreFrame, forceBlackText).leakRef());

I know C-style cast is frowned upon, but static_cast didn't work for me here.

/Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6572:12: error: cannot cast from type 'id' to pointer type 'CGImageRef' (aka 'CGImage *')
    return static_cast<CGImageRef>(CFBridgingRelease(createDragImageForSelection(*coreFrame, forceBlackText).leakRef()));
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Let me know if there's something better I could do here.

[Darin Adler]

Comment on attachment 225393 [details]
Patch

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

>> Source/WebKit/mac/WebView/WebHTMLView.mm:6572
>> +    return (CGImageRef)CFBridgingRelease(createDragImageForSelection(*coreFrame, forceBlackText).leakRef());
> 
> I know C-style cast is frowned upon, but static_cast didn't work for me here.
> 
> /Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6572:12: error: cannot cast from type 'id' to pointer type 'CGImageRef' (aka 'CGImage *')
>     return static_cast<CGImageRef>(CFBridgingRelease(createDragImageForSelection(*coreFrame, forceBlackText).leakRef()));
>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
> 
> Let me know if there's something better I could do here.

I think we should prefer CFAutorelease here instead of CFBridgingRelease. CFBridgingRelease is good when we want to turn a CF object into an autoreleased NS object, but when we just want to do a CF autorelease, CFAutorelease seems a better choice.

If we did that, then the static_cast would probably work, or might not be needed at all.

[Pratik Solanki]

Comment on attachment 225393 [details]
Patch

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

>>> Source/WebKit/mac/WebView/WebHTMLView.mm:6572
>>> +    return (CGImageRef)CFBridgingRelease(createDragImageForSelection(*coreFrame, forceBlackText).leakRef());
>> 
>> I know C-style cast is frowned upon, but static_cast didn't work for me here.
>> 
>> /Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6572:12: error: cannot cast from type 'id' to pointer type 'CGImageRef' (aka 'CGImage *')
>>     return static_cast<CGImageRef>(CFBridgingRelease(createDragImageForSelection(*coreFrame, forceBlackText).leakRef()));
>>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 1 error generated.
>> 
>> Let me know if there's something better I could do here.
> 
> I think we should prefer CFAutorelease here instead of CFBridgingRelease. CFBridgingRelease is good when we want to turn a CF object into an autoreleased NS object, but when we just want to do a CF autorelease, CFAutorelease seems a better choice.
> 
> If we did that, then the static_cast would probably work, or might not be needed at all.

So I tried CFAutorelease. Since CFAutorelease will crash if passed null, I wrote the code as

    CGImageRef dragImage = createDragImageForSelection(*coreFrame, forceBlackText).leakRef();
    return dragImage ? CFAutorelease(dragImage) : nil;

clang didn't like that

/Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6573:12: error: cannot initialize return object of type 'CGImageRef' (aka 'CGImage *') with an rvalue of type 'CFTypeRef' (aka 'const void *')
    return dragImage ? CFAutorelease(dragImage) : nil;
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Next up, static_cast

/Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6573:24: error: static_cast from 'CFTypeRef' (aka 'const void *') to 'CGImageRef' (aka 'CGImage *') casts away qualifiers
    return dragImage ? static_cast<CGImageRef>(CFAutorelease(dragImage)) : nil;
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Do I need a const_cast?

    return dragImage ? static_cast<CGImageRef>(const_cast<void *>(CFAutorelease(dragImage))) : nil;

That seems ugly...

[Darin Adler]

Comment on attachment 225393 [details]
Patch

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

>>>> Source/WebKit/mac/WebView/WebHTMLView.mm:6572
>>>> +    return (CGImageRef)CFBridgingRelease(createDragImageForSelection(*coreFrame, forceBlackText).leakRef());
>>> 
>>> I know C-style cast is frowned upon, but static_cast didn't work for me here.
>>> 
>>> /Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6572:12: error: cannot cast from type 'id' to pointer type 'CGImageRef' (aka 'CGImage *')
>>>     return static_cast<CGImageRef>(CFBridgingRelease(createDragImageForSelection(*coreFrame, forceBlackText).leakRef()));
>>>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> 1 error generated.
>>> 
>>> Let me know if there's something better I could do here.
>> 
>> I think we should prefer CFAutorelease here instead of CFBridgingRelease. CFBridgingRelease is good when we want to turn a CF object into an autoreleased NS object, but when we just want to do a CF autorelease, CFAutorelease seems a better choice.
>> 
>> If we did that, then the static_cast would probably work, or might not be needed at all.
> 
> So I tried CFAutorelease. Since CFAutorelease will crash if passed null, I wrote the code as
> 
>     CGImageRef dragImage = createDragImageForSelection(*coreFrame, forceBlackText).leakRef();
>     return dragImage ? CFAutorelease(dragImage) : nil;
> 
> clang didn't like that
> 
> /Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6573:12: error: cannot initialize return object of type 'CGImageRef' (aka 'CGImage *') with an rvalue of type 'CFTypeRef' (aka 'const void *')
>     return dragImage ? CFAutorelease(dragImage) : nil;
>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
> 
> Next up, static_cast
> 
> /Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6573:24: error: static_cast from 'CFTypeRef' (aka 'const void *') to 'CGImageRef' (aka 'CGImage *') casts away qualifiers
>     return dragImage ? static_cast<CGImageRef>(CFAutorelease(dragImage)) : nil;
>                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
> 
> Do I need a const_cast?
> 
>     return dragImage ? static_cast<CGImageRef>(const_cast<void *>(CFAutorelease(dragImage))) : nil;
> 
> That seems ugly...

OK, how about this?

    CFDragImageRef dragImage = createDragImageForSelection(*coreFrame, forceBlackText).leakRef();
    if (dragImage)
        CFAutorelease(dragImage);
    return dragImage;

I suppose this is really interim code. When bug 125241 is fixed, this should be changed to:

    return createDragImageForSelection(*coreFrame, forceBlackText).autorelease();

Or whatever name we choose for the autorelease member function.

[Darin Adler]

Comment on attachment 225393 [details]
Patch

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

>>>>> Source/WebKit/mac/WebView/WebHTMLView.mm:6572
>>>>> +    return (CGImageRef)CFBridgingRelease(createDragImageForSelection(*coreFrame, forceBlackText).leakRef());
>>>> 
>>>> I know C-style cast is frowned upon, but static_cast didn't work for me here.
>>>> 
>>>> /Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6572:12: error: cannot cast from type 'id' to pointer type 'CGImageRef' (aka 'CGImage *')
>>>>     return static_cast<CGImageRef>(CFBridgingRelease(createDragImageForSelection(*coreFrame, forceBlackText).leakRef()));
>>>>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> 1 error generated.
>>>> 
>>>> Let me know if there's something better I could do here.
>>> 
>>> I think we should prefer CFAutorelease here instead of CFBridgingRelease. CFBridgingRelease is good when we want to turn a CF object into an autoreleased NS object, but when we just want to do a CF autorelease, CFAutorelease seems a better choice.
>>> 
>>> If we did that, then the static_cast would probably work, or might not be needed at all.
>> 
>> So I tried CFAutorelease. Since CFAutorelease will crash if passed null, I wrote the code as
>> 
>>     CGImageRef dragImage = createDragImageForSelection(*coreFrame, forceBlackText).leakRef();
>>     return dragImage ? CFAutorelease(dragImage) : nil;
>> 
>> clang didn't like that
>> 
>> /Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6573:12: error: cannot initialize return object of type 'CGImageRef' (aka 'CGImage *') with an rvalue of type 'CFTypeRef' (aka 'const void *')
>>     return dragImage ? CFAutorelease(dragImage) : nil;
>>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 1 error generated.
>> 
>> Next up, static_cast
>> 
>> /Volumes/Data/psolanki/sources/Safari/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:6573:24: error: static_cast from 'CFTypeRef' (aka 'const void *') to 'CGImageRef' (aka 'CGImage *') casts away qualifiers
>>     return dragImage ? static_cast<CGImageRef>(CFAutorelease(dragImage)) : nil;
>>                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 1 error generated.
>> 
>> Do I need a const_cast?
>> 
>>     return dragImage ? static_cast<CGImageRef>(const_cast<void *>(CFAutorelease(dragImage))) : nil;
>> 
>> That seems ugly...
> 
> OK, how about this?
> 
>     CFDragImageRef dragImage = createDragImageForSelection(*coreFrame, forceBlackText).leakRef();
>     if (dragImage)
>         CFAutorelease(dragImage);
>     return dragImage;
> 
> I suppose this is really interim code. When bug 125241 is fixed, this should be changed to:
> 
>     return createDragImageForSelection(*coreFrame, forceBlackText).autorelease();
> 
> Or whatever name we choose for the autorelease member function.

Or we could do this:

    CGImageRef dragImage = createDragImageForSelection(*coreFrame, forceBlackText).leakRef();
    return dragImage ? (CFImageRef)CFAutorelease(dragImage) : nil;

That is still better than CFBridgingRelease, since that function is for a different purpose, and there is a difference between CFRelease and CFBridgingRelease under ARC and we may wish to convert one day.

[Pratik Solanki]

(In reply to comment #7)
> (From update of attachment 225393 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225393&action=review

> Or we could do this:
> 
>     CGImageRef dragImage = createDragImageForSelection(*coreFrame, forceBlackText).leakRef();
>     return dragImage ? (CFImageRef)CFAutorelease(dragImage) : nil;
> 
> That is still better than CFBridgingRelease, since that function is for a different purpose, and there is a difference between CFRelease and CFBridgingRelease under ARC and we may wish to convert one day.

I think I will just go with this approach. I found another place where we had the exact same issue. New patch coming up.

[Pratik Solanki]

Created attachment 225491 [details]
Patch

[Pratik Solanki]

Created attachment 225495 [details]
Patch

[Darin Adler]

Comment on attachment 225495 [details]
Patch

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

> Source/WebCore/bindings/objc/DOM.mm:615
> +    return dragImage ? (CGImageRef)(CFAutorelease(dragImage)) : nil;

No need for these extra parentheses here.

[Pratik Solanki]

Committed r164931: <http://trac.webkit.org/changeset/164931>