[webkit-reviews] review denied: [Bug 185324] Activate ARC for libwebrtc Objective C files : [Attachment 339600] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied youenn fablet
<youennf at gmail.com>'s request for review:
Bug 185324: Activate ARC for libwebrtc Objective C files
https://bugs.webkit.org/show_bug.cgi?id=185324

Attachment 339600: Patch

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




--- Comment #4 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 339600
  --> https://bugs.webkit.org/attachment.cgi?id=339600
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.