Bug 67866 - HRTFDatabaseLoader should not call WTF::waitForThreadCompletion() more than once
Summary: HRTFDatabaseLoader should not call WTF::waitForThreadCompletion() more than once
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Rogers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-09 14:46 PDT by Chris Rogers
Modified: 2011-09-09 17:58 PDT (History)
2 users (show)

See Also:

Attachments
Patch (3.80 KB, patch)
2011-09-09 14:50 PDT, Chris Rogers 		no flags Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2011-09-09 16:26 PDT, Chris Rogers 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2011-09-09 14:46:44 PDT 
HRTFDatabaseLoader should not call WTF::waitForThreadCompletion() more than once
Comment 1 Chris Rogers 2011-09-09 14:50:14 PDT 
Created attachment 106926 [details]
Patch
Comment 2 David Levin 2011-09-09 15:21:33 PDT 
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?
Comment 3 Chris Rogers 2011-09-09 16:26:43 PDT 
Created attachment 106936 [details]
Patch
Comment 4 Chris Rogers 2011-09-09 16:27:57 PDT 
(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 5 David Levin 2011-09-09 16:29:44 PDT 
Comment on attachment 106936 [details]
Patch

Thanks!
Comment 6 Chris Rogers 2011-09-09 16:32:08 PDT 
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 7 WebKit Review Bot 2011-09-09 17:58:50 PDT 
Comment on attachment 106936 [details]
Patch

Clearing flags on attachment: 106936

Committed r94893: <http://trac.webkit.org/changeset/94893>
Comment 8 WebKit Review Bot 2011-09-09 17:58:54 PDT 
All reviewed patches have been landed.  Closing bug.