[webkit-reviews] review granted: [Bug 197171] Create AVFoundationSoftLink.{h, mm} to reduce duplicate code : [Attachment 367961] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted  review:
Bug 197171: Create AVFoundationSoftLink.{h,mm} to reduce duplicate code
https://bugs.webkit.org/show_bug.cgi?id=197171

Attachment 367961: Patch

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




--- Comment #6 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 367961
  --> https://bugs.webkit.org/attachment.cgi?id=367961
Patch

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

> Source/WebCore/PAL/pal/cocoa/AVFoundationSoftLink.h:67
> +SOFT_LINK_CLASS_FOR_HEADER(PAL, AVMutableAudioMixInputParameters)

Should we sort them lexicographically?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:32568
> +				070F2DDB226DF54200D7BBA3 /* AudioSessionIOS.mm
in Sources */,

Changes not needed.

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:291
> +	       [[NSNotificationCenter defaultCenter] addObserver:protectedSelf
selector:@selector(wirelessRoutesAvailableDidChange:)
name:AVRouteDetectorMultipleRoutesDetectedDidChangeNotification
object:protectedSelf->_routeDetector.get()];

I would mention somewhere why we have
AVRouteDetectorMultipleRoutesDetectedDidChangeNotification here but we have
PAL::getAVAudioSessionClass() now

> Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:38
> +#import <pal/cocoa/AVFoundationSoftLink.h>

Add a space so that it is kept after all includes/imports?

> Source/WebCore/platform/graphics/avfoundation/MediaPlaybackTargetMac.mm:33
>  #import <wtf/SoftLinking.h>

To remove.

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreaming
AVFObjC.mm:39
> +#import <pal/cocoa/AVFoundationSoftLink.h>

Should we move this import one below with pal/spi?
Or keep pal/spi/Mac/AVFoundationSPI.h and add below
<pal/cocoa/AVFoundationSoftLink.h>

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySessio
n.mm:43
>  #import <wtf/SoftLinking.h>

To remove

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVFoundationObjC.m
m:41
>  #import <wtf/SoftLinking.h>

Needed?

>
Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm
:43
>  #import <wtf/SoftLinking.h>

Needed?

>
Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateLegacy
AVFObjC.mm:36
>  #import <wtf/SoftLinking.h>

Could be removed.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlaybackTargetPickerMac
.mm:40
> +#import <wtf/SoftLinking.h>

Not needed

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:139
>  #pragma mark - Soft Linking

Do we need this pragma?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:-2072
> -#if HAVE(AVFOUNDATION_MEDIA_SELECTION_GROUP)

If we remove all cases of this one, maybe we should update

>
Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:3
4
> +#import <pal/cocoa/AVFoundationSoftLink.h>

To put separately?

> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:34
>  #import <wtf/SoftLinking.h>

Not needed.

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:587
> +	   for (AVFrameRateRange *range in [format
videoSupportedFrameRateRanges])

AVFrameRateRange* ?
Or on the contrary AVCaptureDeviceFormat *format above?