[Webkit-unassigned] [Bug 44890] Add AudioContext files

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=44890


Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68279|review?                     |review+
               Flag|                            |




--- Comment #15 from Kenneth Russell <kbr at google.com>  2010-09-22 17:51:06 PST ---
(From update of attachment 68279)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.