Bug 150030

Created attachment 262895 [details] Patch

Comment on attachment 262895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262895&action=review Looks pretty good, but I'm going to suggest a few tweaks. > Source/WebCore/platform/audio/Biquad.cpp:40 > -#if OS(DARWIN) > +#if USING_ACCELERATE Why USING instead of USE_, which is more common in WebKit and would allow the use of the USE(...) macro? > Source/WebCore/platform/audio/Biquad.h:40 > +#if OS(DARWIN) && !PLATFORM(GTK) > +#define USING_ACCELERATE 1 > +#else > +#define USING_ACCELERATE 0 > +#endif I think it might be worth calling this USE_ACCELERATE_FRAMEWORK since ACCELERATE is such a general term. > Source/WebCore/platform/audio/DirectConvolver.cpp:77 > -#if OS(DARWIN) > +#if OS(DARWIN) && !PLATFORM(GTK) > #if defined(__ppc__) || defined(__i386__) Why don't these apply to GTK+? The ChangeLog does not specify. Would USE(ACCELERATE_FRAMEWORK) make sense here as well? > Source/WebCore/platform/audio/FFTFrame.h:41 > +#elif OS(DARWIN) > +#include <Accelerate/Accelerate.h> > #endif // USE(WEBAUDIO_GSTREAMER) I think it might make sense to pull this out into its own block, simply because it's a little confusing to mix it with the GStreamer ones.

Comment on attachment 262895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262895&action=review >> Source/WebCore/platform/audio/DirectConvolver.cpp:77 >> #if defined(__ppc__) || defined(__i386__) > > Why don't these apply to GTK+? The ChangeLog does not specify. Would USE(ACCELERATE_FRAMEWORK) make sense here as well? The vDSP_conv function is part of the Accelerate.framework AFAICS. So yeah I'll update the ifdef accordingly :)

Created attachment 263684 [details] Patch

Comment on attachment 263684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263684&action=review > Source/WTF/wtf/Platform.h:1136 > +/* Disable the Accelerate.framework on Darwin (and Linux obviously) flavors of the GTK and EFL ports. */ > +#if (OS(DARWIN) || OS(LINUX)) && (PLATFORM(GTK) || PLATFORM(EFL)) > +#define USE_ACCELERATE_FRAMEWORK 0 > +#else > +#define USE_ACCELERATE_FRAMEWORK 1 > +#endif This conditional seems wrong. I wouldn’t want USE_ACCELERATE_FRAMEWORK to be 1 on Windows, for example. I think we want: #if OS(DARWIN) && !(PLATFORM(GTK) || PLATFORM(EFL)) #define USE_ACCELERATE_FRAMEWORK 1 #else #define USE_ACCELERATE_FRAMEWORK 0 #endif But maybe that’s not quite right either. > Source/WebCore/platform/audio/DirectConvolver.cpp:355 > +#endif // OS(DARWIN) && !PLATFORM(GTK) Not right. Please fix. > Source/WebCore/platform/audio/FFTFrame.h:43 > +#if USE(ACCELERATE_FRAMEWORK) > +#include <Accelerate/Accelerate.h> > +#endif // USE(ACCELERATE_FRAMEWORK) The comment on the #endif here doesn’t seem helpful. I think we should not do these when they are so close to the #if. Just makes things messier and less clear. Probably should mention this in coding style guidelines.

Comment on attachment 263684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263684&action=review > Source/WTF/wtf/Platform.h:1131 > +/* Disable the Accelerate.framework on Darwin (and Linux obviously) flavors of the GTK and EFL ports. */ Note, I have something similar on #144560 at https://bugs.webkit.org/attachment.cgi?id=255295&action=diff#a/Source/WTF/wtf/Platform.h_sec1 You might not have to add USE_ACCELERATE_FRAMEWORK, it looks like USE_ACCELERATE already exists.

Created attachment 263830 [details] Patch

Comment on attachment 263830 [details] Patch Clearing flags on attachment: 263830 Committed r191844: <http://trac.webkit.org/changeset/191844>

All reviewed patches have been landed. Closing bug.