Bug 67866 – HRTFDatabaseLoader should not call WTF::waitForThreadCompletion() more than once

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

HRTFDatabaseLoader should not call WTF::waitForThreadCompletion() more than once

[Chris Rogers]

Created attachment 106926 [details]
Patch

[David Levin]

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?

[Chris Rogers]

Created attachment 106936 [details]
Patch

[Chris Rogers]

(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

[David Levin]

Comment on attachment 106936 [details]
Patch

Thanks!

[Chris Rogers]

Dave, thanks for the review.  I really appreciate you bearing through this problem with me.  Now hopefully my win-debug layout tests will greenify!

[WebKit Review Bot]

Comment on attachment 106936 [details]
Patch

Clearing flags on attachment: 106936

Committed r94893: <http://trac.webkit.org/changeset/94893>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.