[webkit-reviews] review granted: [Bug 44890] Add AudioContext files : [Attachment 68279] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Chris Rogers
<crogers at google.com>'s request for review:
Bug 44890: Add AudioContext files
https://bugs.webkit.org/show_bug.cgi?id=44890

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68279&action=review

We've gone through the locking semantics of this class as well as AudioNode at
length offline, and at this point I think this looks great. Marking r+; a
couple of minor issues that aren't really necessary before commit, so leave
them up to you.

> WebCore/webaudio/AudioContext.cpp:60
> +const int UndefinedThreadIdentifier = 0xffffffff;

>From looking at ThreadingPthreads.cpp and ThreadingWin.cpp, I think that 0 is
the correct value to use here. Regardless it would be good to define that
explicitly in WTF.

> WebCore/webaudio/AudioContext.cpp:114
> +    printf("%p: AudioContext::~AudioContext()\n", this);

Should use LOG_ERROR or similar.

> WebCore/webaudio/AudioContext.cpp:352
> +    m_graphOwnerThread = thisThread;

This could be placed in the "else" clause to avoid mutating this member over
and over; not a big deal.

> WebCore/webaudio/AudioContext.cpp:437
> +    ASSERT(isAudioThread());

Could use an ASSERT(isGraphOwner()) too.