[Mac] Refine the HardAutorelease function and rename it BridgingAutorelease
Created attachment 210225 [details] Patch
I wonder if we always have CFBridgingRelease on all Mac and iOS platforms and build environments we support. If so, then maybe we should just replacing HardRelease/BridgingRelease with CFBridgingRelease, adding null checks as needed and not worrying too much about the extra overhead for non-GC.
Attachment 210225 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSValue.mm', u'Source/JavaScriptCore/ChangeLog', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/ObjcRuntimeExtras.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', u'Source/WebCore/platform/mac/KURLMac.mm', u'Source/WebCore/platform/mac/WebCoreNSURLExtras.mm', u'Source/WebCore/platform/text/mac/StringImplMac.mm', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/Misc/WebNSFileManagerExtras.mm', u'Source/WebKit/mac/Misc/WebNSURLExtras.mm', u'Source/WebKit/mac/Plugins/WebBasePluginPackage.mm', u'Source/WebKit/mac/WebView/WebPDFRepresentation.mm', u'Source/WebKit/mac/WebView/WebView.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Platform/mac/StringUtilities.mm', u'Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm', u'Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm', u'Tools/ChangeLog', u'Tools/DumpRenderTree/mac/DumpRenderTree.mm']" exit_code: 1 Source/WTF/wtf/ObjcRuntimeExtras.h:68: Extra space before [ [whitespace/braces] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
I think I’ll need Anders’ help to decide whether to keep BridgingAutorelease or just start using CFBridgingRelease.
(In reply to comment #2) > I wonder if we always have CFBridgingRelease on all Mac and iOS platforms and build environments we support. If so, then maybe we should just replacing HardRelease/BridgingRelease with CFBridgingRelease, adding null checks as needed and not worrying too much about the extra overhead for non-GC. Where would we have to add null checks? (In reply to comment #4) > I think I’ll need Anders’ help to decide whether to keep BridgingAutorelease or just start using CFBridgingRelease. I think we should just use CFBridgingRelease.
(In reply to comment #5) > (In reply to comment #2) > > I wonder if we always have CFBridgingRelease on all Mac and iOS platforms and build environments we support. If so, then maybe we should just replacing HardRelease/BridgingRelease with CFBridgingRelease, adding null checks as needed and not worrying too much about the extra overhead for non-GC. > > Where would we have to add null checks? I don’t know. That’s the problem. It requires studying each call site. If any of the call sites are passing in something that can be null we’d need a null check. I don’t know, for example, which create functions can fail and return null and which ones can’t. > (In reply to comment #4) > > I think I’ll need Anders’ help to decide whether to keep BridgingAutorelease or just start using CFBridgingRelease. > > I think we should just use CFBridgingRelease. The three issues are: 1) Annoying to pay for CFMakeCollectable on non-GC platforms like iOS. 2) Not sure CFBridgingRelease is present on all Mac and iOS platforms we compile WebKit for. 3) Not sure where to add null checks. I’ll start putting up patches that use CFBridgingRelease and I won’t rename HardAutorelease -- I’ll delete it instead.
(In reply to comment #6) > The three issues are: > > 1) Annoying to pay for CFMakeCollectable on non-GC platforms like iOS. > 2) Not sure CFBridgingRelease is present on all Mac and iOS platforms we compile WebKit for. It is present on Lion and newer AFIK. > 3) Not sure where to add null checks. I still don't understand where we'd need null checks now where we wouldn't before. It seems like both CFBridgingRetain and CFBridgingRelease handle null just fine.
(In reply to comment #7) > It seems like both CFBridgingRetain and CFBridgingRelease handle null just fine. The non-ARC version of CFBridgingRelease calls CFMakeCollectable unconditionally. I believe that won’t work on a null pointer. Am I wrong?
(In reply to comment #8) > (In reply to comment #7) > > It seems like both CFBridgingRetain and CFBridgingRelease handle null just fine. > > The non-ARC version of CFBridgingRelease calls CFMakeCollectable unconditionally. I believe that won’t work on a null pointer. Am I wrong? Despite what our documentation says about CFMakeCollectable(), the current implementation of that function handles NULL correctly. I don't know if that's true on all the platforms we need to support, though.
Party time!
New title, new approach.
Created attachment 210256 [details] Patch
Comment on attachment 210256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210256&action=review > Source/JavaScriptCore/ChangeLog:3 > + [Mac] No need for HardAutorelease, which is same as CFBridgingRelease which is *the* same?
Comment on attachment 210256 [details] Patch Clearing flags on attachment: 210256 Committed r154963: <http://trac.webkit.org/changeset/154963>
All reviewed patches have been landed. Closing bug.