Bug 120569 – [Mac] No need for HardAutorelease, which is same as CFBridgingRelease

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

[Mac] Refine the HardAutorelease function and rename it BridgingAutorelease

[Darin Adler]

Created attachment 210225 [details]
Patch

[Darin Adler]

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.

[WebKit Commit Bot]

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.

[Darin Adler]

I think I’ll need Anders’ help to decide whether to keep BridgingAutorelease or just start using CFBridgingRelease.

[Anders Carlsson]

(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.

[Darin Adler]

(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.

[Anders Carlsson]

(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.

[Darin Adler]

(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?

[Andy Estes]

(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.

[Darin Adler]

Party time!

[Darin Adler]

New title, new approach.

[Darin Adler]

Created attachment 210256 [details]
Patch

[Andy Estes]

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?

[WebKit Commit Bot]

Comment on attachment 210256 [details]
Patch

Clearing flags on attachment: 210256

Committed r154963: <http://trac.webkit.org/changeset/154963>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.