| Summary: | [iOS] selectionImageForcingBlackText should return autoreleased object | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pratik Solanki <psolanki> | ||||||||
| Component: | WebKit Misc. | Assignee: | Pratik Solanki <psolanki> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | andersca, darin, mitz, psolanki, simon.fraser | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Pratik Solanki
2014-02-27 10:44:57 PST
Created attachment 225393 [details]
Patch
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 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 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 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 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. (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. Created attachment 225491 [details]
Patch
Created attachment 225495 [details]
Patch
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. Committed r164931: <http://trac.webkit.org/changeset/164931> |