Bug 227205 – Merge AudioFileReaderMac and AudioFileReaderIOS into AudioFileReaderCocoa

• Component: Web Audio
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

The files are mostly identical. Some changes/fixes appears to have been made over the years into one but not the other and vice-versa leading them to diverge slightly.

Let's deal with a unique class which will make fixing bug 227110 easier.

[Radar WebKit Bug Importer]

<rdar://problem/79549527>

[Jean-Yves Avenard [:jya]]

Created attachment 431832 [details]
Patch

[Chris Dumez]

Comment on attachment 431832 [details]
Patch

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

r=me with nits.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:103
> +    : m_data(nullptr)

These data members should use inline initialization in the header. Also please use nullptr instead of 0 for pointers.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:108
> +    RetainPtr<CFStringRef> filePathString = adoptCF(CFStringCreateWithCString(kCFAllocatorDefault, filePath, kCFStringEncodingUTF8));

auto

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:109
> +    RetainPtr<CFURLRef> urlRef = adoptCF(CFURLCreateWithFileSystemPath(kCFAllocatorDefault, filePathString.get(), kCFURLPOSIXPathStyle, false));

auto

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:118
> +    , m_audioFileID(0)

These should use inline initialization in the header

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:125
> +        m_extAudioFileRef = 0;

= nullptr;

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:143
> +    AudioFileReader* audioFileReader = static_cast<AudioFileReader*>(clientData);

auto

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:145
> +    size_t dataSize = audioFileReader->dataSize();

auto

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:146
> +    const void* data = audioFileReader->data();

auto

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.h:47
> +    explicit AudioFileReader(const void* data, size_t dataSize);

We usually don't use explicit for constructors with 2 mandatory parameters.

[Eric Carlson]

Comment on attachment 431832 [details]
Patch

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

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:56
> +#if PLATFORM(IOS_FAMILY)
> +#include <wtf/SoftLinking.h>
> +
> +SOFT_LINK_FRAMEWORK(AudioToolbox)
> +SOFT_LINK(AudioToolbox, AudioFileClose, OSStatus, (AudioFileID inAudioFile), (inAudioFile))
> +SOFT_LINK(AudioToolbox, AudioFileOpenWithCallbacks, OSStatus, (void *inClientData, AudioFile_ReadProc inReadFunc, AudioFile_WriteProc inWriteFunc, AudioFile_GetSizeProc inGetSizeFunc, AudioFile_SetSizeProc inSetSizeFunc, AudioFileTypeID inFileTypeHint, AudioFileID *outAudioFile), (inClientData, inReadFunc, inWriteFunc, inGetSizeFunc, inSetSizeFunc, inFileTypeHint, outAudioFile))
> +SOFT_LINK(AudioToolbox, ExtAudioFileDispose, OSStatus, (ExtAudioFileRef inExtAudioFile), (inExtAudioFile))
> +SOFT_LINK(AudioToolbox, ExtAudioFileGetProperty, OSStatus, (ExtAudioFileRef inExtAudioFile, ExtAudioFilePropertyID inPropertyID, UInt32 *ioPropertyDataSize, void *outPropertyData), (inExtAudioFile, inPropertyID, ioPropertyDataSize, outPropertyData))
> +SOFT_LINK(AudioToolbox, ExtAudioFileRead, OSStatus, (ExtAudioFileRef inExtAudioFile, UInt32 *ioNumberFrames, AudioBufferList *ioData), (inExtAudioFile, ioNumberFrames, ioData))
> +SOFT_LINK(AudioToolbox, ExtAudioFileSetProperty, OSStatus, (ExtAudioFileRef inExtAudioFile, ExtAudioFilePropertyID inPropertyID, UInt32 inPropertyDataSize, const void *inPropertyData), (inExtAudioFile, inPropertyID, inPropertyDataSize, inPropertyData))
> +SOFT_LINK(AudioToolbox, ExtAudioFileWrapAudioFileID, OSStatus, (AudioFileID inFileID, Boolean inForWriting, ExtAudioFileRef *outExtAudioFile), (inFileID, inForWriting, outExtAudioFile))
> +SOFT_LINK(AudioToolbox, ExtAudioFileOpenURL, OSStatus, (CFURLRef inURL, ExtAudioFileRef* outExtAudioFile), (inURL, outExtAudioFile))
> +#endif

It is fine to do in a separate patch, but this should be in a (new) AudioToolboxSoftLink.cpp|.h.

[Jean-Yves Avenard [:jya]]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 431832 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431832&action=review
> 
> r=me with nits.
> 
> > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:103
> > +    : m_data(nullptr)
> 
> These data members should use inline initialization in the header. Also
> please use nullptr instead of 0 for pointers.
> 
> > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:108
> > +    RetainPtr<CFStringRef> filePathString = adoptCF(CFStringCreateWithCString(kCFAllocatorDefault, filePath, kCFStringEncodingUTF8));
> 
> auto
> 
> > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:109
> > +    RetainPtr<CFURLRef> urlRef = adoptCF(CFURLCreateWithFileSystemPath(kCFAllocatorDefault, filePathString.get(), kCFURLPOSIXPathStyle, false));
> 
> auto
> 
> > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:118
> > +    , m_audioFileID(0)
> 
> These should use inline initialization in the header
> 
> > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:125
> > +        m_extAudioFileRef = 0;
> 
> = nullptr;
> 
> > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:143
> > +    AudioFileReader* audioFileReader = static_cast<AudioFileReader*>(clientData);
> 
> auto
> 
> > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:145
> > +    size_t dataSize = audioFileReader->dataSize();
> 
> auto
> 
> > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:146
> > +    const void* data = audioFileReader->data();
> 
> auto
> 
> > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.h:47
> > +    explicit AudioFileReader(const void* data, size_t dataSize);
> 
> We usually don't use explicit for constructors with 2 mandatory parameters.

All this code is existing and a direct copy/paste of what is already there. 
It seems out of scope to me to not just merge but also modify the resulting code.
Doing so is error prone and make for harder reviews. 
IMHO it should always be avoided

[Chris Dumez]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 431832 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=431832&action=review
> > 
> > r=me with nits.
> > 
> > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:103
> > > +    : m_data(nullptr)
> > 
> > These data members should use inline initialization in the header. Also
> > please use nullptr instead of 0 for pointers.
> > 
> > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:108
> > > +    RetainPtr<CFStringRef> filePathString = adoptCF(CFStringCreateWithCString(kCFAllocatorDefault, filePath, kCFStringEncodingUTF8));
> > 
> > auto
> > 
> > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:109
> > > +    RetainPtr<CFURLRef> urlRef = adoptCF(CFURLCreateWithFileSystemPath(kCFAllocatorDefault, filePathString.get(), kCFURLPOSIXPathStyle, false));
> > 
> > auto
> > 
> > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:118
> > > +    , m_audioFileID(0)
> > 
> > These should use inline initialization in the header
> > 
> > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:125
> > > +        m_extAudioFileRef = 0;
> > 
> > = nullptr;
> > 
> > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:143
> > > +    AudioFileReader* audioFileReader = static_cast<AudioFileReader*>(clientData);
> > 
> > auto
> > 
> > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:145
> > > +    size_t dataSize = audioFileReader->dataSize();
> > 
> > auto
> > 
> > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:146
> > > +    const void* data = audioFileReader->data();
> > 
> > auto
> > 
> > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.h:47
> > > +    explicit AudioFileReader(const void* data, size_t dataSize);
> > 
> > We usually don't use explicit for constructors with 2 mandatory parameters.
> 
> All this code is existing and a direct copy/paste of what is already there. 
> It seems out of scope to me to not just merge but also modify the resulting
> code.
> Doing so is error prone and make for harder reviews. 
> IMHO it should always be avoided

I already reviewed it so what's the hold up? Cleaning up old code is important too and this is a good opportunity to do so.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Jean-Yves Avenard [:jya] from comment #5)
> > (In reply to Chris Dumez from comment #3)
> > > Comment on attachment 431832 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=431832&action=review
> > > 
> > > r=me with nits.
> > > 
> > > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:103
> > > > +    : m_data(nullptr)
> > > 
> > > These data members should use inline initialization in the header. Also
> > > please use nullptr instead of 0 for pointers.
> > > 
> > > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:108
> > > > +    RetainPtr<CFStringRef> filePathString = adoptCF(CFStringCreateWithCString(kCFAllocatorDefault, filePath, kCFStringEncodingUTF8));
> > > 
> > > auto
> > > 
> > > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:109
> > > > +    RetainPtr<CFURLRef> urlRef = adoptCF(CFURLCreateWithFileSystemPath(kCFAllocatorDefault, filePathString.get(), kCFURLPOSIXPathStyle, false));
> > > 
> > > auto
> > > 
> > > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:118
> > > > +    , m_audioFileID(0)
> > > 
> > > These should use inline initialization in the header
> > > 
> > > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:125
> > > > +        m_extAudioFileRef = 0;
> > > 
> > > = nullptr;
> > > 
> > > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:143
> > > > +    AudioFileReader* audioFileReader = static_cast<AudioFileReader*>(clientData);
> > > 
> > > auto
> > > 
> > > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:145
> > > > +    size_t dataSize = audioFileReader->dataSize();
> > > 
> > > auto
> > > 
> > > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:146
> > > > +    const void* data = audioFileReader->data();
> > > 
> > > auto
> > > 
> > > > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.h:47
> > > > +    explicit AudioFileReader(const void* data, size_t dataSize);
> > > 
> > > We usually don't use explicit for constructors with 2 mandatory parameters.
> > 
> > All this code is existing and a direct copy/paste of what is already there. 
> > It seems out of scope to me to not just merge but also modify the resulting
> > code.
> > Doing so is error prone and make for harder reviews. 
> > IMHO it should always be avoided
> 
> I already reviewed it so what's the hold up? Cleaning up old code is
> important too and this is a good opportunity to do so.

I'll also note that the 2 files getting merged had already diverged so it's not a straight move anyway.

[Jean-Yves Avenard [:jya]]

(In reply to Chris Dumez from comment #7)
> > I already reviewed it so what's the hold up? Cleaning up old code is
> > important too and this is a good opportunity to do so.

Oh, I agreed it should be cleaned up. But I believe it should be done so in a follow-up change.

> I'll also note that the 2 files getting merged had already diverged so it's
> not a straight move anyway.

How they diverge is actually a good example on why I believe what I mentioned above is good practice.

When the AudioFileReaderIOS file was created, in the Changelog you read (bug 126654):
```* platform/audio/ios/AudioFileReaderIOS.cpp: Copied from Source/WebCore/platform/audio/mac/AudioFileReaderMac.cpp.```

But it's not, on-the-fly fixes had already been applied. So when comparing the two files you now have to guess on what was done and when to determine if the changes are important or not as you can't rely on the changelog and comments.

[Jean-Yves Avenard [:jya]]

Just to be clear, I do believe that the changes you mentioned absolutely need to go in. I just think they should go in a follow-up change.

However, considering the triviality of the changes, I will do them here.

[Jean-Yves Avenard [:jya]]

Created attachment 431942 [details]
Patch

[EWS]

Committed r279102 (239019@main): <https://commits.webkit.org/239019@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431942 [details].