[webkit-reviews] review granted: [Bug 109755] Add support for using ARM FFT in WebAudio : [Attachment 191793] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Raymond Toy <rtoy at chromium.org>'s
request for review:
Bug 109755: Add support for using ARM FFT in WebAudio
https://bugs.webkit.org/show_bug.cgi?id=109755

Attachment 191793: Patch
https://bugs.webkit.org/attachment.cgi?id=191793&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191793&action=review


Please get a green cr-android before landing this patch (i.e., land the
chromium side change, roll WebKit DEPS, reupload the patch).

> Source/WebCore/WebCore.gyp/WebCore.gyp:1941
> +	       ['include', 'platform/audio/android/FFTFrameOpenMAXDL\\.cpp$'],

Nit: It's probably a bit better to name this file
platform/audio/chromium/FFTFrameOpenMAXDLAndroid.cpp.  It looks like we don't
have any other android subdirectories (since they are part of the chromium
port) and we have a filename regex rule that already handles files that end in
Android.cpp.  Which means you don't need this line if the filename ends in
Android.cpp.

> Source/WebKit/chromium/features.gypi:229
> +	 ['OS!="mac"', {
> +	   'conditions' : [
> +	     ['OS=="android"', {

Nit: The nesting here is confusing.  I would keep the OS!="android" and
OS!="mac" section and add a section below OS=="android" that is OS=="android"
and use_openmax_dl_fft!=0 for setting WTF_USE_WEBAUDIO_OPENMAX_DL_FFT=1.