[webkit-reviews] review denied: [Bug 44890] Add AudioContext files : [Attachment 66969] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied 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 66969: Patch
https://bugs.webkit.org/attachment.cgi?id=66969&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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?