[webkit-reviews] review denied: [Bug 46234] Add HRTFDatabaseLoader files : [Attachment 68981] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Chris Rogers
<crogers at google.com>'s request for review:
Bug 46234: Add HRTFDatabaseLoader files
https://bugs.webkit.org/show_bug.cgi?id=46234

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68981&action=review

Looking pretty good!  Thanks for the threading ASSERT()s, they make it much
easier to reason about.

r- for nits

> WebCore/platform/audio/HRTFDatabaseLoader.cpp:65
> +    , m_databaseSampleRate(44100.0) // proper value gets set later

the constructor should just take the sample rate as a parameter since it's
always set immediately after construction

> WebCore/platform/audio/HRTFDatabaseLoader.h:60
> +    static HRTFDatabaseLoader* s_loader; // singleton

should this be public?

> WebCore/platform/audio/HRTFDatabaseLoader.h:72
> +    mutable OwnPtr<HRTFDatabase> m_hrtfDatabase;

nit: it doesn't appear mutable is needed here

> WebCore/platform/audio/HRTFDatabaseLoader.h:81
> +// defaultHRTFDatabase() gives access to the loaded database.
> +// This can be called from any thread, but it is the callers responsibilty
to call this while the context (and thus HRTFDatabaseLoader)
> +// is still alive.  Otherwise this will return 0.
> +extern HRTFDatabase* defaultHRTFDatabase();

This should be a static function on HRTFDatabaseLoader, no reason to put this
directly in the WebCore namespace.