Bug 185324 – Activate ARC for libwebrtc Objective C files

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Activate ARC for libwebrtc Objective C files

[youenn fablet]

Created attachment 339594 [details]
Patch

[youenn fablet]

Created attachment 339600 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/40031043>

[David Kilzer (:ddkilzer)]

Comment on attachment 339600 [details]
Patch

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

Okay, after reviewing all of this, I suggest you just back out the -fobjc-arc changes to libwebrtc.xcodeproj/project.pbxproj and simply change Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig instead:

 DYLIB_INSTALL_NAME_BASE_WK_RELOCATABLE_FRAMEWORKS_ = $(DYLIB_INSTALL_NAME_BASE);
 DYLIB_INSTALL_NAME_BASE_WK_RELOCATABLE_FRAMEWORKS_YES = @loader_path/../../../;
 
+CLANG_ENABLE_OBJC_ARC = YES;
+
 CLANG_WARN_BOOL_CONVERSION = YES;
 CLANG_WARN_ENUM_CONVERSION = YES;
 CLANG_WARN_INT_CONVERSION = YES;

Keep the rest of the changes to the source code; I don't think you'll need to change anything else to enable ARC.

> Source/ThirdParty/libwebrtc/ChangeLog:24
> +        * libwebrtc.xcodeproj/project.pbxproj:

These files also need -fobjc-arc under the Source/webrtc/sdk directory:

NSString+StdString.mm
- Fix leak in -[NSString(StdString) stringForString:(const std::string&)].
RTCUIApplicationStatusObserver.m
- The __weak modifier won't do what you think it should without it.
UIDevice+RTCDevice.mm
- Fix leak in +[UIDevice(RTCDevice) machineName].

RTCI420Buffer.mm
- Consistency for Objective-C++ files under sdk/objc/Framework/Classes/Video/ (in case a future merge introduces a real -fobjc-arc requirement).

This Objective-C++ code in sdk/WebKit doesn't need -fobjc-arc, although it should probably have -fobc-arc added in case the upstream code changes in the future:
decoder.mm
encoder.mm
encoder_vcp.mm

These files don't require -fobjc-arc, but won't hurt to enable it under Source/webrtc/rtc_base (in case future upstream changes require it):

logging_mac.mm
noop.mm
thread_darwin.mm

One of these three files in Source/webrtc/modules/audio_device/ios requires -fobjc-arc (the others don't now, but might need it with future upstream changes):
audio_device_ios.mm
- Fix leak in RTCAudioSessionDelegateAdapter when AudioDeviceIOS is deallocated.
audio_device_not_implemented_ios.mm
- Not needed.
voice_processing_audio_unit.mm
- Not needed.

[David Kilzer (:ddkilzer)]

Comment on attachment 339600 [details]
Patch

So r=me if you back out the changes to libwebrtc.xcodeproj/project.pbxproj and just change Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig to enable ARC for all Objective-C source instead.

[youenn fablet]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Comment on attachment 339600 [details]
> Patch
> 
> So r=me if you back out the changes to libwebrtc.xcodeproj/project.pbxproj
> and just change
> Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig to enable ARC
> for all Objective-C source instead.

Thanks for the review!
I will try enabling ARC for all source code.

FWIW, decoder.mm, encoder.mm and encoder_vcp.mm do not need arc, these are the legacy non ARC codecs and are no longer in upstreamed libwebrtc tree.
I will update the project to no longer compile these.

[youenn fablet]

Created attachment 339746 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 339746 [details]
Patch

r=me if this passes EWS.

[WebKit Commit Bot]

Comment on attachment 339746 [details]
Patch

Clearing flags on attachment: 339746

Committed r231459: <https://trac.webkit.org/changeset/231459>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.