WebKit Bugzilla
HRTFDatabaseLoader should not call WTF::waitForThreadCompletion() more than once
Created attachment 106926 [details] Patch
Comment on attachment 106926 [details] Patch 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?
Created attachment 106936 [details] Patch
(In reply to comment #2) > (From update of attachment 106926 [details]) > 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.) FIXED > > > 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 FIXED: added similar comment > > 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? Good point - removed m_startedLoadingDatabase
Comment on attachment 106936 [details] Patch Thanks!
Dave, thanks for the review. I really appreciate you bearing through this problem with me. Now hopefully my win-debug layout tests will greenify!
Comment on attachment 106936 [details] Patch Clearing flags on attachment: 106936 Committed r94893: <http://trac.webkit.org/changeset/94893>
All reviewed patches have been landed. Closing bug.