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

[bugzilla-daemon at webkit.org]

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


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66969|review?                     |review-
               Flag|                            |




--- Comment #8 from James Robinson <jamesr at chromium.org>  2010-09-08 17:44:02 PST ---
(From update of attachment 66969)
View in context: https://bugs.webkit.org/attachment.cgi?id=66969&action=prettypatch

Looking good overall.  You're right about #if ENABLE()s in a header file, not sure why I thought otherwise.

r- because of the RefPtr<Document> issue - I want to be sure we are handling that correctly since if it's subtly wrong it could be very hard to debug and fix down the road.

> WebCore/webaudio/AudioContext.cpp:51
> +#include "PlatformString.h"
Include <wtf/text/WTFString.h> to get the string class (it moved fairly recently).

> WebCore/webaudio/AudioContext.cpp:60
> +PassRefPtr<CachedAudio> AudioContext::createAudioRequest(const String &url, bool mixToMono)
Do we have to use strings for URLs?  I think this will make the security folks sad pandas.

As a general note, prefer enums to bools for parameters.  It's very difficult to tell what the 'false' in createAudioContext(..., false) means.  Something like createAudioContext(..., MixToMono/MixToStereo) or whatever the appropriate two values would be is easier to understand.

> WebCore/webaudio/AudioContext.h:119
> +    AudioContext(Document* document);
nit: don't name this parameter

> WebCore/webaudio/AudioContext.h:123
> +    // The context itself keeps a reference to all source nodes.  The source nodes, then reference all nodes they're connected to, and so on...
This comment sort of trails off.  I'm not sure what it's trying to say.

> WebCore/webaudio/AudioContext.h:149
> +    // FIXME: check if we really need to hold on to document - we may need to, so be careful!
> +    RefPtr<Document> m_document;
This makes me a little bit nervous.  What's the plan to resolve this FIXME?

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