Bug 129437 - [iOS] selectionImageForcingBlackText should return autoreleased object
Summary: [iOS] selectionImageForcingBlackText should return autoreleased object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-27 10:44 PST by Pratik Solanki
Modified: 2014-03-01 15:25 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.42 KB, patch)
2014-02-27 10:46 PST, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch (2.90 KB, patch)
2014-02-28 14:15 PST, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch (2.90 KB, patch)
2014-02-28 15:15 PST, Pratik Solanki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2014-02-27 10:44:57 PST
[WebHTMLView(WebDocumentPrivateProtocols) selectionImageForcingBlackText:] returns a retained CGImageRef on iOS. It should return an autoreleased object like we do on OS X.
Comment 1 Pratik Solanki 2014-02-27 10:45:10 PST
<rdar://problem/15810384>
Comment 2 Pratik Solanki 2014-02-27 10:46:23 PST
Created attachment 225393 [details]
Patch
Comment 3 Pratik Solanki 2014-02-27 10:48:15 PST
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.
Comment 4 Darin Adler 2014-02-27 12:21:22 PST
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.
Comment 5 Pratik Solanki 2014-02-27 22:47:55 PST
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...
Comment 6 Darin Adler 2014-02-28 07:47:33 PST
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.
Comment 7 Darin Adler 2014-02-28 07:51:09 PST
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.
Comment 8 Pratik Solanki 2014-02-28 14:11:43 PST
(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.
Comment 9 Pratik Solanki 2014-02-28 14:15:31 PST
Created attachment 225491 [details]
Patch
Comment 10 Pratik Solanki 2014-02-28 15:15:21 PST
Created attachment 225495 [details]
Patch
Comment 11 Darin Adler 2014-03-01 12:59:39 PST
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.
Comment 12 Pratik Solanki 2014-03-01 15:25:42 PST
Committed r164931: <http://trac.webkit.org/changeset/164931>