Create a class to wrap a CA audio stream description.
Created attachment 300219 [details] Proposed patch
Comment on attachment 300219 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=300219&action=review > Source/WebCore/platform/audio/AudioStreamDescription.h:39 > + } description; Can we use Variant<std::nullptr_t, const AudioStreamBasicDescription*>? > Source/WebCore/platform/audio/AudioStreamDescription.h:49 > + Unknown, Do we need Unknown and isPCM? > Source/WebCore/platform/audio/mac/CAAudioStreamDescription.cpp:115 > + return Unknown; Why do we return Unknown here? Shouldn't we return m_format? > Source/WebCore/platform/audio/mac/CAAudioStreamDescription.cpp:125 > + return m_format; Would it be clearer if we were returning early whenever we know the format. Then at the end, we would return Unknown I guess? > Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:36 > +class CAAudioStreamDescription : public AudioStreamDescription { Can it be final? > Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:41 > + const PlatformDescription& platformDescription() const final; Make all of these private? > Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:47 > + bool isPCM() const final { return m_streamDescription.mFormatID == kAudioFormatLinearPCM; } Do we need to override this one? > Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:64 > + bool operator!=(const AudioStreamBasicDescription& other) { return !operator==(other); } space around ==? > Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:66 > + bool operator!=(const AudioStreamDescription& other) { return !operator==(other); } Ditto? > Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:78 > +CAAudioStreamDescription toCAAudioStreamDescription(const AudioStreamDescription&); Could be inlined maybe?
Comment on attachment 300219 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=300219&action=review >> Source/WebCore/platform/audio/AudioStreamDescription.h:39 >> + } description; > > Can we use Variant<std::nullptr_t, const AudioStreamBasicDescription*>? Yes, done. >> Source/WebCore/platform/audio/AudioStreamDescription.h:49 >> + Unknown, > > Do we need Unknown and isPCM? "None" is probably better than "Unknown" >> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.cpp:115 >> + return Unknown; > > Why do we return Unknown here? Shouldn't we return m_format? Oops! >> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.cpp:125 >> + return m_format; > > Would it be clearer if we were returning early whenever we know the format. > Then at the end, we would return Unknown I guess? OK, fixed. >> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:36 >> +class CAAudioStreamDescription : public AudioStreamDescription { > > Can it be final? Yes. >> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:41 >> + const PlatformDescription& platformDescription() const final; > > Make all of these private? No, because we will use this class directly in a number of places. >> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:47 >> + bool isPCM() const final { return m_streamDescription.mFormatID == kAudioFormatLinearPCM; } > > Do we need to override this one? No, fixed. >> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:64 >> + bool operator!=(const AudioStreamBasicDescription& other) { return !operator==(other); } > > space around ==? Fixed. >> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:66 >> + bool operator!=(const AudioStreamDescription& other) { return !operator==(other); } > > Ditto? Fixed. >> Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h:78 >> +CAAudioStreamDescription toCAAudioStreamDescription(const AudioStreamDescription&); > > Could be inlined maybe? Good idea, fixed.
Created attachment 300226 [details] Patch for landing.
Comment on attachment 300226 [details] Patch for landing. Clearing flags on attachment: 300226 Committed r211437: <http://trac.webkit.org/changeset/211437>