[webkit-reviews] review denied: [Bug 67866] HRTFDatabaseLoader should not call WTF::waitForThreadCompletion() more than once : [Attachment 106926] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 67866: HRTFDatabaseLoader should not call WTF::waitForThreadCompletion()
more than once
https://bugs.webkit.org/show_bug.cgi?id=67866

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

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106926&action=review


> Source/WebCore/platform/audio/HRTFDatabaseLoader.cpp:127
> +    // WTF::waitForThreadCompletion() should not be called twice for the
same thread.

It feels odd to have the WTF:: prefix here but not in the code. It takes me a
little more time to pair the two. (I'd just leave it off in the comment.)

> Source/WebCore/platform/audio/HRTFDatabaseLoader.h:84
> +    Mutex m_threadLock;

Ideally there would be some comment like what is here:
http://trac.webkit.org/browser/trunk/Source/WebCore/loader/icon/IconDatabase.h#
L160

I think it is only guarding  m_databaseLoaderThread but in one instance in the
code, it is also guarding "m_startedLoadingDatabase = true;" which I don't
think is needed (and is confusing because that variable isn't guarded when used
elsewhere.

Why do we even have m_startedLoadingDatabase when it seems like the value of
m_databaseLoaderThread serves the same purpose?