[webkit-reviews] review denied: [Bug 34660] audio engine: add audio resources abstraction : [Attachment 48257] patch for spatialization resources abstraction

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 34660: audio engine: add audio resources abstraction
https://bugs.webkit.org/show_bug.cgi?id=34660

Attachment 48257: patch for spatialization resources abstraction
https://bugs.webkit.org/attachment.cgi?id=48257&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
I have no idea what a spatialization resource is, so please take the feedback
with a grain of salt.

> +const char* GetAudioSpatializationResourcePath();

WebKit function names begin with a lower case letter.

WebKit function names use "get" in their names only if the use out arguments.
Functions with return values are named after their return values.

> +    // First look in Resources directory for application's main bundle 
> +    NSString* resourcePath = [[NSBundle mainBundle] resourcePath];
> +
> +    NSString* spatializationPath = [resourcePath
stringByAppendingPathComponent:@"AudioSpatialization"];
> +
> +    if ([[NSFileManager defaultManager]
fileExistsAtPath:spatializationPath]) {
> +	   return [spatializationPath UTF8String];
> +    }
> +    
> +    // Impulse responses are not found in browser application's resources
directory
> +    // fallback to looking in WebCore.framework
> +
> +    NSString* webCorePath = [[NSBundle
bundleWithIdentifier:@"com.apple.WebCore"] resourcePath];
> +
> +    spatializationPath = [webCorePath
stringByAppendingPathComponent:@"AudioSpatialization"];
> +
> +    if ([[NSFileManager defaultManager]
fileExistsAtPath:spatializationPath]) {
> +	   return [spatializationPath UTF8String];
> +    }

WebKit project uses a space between NSString and *, although we are talking
about changing that guideline, because it's inconsistent with what we do for
C++ types. Please follow our current guideline.

WebKit project uses no braces around a single line if statement body.

The correct method to use for a path is fileSystemRepresentation rather than
UTF8String.

> +    return NULL; // not found anywhere

WebKit project uses 0 for this.

In general this code seems to have so little to do with audio that I think it
should be calling some shared functions that can get at resources. It shouldn’t
be doing all the bundle stuff directly