Bug 227205

Summary:

Merge AudioFileReaderMac and AudioFileReaderIOS into AudioFileReaderCocoa

Product:

WebKit

Reporter:

Jean-Yves Avenard [:jya] <jean-yves.avenard>

Component:

Web Audio

Assignee:

Jean-Yves Avenard [:jya] <jean-yves.avenard>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annulen, cdumez, eric.carlson, ews-watchlist, glenn, gyuyoung.kim, jer.noble, philipj, ryuan.choi, sergio, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

227110, 227208

Attachments:

Description	Flags
Patch	none
Patch	none

Description Jean-Yves Avenard [:jya] 2021-06-20 23:41:20 PDT

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.

Comment 1 Radar WebKit Bug Importer 2021-06-20 23:42:05 PDT

<rdar://problem/79549527>

Comment 2 Jean-Yves Avenard [:jya] 2021-06-21 00:36:32 PDT

Created attachment 431832 [details]
Patch

Comment 3 Chris Dumez 2021-06-21 08:26:53 PDT

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.

Comment 4 Eric Carlson 2021-06-21 13:52:30 PDT

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.

Comment 5 Jean-Yves Avenard [:jya] 2021-06-21 15:28:29 PDT

(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

Comment 6 Chris Dumez 2021-06-21 15:29:54 PDT

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

Comment 7 Chris Dumez 2021-06-21 15:31:15 PDT

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

Comment 8 Jean-Yves Avenard [:jya] 2021-06-21 16:23:42 PDT

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

Comment 9 Jean-Yves Avenard [:jya] 2021-06-21 16:38:16 PDT

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.

Comment 10 Jean-Yves Avenard [:jya] 2021-06-21 20:50:20 PDT

Created attachment 431942 [details]
Patch

Comment 11 EWS 2021-06-21 21:22:01 PDT

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