Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2727733002:
Implement AudioWorkletProcessor interface (Closed)
|
Created:
3 years, 9 months ago by hongchan Modified:
3 years, 9 months ago CC:
chromium-reviews, shimazu+worker_chromium.org, kinuko+worker_chromium.org, Raymond Toy, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement AudioWorkletProcessor interface
This CL add the actual implementation of AudioWorkletProcessor and
AudioWorkletGlobaScope, plus a basic unit test for the AWGS.
- AudioWorkletGlobalScope::registerProcessor()
- IDL: AudioWorkletGlobalScope, AudioWorkletProcessor
- AudioWorkletProcessorDefinition class
This CL does NOT include the activation of AudioWorkletGlobalScope,
which requires comprehensive changes in AudioContext, AudioDestination
and AudioWorkletThread.
BUG=695219
TEST=AudioWorkletGlobalScopeTest.cpp
Review-Url: https://codereview.chromium.org/2727733002
Cr-Commit-Position: refs/heads/master@{#458143}
Committed: https://chromium.googlesource.com/chromium/src/+/71cd36fffa8aad850e1829ca71d9b807590a782e
Patch Set 1 : Added AudioWorkletProcessor and AudioWorkletProcessorDefinition classes #
Total comments: 23
Patch Set 2 : Addressing feedback #
Total comments: 4
Patch Set 3 : Refactored AudioWorkletGlobalScope to centralize V8 operation #
Total comments: 28
Patch Set 4 : Added AudioWorkletGlobalScopeTest.cpp #
Total comments: 31
Patch Set 5 : Added more unit test for parsing #
Total comments: 35
Patch Set 6 : Addressing feedback from rtoy@ #Messages
Total messages: 51 (23 generated)
Description was changed from ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcess() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class BUG=695219 TEST= AudioWorkletGlobalScopeTest.cpp ========== to ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcess() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. The follow-up CL will handle them. BUG=695219 TEST= AudioWorkletGlobalScopeTest.cpp ==========
hongchan@chromium.org changed reviewers: + ikilpatrick@chromium.org, nhiroki@chromium.org, rtoy@chromium.org
https://codereview.chromium.org/2727733002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp (right): https://codereview.chromium.org/2727733002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:35: bool AudioWorkletProcessor::process(/* inputs, outputs, parameters */) { What is the intent here? You'll add support for these parameters in a followup CL? https://codereview.chromium.org/2727733002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.cpp (right): https://codereview.chromium.org/2727733002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.cpp:55: // ToV8(parameters, m_scriptState->context()->Global(), isolate)}; These can't be implemented now? https://codereview.chromium.org/2727733002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.cpp:65: // instance as invalid node and throw. "throw" what? It doesn't look like you're throwing anything.
Please disregard the CL until it's ready. I pressed the "edit issue" button but it sent the CL out to all the reviewers. I will request the review when it's ready.
hongchan@chromium.org changed reviewers: - ikilpatrick@chromium.org, nhiroki@chromium.org, rtoy@chromium.org
Description was changed from ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcess() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. The follow-up CL will handle them. BUG=695219 TEST= AudioWorkletGlobalScopeTest.cpp ========== to ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. The follow-up CL will handle them. BUG=695219 TEST= AudioWorkletGlobalScopeTest.cpp ==========
Description was changed from ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. The follow-up CL will handle them. BUG=695219 TEST= AudioWorkletGlobalScopeTest.cpp ========== to ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST= AudioWorkletGlobalScopeTest.cpp ==========
Description was changed from ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST= AudioWorkletGlobalScopeTest.cpp ========== to ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST=(none) ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST=(none) ========== to ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST=(none) ==========
hongchan@chromium.org changed reviewers: + ikilpatrick@chromium.org, nhiroki@chromium.org, rtoy@chromium.org
PTAL at the general approach. I tried to follow the pattern from PaintWorklet and CSSPaintDefinition, but they run on the main thread so there might be some mistakes. Also I would like to add a unit test for this CL and I am looking at the threaded worklet's test as an example.
Looks good overall from the worker infra's pov. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:47: ExceptionState& exceptionState) { I'd recommend that threaded objects have thread checks (e.g., DCHECK(isContextThread()) as much as possible because sometimes sourcecode readers (including authors) lose the thread they're on. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:20: return nullptr; It looks strange that create() returns a nullptr. How about checking |findDefinition()| in the caller and then passing a valid |definition| to create() instead of |globalScope|? https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.cpp (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.cpp:35: v8::Local<v8::Object> AudioWorkletProcessorDefinition::createInstance() { Ditto: Can you add thread checks if possible? https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:18: // AudioWorkletGlobalScope. After the registration, a definition class contains Can you explain the thread that this class lives on? For example, "This is constructed and destroyed on a worker thread. All methods must be called on the worker thread."
nhiroki@chromium.org changed reviewers: + haraken@chromium.org
On 2017/03/03 19:02:39, hongchan wrote: > PTAL at the general approach. > > I tried to follow the pattern from PaintWorklet and CSSPaintDefinition, but they > run on the main thread so there might be some mistakes. Looks good to me, but I'm not really familiar with V8 API calls. +haraken@, can you check them? > Also I would like to add a unit test for this CL and I am looking at the > threaded worklet's test as an example. Good :) Let me know if you have any questions about the test infra on workers/worklets.
I'd like to put as many V8 related things in AudioWorkletGlobalScope as possible (and probably move AudioWorkletGlobalScope to bindings/modules/). Can we try to make AudioWorkletProcessorDefinition a very simple wrapper of m_constructor and m_process and move process() and createInstance() to AudioWorkletGlobalScope? Also would you help me understand why you adopted the constructor & process pattern? Is it a common pattern for Worklet APIs? https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:61: v8::Local<v8::Context> context = scriptController()->context(); If you pass in [ScriptState] to registerProcessor from the idl, you can just use it in this method. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:33: const ScriptValue& ctor, constructor https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.idl (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.idl:12: [RaisesException] void registerProcessor(DOMString name, Function processorCtor); I'd prefer passing [ScriptState] here. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:40: AudioWorkletProcessor(AudioWorkletProcessorDefinition*); Add explicit. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:43: v8::Local<v8::Object> m_instance; You should not use v8::Local on a heap-allocated object. v8::Local is only for on-stack objects. You'll need to use ScopedPersistent. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:34: // Perform process() call with a instance. an instance https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:48: RefPtr<ScriptState> m_scriptState; I'd like to remove this ScriptState since it should be equal to AudioWorkletGlobalScope's ScriptState.
On 2017/03/06 11:51:12, haraken wrote: > I'd like to put as many V8 related things in AudioWorkletGlobalScope as possible > (and probably move AudioWorkletGlobalScope to bindings/modules/). > I think keeping this in AudioWorkletProcessorDefinition is ok? This matches how custom elements and custom paint work today? It helps keep separation of concerns, e.g. if we have another type of class for a specific worklet, which both had the same/similar method names, it'd get confusing pretty quickly :). > Can we try to make AudioWorkletProcessorDefinition a very simple wrapper of > m_constructor and m_process and move process() and createInstance() to > AudioWorkletGlobalScope? > > Also would you help me understand why you adopted the constructor & process > pattern? Is it a common pattern for Worklet APIs? Yes this is how all of the worklet apis are shaped. They register a class (described in a definition), which the engine is responsible for constructing that class and invoking callbacks upon it when necessary. > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp > (right): > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:61: > v8::Local<v8::Context> context = scriptController()->context(); > > If you pass in [ScriptState] to registerProcessor from the idl, you can just use > it in this method. > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h > (right): > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:33: const > ScriptValue& ctor, > > constructor > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.idl > (right): > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.idl:12: > [RaisesException] void registerProcessor(DOMString name, Function > processorCtor); > > I'd prefer passing [ScriptState] here. > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h (right): > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:40: > AudioWorkletProcessor(AudioWorkletProcessorDefinition*); > > Add explicit. > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:43: > v8::Local<v8::Object> m_instance; > > You should not use v8::Local on a heap-allocated object. v8::Local is only for > on-stack objects. > > You'll need to use ScopedPersistent. > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h > (right): > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:34: > // Perform process() call with a instance. > > an instance > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:48: > RefPtr<ScriptState> m_scriptState; > > I'd like to remove this ScriptState since it should be equal to > AudioWorkletGlobalScope's ScriptState.
Thanks for the review on rough draft. Here are some notes from my point of view: - AudioWorkletProcessor is different from CSSPaintDefinition because AudioWorkletGlobalScope can have multiple instances of AudioWorkletProcessor that are created from a definition. - The classes in this CL are created and destroyed on ThreadedWorkletThread, to be precise, AudioWorkletThread class. This CL would be the first example of actual usage of ThreadedWorklet infra. (The work on AnimationWorklet has not been started yet. Right?) If PS2 looks good enough to work on, I would like to move on to the unit test. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:47: ExceptionState& exceptionState) { On 2017/03/06 10:08:44, nhiroki (slow) wrote: > I'd recommend that threaded objects have thread checks (e.g., > DCHECK(isContextThread()) as much as possible because sometimes sourcecode > readers (including authors) lose the thread they're on. Good point. I do not have the thread for this code yet, but it would be AudioWorkletThread. Is DCHECK(isContextThread()) the right way to check it? https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:61: v8::Local<v8::Context> context = scriptController()->context(); On 2017/03/06 11:51:11, haraken wrote: > > If you pass in [ScriptState] to registerProcessor from the idl, you can just use > it in this method. Done. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:33: const ScriptValue& ctor, On 2017/03/06 11:51:11, haraken wrote: > > constructor Done. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.idl (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.idl:12: [RaisesException] void registerProcessor(DOMString name, Function processorCtor); On 2017/03/06 11:51:12, haraken wrote: > > I'd prefer passing [ScriptState] here. Done. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:20: return nullptr; On 2017/03/06 10:08:44, nhiroki (slow) wrote: > It looks strange that create() returns a nullptr. How about checking > |findDefinition()| in the caller and then passing a valid |definition| to > create() instead of |globalScope|? Ah, that's right. This method does not get called by user-supplied JS code. Thus it's okay to check the definition in the caller. I was following our WebAudio's regular constructor pattern. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:40: AudioWorkletProcessor(AudioWorkletProcessorDefinition*); On 2017/03/06 11:51:12, haraken wrote: > > Add explicit. Done. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:43: v8::Local<v8::Object> m_instance; On 2017/03/06 11:51:12, haraken wrote: > > You should not use v8::Local on a heap-allocated object. v8::Local is only for > on-stack objects. > > You'll need to use ScopedPersistent. Done. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.cpp (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.cpp:35: v8::Local<v8::Object> AudioWorkletProcessorDefinition::createInstance() { On 2017/03/06 10:08:44, nhiroki (slow) wrote: > Ditto: Can you add thread checks if possible? Hmm. AudioWorkletGlobalScope and this definition only can be accessed by the worklet thread. The check requires us to pass in GlobalScope object to the definition (for isContextThread() method). Is that what we want? https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:18: // AudioWorkletGlobalScope. After the registration, a definition class contains On 2017/03/06 10:08:44, nhiroki (slow) wrote: > Can you explain the thread that this class lives on? For example, "This is > constructed and destroyed on a worker thread. All methods must be called on the > worker thread." Done. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:34: // Perform process() call with a instance. On 2017/03/06 11:51:12, haraken wrote: > > an instance Done. https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:48: RefPtr<ScriptState> m_scriptState; On 2017/03/06 11:51:12, haraken wrote: > > I'd like to remove this ScriptState since it should be equal to > AudioWorkletGlobalScope's ScriptState. ikilpatrick@ responded to this - I think the definition needs to keep the state in order to invoke constructor and process method properly.
https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h (right): https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:48: RefPtr<ScriptState> m_scriptState; On 2017/03/06 20:06:12, hongchan wrote: > On 2017/03/06 11:51:12, haraken wrote: > > > > I'd like to remove this ScriptState since it should be equal to > > AudioWorkletGlobalScope's ScriptState. > > ikilpatrick@ responded to this - I think the definition needs to keep the state > in order to invoke constructor and process method properly. So my proposal is to move process() and createInstance() to AudioWorkletGlobalScope. I'd prefer centralizing script controlling logics into AudioWorkletGlobalScope.
On 2017/03/06 20:49:44, haraken wrote: > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h > (right): > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:48: > RefPtr<ScriptState> m_scriptState; > On 2017/03/06 20:06:12, hongchan wrote: > > On 2017/03/06 11:51:12, haraken wrote: > > > > > > I'd like to remove this ScriptState since it should be equal to > > > AudioWorkletGlobalScope's ScriptState. > > > > ikilpatrick@ responded to this - I think the definition needs to keep the > state > > in order to invoke constructor and process method properly. > > So my proposal is to move process() and createInstance() to > AudioWorkletGlobalScope. I'd prefer centralizing script controlling logics into > AudioWorkletGlobalScope. So this differs from custom paint and elements?, e.g. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/csspai... https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... To be clear I'm fine with either, but previously we've kept class specific bindings separate. I'm just concerned when we add additional types of class definitions inside worklets we'll try to keep everything in the same file (when they should be separate).
On 2017/03/07 22:41:47, ikilpatrick wrote: > On 2017/03/06 20:49:44, haraken wrote: > > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > > File > > third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h > > (right): > > > > > https://codereview.chromium.org/2727733002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:48: > > RefPtr<ScriptState> m_scriptState; > > On 2017/03/06 20:06:12, hongchan wrote: > > > On 2017/03/06 11:51:12, haraken wrote: > > > > > > > > I'd like to remove this ScriptState since it should be equal to > > > > AudioWorkletGlobalScope's ScriptState. > > > > > > ikilpatrick@ responded to this - I think the definition needs to keep the > > state > > > in order to invoke constructor and process method properly. > > > > So my proposal is to move process() and createInstance() to > > AudioWorkletGlobalScope. I'd prefer centralizing script controlling logics > into > > AudioWorkletGlobalScope. > > So this differs from custom paint and elements?, e.g. > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/csspai... > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > > To be clear I'm fine with either, but previously we've kept class specific > bindings separate. I'm just concerned when we add additional types of class > definitions inside worklets we'll try to keep everything in the same file (when > they should be separate). Makes sense. Maybe can we move AudioWorkletGlobalScope and AudioWorkletProcessorDefinition to bindings/modules/? I'd prefer putting files that heavily use V8 APIs in bindings/.
The overall design looks good. https://codereview.chromium.org/2727733002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:91: if (!v8Call(prototype->Get(context, v8String(isolate, "process")), It looks a bit nasty to look up a prototype object, a function with a name "process" etc, but this is a speced behavior of the audio worklet, right? https://codereview.chromium.org/2727733002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h (right): https://codereview.chromium.org/2727733002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:56: ScopedPersistent<v8::Function> m_process; Just to confirm: You're intending to keep these objects alive until the AudioWorkletGlobalScope is disposed, right? And these ScopedPersistents are destroyed when the AudioWorkletGlobalScope is disposed. If yes, this looks fine. (In general, we should use ScopedPersistent very carefully since it strongly holds V8 objects, which sometimes causes memory leaks.)
To help the understanding of how all these related, I wrote a small design doc. PTAL: https://docs.google.com/document/d/1RHPNJ7Dz5hFebPCG9KI_wG4pb1EnKRmzzE7Io1Lkr... haraken@ > So my proposal is to move process() and createInstance() to AudioWorkletGlobalScope. I'd prefer centralizing script controlling logics into AudioWorkletGlobalScope. Yes, I refactored AWGS as you suggested and ended up with the cleaner structure in overall. Please see PS3 and let me know what you think. > Maybe can we move AudioWorkletGlobalScope and AudioWorkletProcessorDefinition to bindings/modules/? I'd prefer putting files that heavily use V8 APIs in bindings/. Sure, but can we do that after we remove the experimental flag? ikilpatrick@ > To be clear I'm fine with either, but previously we've kept class specific bindings separate. I'm just concerned when we add additional types of class definitions inside worklets we'll try to keep everything in the same file (when they should be separate). With the PS3, AudioWorkletProcessorDefinition has all the ScopePersistent constructor and process but it does not contain scriptState or controller. It only has the reference to the AudioWorkletGlobalScope. I tried to leave script operations (controller, state, isolate) out of the class and I think it's better this way. https://codereview.chromium.org/2727733002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:91: if (!v8Call(prototype->Get(context, v8String(isolate, "process")), On 2017/03/08 09:57:19, haraken wrote: > > It looks a bit nasty to look up a prototype object, a function with a name > "process" etc, but this is a speced behavior of the audio worklet, right? Yes, this is how it is specced. https://codereview.chromium.org/2727733002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h (right): https://codereview.chromium.org/2727733002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:56: ScopedPersistent<v8::Function> m_process; On 2017/03/08 09:57:19, haraken wrote: > > Just to confirm: You're intending to keep these objects alive until the > AudioWorkletGlobalScope is disposed, right? And these ScopedPersistents are > destroyed when the AudioWorkletGlobalScope is disposed. If yes, this looks fine. > > (In general, we should use ScopedPersistent very carefully since it strongly > holds V8 objects, which sometimes causes memory leaks.) Correct. This definition is owned by AWGS and it is destroyed when the global scope goes away.
Description was changed from ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST=(none) ========== to ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST=AudioWorkletTest.cpp ==========
lgtm https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:44: void AudioWorkletGlobalScope::dispose() { DCHECK(isContextThread()) ? https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:135: definition->constructorLocal(isolate)) what happens here if the constructor throws? i.e. registerProcessor('foo', class { constructor() { throw Error('tada!'); } }); this just returns a nullptr?
Mostly looks good. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:45: m_processorDefinitionMap.clear(); Should we also clear m_processorInstances ? https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:50: ScriptState* scriptState, You won't need to pass in a ScriptState because AudioWorkletGlobalScope should always work on scriptController()->getScriptState(). https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:59: "A class with name:'" + name + "' is already registered."); I want to have tests for these exceptions. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:77: if (!v8Call(constructorLocal->Get(context, v8String(isolate, "prototype")), Should we probably use GetPrototype()? https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:122: AudioWorkletProcessor* AudioWorkletGlobalScope::createInstance( Who calls this method? I couldn't find a caller in this CL. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:155: findDefinition(processor->name()); DCHECK(definition) ? https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:24: // must be called on the worker thread. Can you add DCHECK(!isMainThread) for documentation purposes? https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:21: // must be called on the worker thread. Add DCHECK(!isMainThread()).
https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h:21: class WorkerReportingProxy; unused? https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:40: // Create an instance of AudioWorkletProcessor from a registered name. s/Create/Creates/ Can you mention this function may return nullptr? https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:47: AudioWorkletProcessorDefinition* findDefinition(const String& name); Can you move this to the "private" section? https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:44: explicit AudioWorkletProcessor(AudioWorkletGlobalScope*, const String& name); Can you remove 'explicit'?
Patchset #4 (id:80001) has been deleted
PTAL at the new unit test. I am testing two things: (1) A basic assertion for AudioWorkletGlobalScope (2) A simple process() method test with AudioBuffer manipilation It might be sensible to factor test (2) out of the file, and create a unit test file for AudioWorkletProcessor. However, AudioWorkletGlobalScope is also closely involved with the process() call chain, so that's why I put the test in the AWGS test file. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h:21: class WorkerReportingProxy; On 2017/03/10 15:30:12, nhiroki (ooo until Mar 13) wrote: > unused? Done. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:44: void AudioWorkletGlobalScope::dispose() { On 2017/03/09 21:45:23, ikilpatrick wrote: > DCHECK(isContextThread()) ? Done. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:45: m_processorDefinitionMap.clear(); On 2017/03/10 13:43:22, haraken wrote: > > Should we also clear m_processorInstances ? Done. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:50: ScriptState* scriptState, On 2017/03/10 13:43:22, haraken wrote: > > You won't need to pass in a ScriptState because AudioWorkletGlobalScope should > always work on scriptController()->getScriptState(). Done. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:59: "A class with name:'" + name + "' is already registered."); On 2017/03/10 13:43:22, haraken wrote: > > I want to have tests for these exceptions. You mean in the layout test? Or unit test? https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:77: if (!v8Call(constructorLocal->Get(context, v8String(isolate, "prototype")), On 2017/03/10 13:43:21, haraken wrote: > > Should we probably use GetPrototype()? I tried, but it doesn't seem to work. Can you show me an example? v8Value protoValue = ctroLocal->GetPrototype() After this protoValue is not a valid V8 value. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:122: AudioWorkletProcessor* AudioWorkletGlobalScope::createInstance( On 2017/03/10 13:43:21, haraken wrote: > > Who calls this method? I couldn't find a caller in this CL. > The caller part is not connected yet (AudioWorkletNode + AudioWorkletMessagingProxy). Those main thread counterparts will be done in the follow-up CL. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:135: definition->constructorLocal(isolate)) On 2017/03/09 21:45:23, ikilpatrick wrote: > what happens here if the constructor throws? i.e. > > registerProcessor('foo', class { > constructor() { throw Error('tada!'); } > }); > > this just returns a nullptr? I have to think about it. Not sure if the constructor failure is being handled by the spec. The algorithm: https://webaudio.github.io/web-audio-api/#instantiation-of-AudioWorkletNode-a... ES6 constructor: http://www.ecma-international.org/ecma-262/6.0/#sec-construct https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:155: findDefinition(processor->name()); On 2017/03/10 13:43:21, haraken wrote: > > DCHECK(definition) ? Done. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:40: // Create an instance of AudioWorkletProcessor from a registered name. On 2017/03/10 15:30:13, nhiroki (ooo until Mar 13) wrote: > s/Create/Creates/ > > Can you mention this function may return nullptr? Done. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:47: AudioWorkletProcessorDefinition* findDefinition(const String& name); On 2017/03/10 15:30:13, nhiroki (slow) wrote: > Can you move this to the "private" section? I ended up using this method in the testing. If you prefer to have a test-specific method for this, I can fix this. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:24: // must be called on the worker thread. On 2017/03/10 13:43:22, haraken wrote: > > Can you add DCHECK(!isMainThread) for documentation purposes? Done. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:44: explicit AudioWorkletProcessor(AudioWorkletGlobalScope*, const String& name); On 2017/03/10 15:30:13, nhiroki (ooo until Mar 13) wrote: > Can you remove 'explicit'? Done. https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h (right): https://codereview.chromium.org/2727733002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:21: // must be called on the worker thread. On 2017/03/10 13:43:22, haraken wrote: > > Add DCHECK(!isMainThread()). Done. https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/BUILD.gn (right): https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/BUILD.gn:295: "webaudio/AudioWorkletGlobalScopeTest.cpp", I changed the test name because AudioWorkletGlobalScope is the primary test target in this CL.
LGTM! https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:65: exceptionState.throwTypeError("The empty string is not a valid name."); This rejects not only a null 'name' but also an empty 'name'. Is this spec'ed? (I'm now looking at "registerProcessor" algorithm: https://webaudio.github.io/web-audio-api/#methods-8) https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:80: prototypeValue)) Just curious: Why don't we throw an exception in this case? Is this common pattern? According to comments in V8BindingMacros.h, v8Call() is deprecated. Can we use the alternatives? (please ignore this comment if this will be replaced with GetPrototype() haraken@ suggested). https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:99: processValue)) ditto. https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h (right): https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:15: class ExceptionState; super-nit: Can you sort this in alphabetical order? https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp (right): https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:87: thread->workerBackingThread().backingThread().postTask( Why don't we call thread->postTask()? https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:98: thread->workerBackingThread().backingThread().postTask( ditto. https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:109: WaitableEvent* waitEvent) { EXPECT_TRUE(thread->isCurrentThread()); https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:149: WaitableEvent* waitEvent) { EXPECT_TRUE(thread->isCurrentThread()); https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:177: // Set the buffer length to 128. (= WebAudio's render quantum size) For readability and maintainability, how about defining this magic number as a constant somewhere? https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp (right): https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:17: DCHECK(!isMainThread()); Can we replace DCHECK(!isMainThread) with DCHECK(globalScope->isContextThread()) here and elsewhere in this class? https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h (right): https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:18: class AudioWorkletGlobalScope; We already included AWGS.h https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:49: String m_name; const? https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h (right): https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:46: String m_name; const?
LGTM https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:80: prototypeValue)) On 2017/03/16 00:21:35, nhiroki (slow) wrote: > Just curious: Why don't we throw an exception in this case? Is this common > pattern? > > According to comments in V8BindingMacros.h, v8Call() is deprecated. Can we use > the alternatives? (please ignore this comment if this will be replaced with > GetPrototype() haraken@ suggested). > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... Yeah, we need to throw some exception when Get fails. Also use Get().ToLocal() instead of v8Call. https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:121: m_processorDefinitionMap.set(name, definition); Nit: This will be an extremely corner case but you'll need to check if(m_processorDefinitionMap.contains(name)) again here, because the above Get() etc can invoke any arbitrary JavaScript, which may re-enter registerProcessor(), which may set a new definition on m_processorDefinitionMap. https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:175: return false; It's okay to ignore the exception, right?
lgtm https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h (right): https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:40: // function may return nullptr. maybe just comment when it is a nullptr...
wrt "GetPrototype()" method, it just doesn't work. The following code renders
|prototypeValue| into the invalid state:
---
v8::Local<v8::Value> prototypeValueLocal = classDefinitionLocal->GetPrototype();
---
So I ended up using Get().ToLocal() pattern.
---
v8::Local<v8::Value> prototypeValueLocal;
classDefinitionLocal->Get(context, v8String(isolate, "prototype"))
.ToLocal(&prototypeValueLocal);
---
Also Ian and I did some experiments to figure out the proper way of doing this.
The most sane solution is:
---
var class1 = function () {};
class1.prototype.process = function () {};
registerProcessor('class1', class1);
var class2 = function () {};
class2.prototype = { process: function () {} };
registerProcessor('class2', class2);
var class3 = function () {};
Object.defineProperty(class3, 'prototype', {
get: function () {
return {
process: function () {}
};
}
});
registerProcessor('class3', class3);
---
|class1| and |class2| are properly parsed/registered whereas |class3| is not.
Even if I used the deprecated method v8Call(...) |class3| still does not work. I
am not sure if class3 case should be supported or not, but it's just an
interesting observation.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp
(right):
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:65:
exceptionState.throwTypeError("The empty string is not a valid name.");
On 2017/03/16 00:21:35, nhiroki (slow) wrote:
> This rejects not only a null 'name' but also an empty 'name'. Is this spec'ed?
>
> (I'm now looking at "registerProcessor" algorithm:
> https://webaudio.github.io/web-audio-api/#methods-8)
Do you mean an empty string like ''?
JS object supports an empty string being a valid key, but I don't think we
should allow that. I will hold this change here, but will consult WG for
confirmation.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:80:
prototypeValue))
On 2017/03/16 12:07:15, haraken wrote:
> On 2017/03/16 00:21:35, nhiroki (slow) wrote:
> > Just curious: Why don't we throw an exception in this case? Is this common
> > pattern?
> >
> > According to comments in V8BindingMacros.h, v8Call() is deprecated. Can we
use
> > the alternatives? (please ignore this comment if this will be replaced with
> > GetPrototype() haraken@ suggested).
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
>
> Yeah, we need to throw some exception when Get fails.
>
> Also use Get().ToLocal() instead of v8Call.
This parsing pattern is largely based on what PaintWorkletGlobalScope does. I
will revise them as you all suggested.
The exception is happening at l.83.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h
(right):
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:15: class
ExceptionState;
On 2017/03/16 00:21:35, nhiroki (slow) wrote:
> super-nit: Can you sort this in alphabetical order?
Done.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:40: //
function may return nullptr.
On 2017/03/16 18:27:39, ikilpatrick wrote:
> maybe just comment when it is a nullptr...
Done.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp
(right):
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:87:
thread->workerBackingThread().backingThread().postTask(
On 2017/03/16 00:21:35, nhiroki (slow) wrote:
> Why don't we call thread->postTask()?
Done.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:98:
thread->workerBackingThread().backingThread().postTask(
On 2017/03/16 00:21:35, nhiroki (slow) wrote:
> ditto.
Done.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:109:
WaitableEvent* waitEvent) {
On 2017/03/16 00:21:35, nhiroki (slow) wrote:
> EXPECT_TRUE(thread->isCurrentThread());
Done.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:149:
WaitableEvent* waitEvent) {
On 2017/03/16 00:21:35, nhiroki (slow) wrote:
> EXPECT_TRUE(thread->isCurrentThread());
Done.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:177:
// Set the buffer length to 128. (= WebAudio's render quantum size)
On 2017/03/16 00:21:35, nhiroki (slow) wrote:
> For readability and maintainability, how about defining this magic number as a
> constant somewhere?
Done.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp
(right):
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:17:
DCHECK(!isMainThread());
On 2017/03/16 00:21:35, nhiroki (slow) wrote:
> Can we replace DCHECK(!isMainThread) with
DCHECK(globalScope->isContextThread())
> here and elsewhere in this class?
Done.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h (right):
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:18: class
AudioWorkletGlobalScope;
On 2017/03/16 00:21:36, nhiroki (slow) wrote:
> We already included AWGS.h
I removed the header from the inclusion.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.h:49: String
m_name;
On 2017/03/16 00:21:36, nhiroki (slow) wrote:
> const?
Done.
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h
(right):
https://codereview.chromium.org/2727733002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessorDefinition.h:46:
String m_name;
On 2017/03/16 00:21:36, nhiroki (slow) wrote:
> const?
Done.
Description was changed from ========== Implement AudioWorkletProcessor interface This CL add a place holder for AudioWorkletProcessor with the minimum functionality. It also contains changes like: - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST=AudioWorkletTest.cpp ========== to ========== Implement AudioWorkletProcessor interface This CL add the actual implementation of AudioWorkletProcessor and AudioWorkletGlobaScope, plus a basic unit test for the AWGS. - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST=AudioWorkletGlobalScopeTest.cpp ==========
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
haraken@chromium.org changed reviewers: + yukishiino@chromium.org
+yukishiino for the GetPrototype issue. See #33.
On 2017/03/17 09:17:12, haraken wrote:
> +yukishiino for the GetPrototype issue. See #33.
I don't understand exactly what are asked, but I'd say:
v8::Object::GetPrototype() corresponds to "__proto__" in JS (not exactly the
same though), and it's different from "prototype" property in general. e.g.
f.__proto__ != f.prototype in general.
----
var class3 = function () {};
Object.defineProperty(class3, 'prototype', {
get: function () {
return {
process: function () {}
};
}
});
----
This is quite strange because you're defining "prototype" property as an
accessor property, and returning a different object for every call. "prototype"
property is expected to be a data property.
If you have other questions, could you explicitly repeat it again?
On 2017/03/17 09:51:59, Yuki wrote:
> On 2017/03/17 09:17:12, haraken wrote:
> > +yukishiino for the GetPrototype issue. See #33.
>
> I don't understand exactly what are asked, but I'd say:
>
> v8::Object::GetPrototype() corresponds to "__proto__" in JS (not exactly the
> same though), and it's different from "prototype" property in general. e.g.
> f.__proto__ != f.prototype in general.
Thanks for clarification. Would be great if we could find the description in
the comment of GetPrototype() code. Then, I don't know why it is not working
in this CL?
v8::Local<v8::Value> prototypeValueLocal = classDefinitionLocal->GetPrototype();
This does not work. If I do this, the test crashes; registerProcess() method
fails,
thus asserting processor definition leads to crash.
> ----
> var class3 = function () {};
> Object.defineProperty(class3, 'prototype', {
> get: function () {
> return {
> process: function () {}
> };
> }
> });
> ----
>
> This is quite strange because you're defining "prototype" property as an
> accessor property, and returning a different object for every call.
"prototype"
> property is expected to be a data property.
This is just Ian and myself fiddling around with corner cases. Not a valid use
case, but we wanted to test what Get() or GetPrototype() can handle.
lgtm with nits https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:64: if (name.isEmpty()) { I think this isn't stated in the spec. File an issue and add a TODO here until that's resolved. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:78: // v8::Local<v8::Value> prototypeValue = constructorLocal->GetPrototype(); Clarify what you mean by "not valid" after which line. And it's hard to make out what doesn't work since there is not |prototypeValue| anywhere except in this comment. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:81: bool prototypeExtraced = Did you mean "prototypeExtracted" instead of "prototypeExtraced"? https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:86: if (prototypeValueLocal->IsNullOrUndefined()) { The spec doesn't require the distinction between null or undefined vs not an object. Is this just for better more informative error messages? (Which I think is a great thing to do.) Likewise for process below. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:122: AudioWorkletProcessorDefinition* definition = DCHECK(definition), just to be sure it was created and allocated. Or maybe throw some kind of error? https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:147: AudioWorkletProcessor* processor = AudioWorkletProcessor::create(this, name); DCHECK(processor)? https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:156: AudioBuffer* outputBuffer) { DCHECK (or CHECK) that input pointers are not null? Since this is still in the experimental stage, maybe all DCHECK's should be CHECK's. Hmm. Yeah, I would really prefer that. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:170: block.SetVerbose(true); Being unfamiliar with this v8 stuff, what does SetVerbose do? And also describe why you need a TryCatch here. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:179: if (block.HasCaught()) { Maybe replace lines 179-183 with return !block.HasCaught(); https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h (right): https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:44: bool process(AudioWorkletProcessor*, Document this. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.idl (right): https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.idl:12: [RaisesException] void registerProcessor(DOMString name, Function processorConstructor); Spec says VoidFunction. Is Function the same? https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp (right): https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:33: static const float kTestingSampleRate = 44100; Add comment that 44100 isn't really special, just a typical sample rate. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:88: void runBasicTest(WorkerThread* thread) { Describe what these tests are trying to test. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp (right): https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:11: // This static factory should be called implicitly when an instance of How is it called "implicitly"? https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:13: // This factory must not be called by user in |AudioWorkletGlobalScope|. Anyway to check that it's being called the user in AudioWorkletGlobalScope? That would be great. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:17: DCHECK(!isMainThread()); CHECK(globalScope)? https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:18: return new AudioWorkletProcessor(globalScope, name); Is it an error to return nullptr? Is it up to the callers to verify this? https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:24: : m_globalScope(globalScope), m_name(name) {} CHECK(globalScope)? I assume things aren't going to work if globalScope is null for any reason.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, ikilpatrick@chromium.org, nhiroki@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2727733002/#ps140001 (title: "Addressing feedback from rtoy@")
Thanks all for the review! https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:64: if (name.isEmpty()) { On 2017/03/17 16:45:32, Raymond Toy wrote: > I think this isn't stated in the spec. File an issue and add a TODO here until > that's resolved. Done. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:78: // v8::Local<v8::Value> prototypeValue = constructorLocal->GetPrototype(); On 2017/03/17 16:45:31, Raymond Toy wrote: > Clarify what you mean by "not valid" after which line. And it's hard to make > out what doesn't work since there is not |prototypeValue| anywhere except in > this comment. I am going to remove this line since it just does not work. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:81: bool prototypeExtraced = On 2017/03/17 16:45:31, Raymond Toy wrote: > Did you mean "prototypeExtracted" instead of "prototypeExtraced"? Typo! Done. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:86: if (prototypeValueLocal->IsNullOrUndefined()) { On 2017/03/17 16:45:31, Raymond Toy wrote: > The spec doesn't require the distinction between null or undefined vs not an > object. Is this just for better more informative error messages? (Which I think > is a great thing to do.) > > Likewise for process below. Hmm. This is a good point. The class definition must be either 'class' or 'function'. (which is typeof 'function' for both cases). If the check at l.73 passes we don't need these checks at all. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:122: AudioWorkletProcessorDefinition* definition = On 2017/03/17 16:45:31, Raymond Toy wrote: > DCHECK(definition), just to be sure it was created and allocated. > > Or maybe throw some kind of error? DCHECK is good enough here, I think. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:147: AudioWorkletProcessor* processor = AudioWorkletProcessor::create(this, name); On 2017/03/17 16:45:31, Raymond Toy wrote: > DCHECK(processor)? Done. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:156: AudioBuffer* outputBuffer) { On 2017/03/17 16:45:31, Raymond Toy wrote: > DCHECK (or CHECK) that input pointers are not null? > > Since this is still in the experimental stage, maybe all DCHECK's should be > CHECK's. Hmm. Yeah, I would really prefer that. Done. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:170: block.SetVerbose(true); On 2017/03/17 16:45:31, Raymond Toy wrote: > Being unfamiliar with this v8 stuff, what does SetVerbose do? And also describe > why you need a TryCatch here. Done. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.cpp:179: if (block.HasCaught()) { On 2017/03/17 16:45:31, Raymond Toy wrote: > Maybe replace lines 179-183 with > > return !block.HasCaught(); Done. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h (right): https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScope.h:44: bool process(AudioWorkletProcessor*, On 2017/03/17 16:45:32, Raymond Toy wrote: > Document this. Done. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp (right): https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:33: static const float kTestingSampleRate = 44100; On 2017/03/17 16:45:32, Raymond Toy wrote: > Add comment that 44100 isn't really special, just a typical sample rate. Done. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp:88: void runBasicTest(WorkerThread* thread) { On 2017/03/17 16:45:32, Raymond Toy wrote: > Describe what these tests are trying to test. Done. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp (right): https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:11: // This static factory should be called implicitly when an instance of On 2017/03/17 16:45:32, Raymond Toy wrote: > How is it called "implicitly"? I tried to explain the procedure in he design doc: https://docs.google.com/document/d/1RHPNJ7Dz5hFebPCG9KI_wG4pb1EnKRmzzE7Io1Lkr... https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:13: // This factory must not be called by user in |AudioWorkletGlobalScope|. On 2017/03/17 16:45:32, Raymond Toy wrote: > Anyway to check that it's being called the user in AudioWorkletGlobalScope? > That would be great. DCHECK(globalScope) is the best I can do here. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:17: DCHECK(!isMainThread()); On 2017/03/17 16:45:32, Raymond Toy wrote: > CHECK(globalScope)? Done. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:18: return new AudioWorkletProcessor(globalScope, name); On 2017/03/17 16:45:32, Raymond Toy wrote: > Is it an error to return nullptr? Is it up to the callers to verify this? Checks are done by AudioWorkletGlobalScope. This AudioWorkletProcessor is basically a thin wrapper for the class definition. https://codereview.chromium.org/2727733002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioWorkletProcessor.cpp:24: : m_globalScope(globalScope), m_name(name) {} On 2017/03/17 16:45:32, Raymond Toy wrote: > CHECK(globalScope)? I assume things aren't going to work if globalScope is null > for any reason. The static factory already checks |globalScope| and the constructor is private as well.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490029394557660,
"parent_rev": "973641c0ccf64c6c4c0919ed93cd3cd1f609b2e7", "commit_rev":
"16b5e3053574441e74b7cfb5901913c6d9c0d079"}
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490029394557660,
"parent_rev": "2d06e7e205ebb8cd0ea2d65c2c641daa8d967ecf", "commit_rev":
"71cd36fffa8aad850e1829ca71d9b807590a782e"}
Message was sent while issue was closed.
Description was changed from ========== Implement AudioWorkletProcessor interface This CL add the actual implementation of AudioWorkletProcessor and AudioWorkletGlobaScope, plus a basic unit test for the AWGS. - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST=AudioWorkletGlobalScopeTest.cpp ========== to ========== Implement AudioWorkletProcessor interface This CL add the actual implementation of AudioWorkletProcessor and AudioWorkletGlobaScope, plus a basic unit test for the AWGS. - AudioWorkletGlobalScope::registerProcessor() - IDL: AudioWorkletGlobalScope, AudioWorkletProcessor - AudioWorkletProcessorDefinition class This CL does NOT include the activation of AudioWorkletGlobalScope, which requires comprehensive changes in AudioContext, AudioDestination and AudioWorkletThread. BUG=695219 TEST=AudioWorkletGlobalScopeTest.cpp Review-Url: https://codereview.chromium.org/2727733002 Cr-Commit-Position: refs/heads/master@{#458143} Committed: https://chromium.googlesource.com/chromium/src/+/71cd36fffa8aad850e1829ca71d9... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/71cd36fffa8aad850e1829ca71d9... |
