Implement OfflineAudioContext constructor
Created attachment 177873 [details] Patch
Comment on attachment 177873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177873&action=review > Source/WebCore/bindings/v8/custom/V8OfflineAudioContextCustom.cpp:42 > +v8::Handle<v8::Value> V8OfflineAudioContext::constructorCallbackCustom(const v8::Arguments& args) This looks like it can be auto generated. You'll just need to do the bounds checking in WebCore and use an ec to throw an exception.
Created attachment 177905 [details] Patch
Comment on attachment 177905 [details] Patch Attachment 177905 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15152657
Comment on attachment 177905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177905&action=review > Source/WebCore/Modules/webaudio/DOMWindowWebAudio.idl:33 > [JSCustomGetter, V8EnabledAtRuntime] attribute AudioContextConstructor webkitAudioContext; > + [JSCustomGetter, V8EnabledAtRuntime] attribute OfflineAudioContextConstructor webkitOfflineAudioContext; Can't you remove JSCustomGetter by improving CodeGeneratorJS.pm?
Comment on attachment 177905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177905&action=review > Source/WebCore/bindings/js/JSOfflineAudioContextCustom.cpp:75 > + if (numberOfChannels <= 0 || numberOfChannels > 10) > + return throwVMError(exec, createSyntaxError(exec, "Invalid number of channels")); > + > + if (numberOfFrames <= 0) > + return throwVMError(exec, createSyntaxError(exec, "Invalid number of frames")); > + > + if (sampleRate <= 0) > + return throwVMError(exec, createSyntaxError(exec, "Invalid sample rate")); Should these really be JavaScript errors not DOM exceptions? (Would you tell me the spec link?) If they are DOM exceptions, you can throw them in OfflineAudioContext::create(). Then you can completely remove custom code for constructors. > Source/WebCore/bindings/v8/custom/V8OfflineAudioContextCustom.cpp:65 > + int32_t numberOfChannels = toInt32(args[0], ok); > + if (!ok || numberOfChannels <= 0 || numberOfChannels > 10) > + return throwError(v8SyntaxError, "Invalid number of channels", args.GetIsolate()); > + > + int32_t numberOfFrames = toInt32(args[1], ok); > + if (!ok || numberOfFrames <= 0) > + return throwError(v8SyntaxError, "Invalid number of frames", args.GetIsolate()); > + > + float sampleRate = toFloat(args[2]); > + if (sampleRate <= 0) > + return throwError(v8SyntaxError, "Invalid sample rate", args.GetIsolate()); Ditto.
(In reply to comment #2) > (From update of attachment 177873 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177873&action=review > > > Source/WebCore/bindings/v8/custom/V8OfflineAudioContextCustom.cpp:42 > > +v8::Handle<v8::Value> V8OfflineAudioContext::constructorCallbackCustom(const v8::Arguments& args) > > This looks like it can be auto generated. You'll just need to do the bounds checking in WebCore and use an ec to throw an exception. Adam, could you please provide some more details about *how* this can be auto-generated? I know how to do this kind of thing for methods (such as AudioContext methods), but don't understand how to have the DOMWindow *constructor* auto-generated to have the appropriate arguments which will call into OfflineAudioContext::create(...) Thanks!
(In reply to comment #7) > > This looks like it can be auto generated. You'll just need to do the bounds checking in WebCore and use an ec to throw an exception. > > Adam, could you please provide some more details about *how* this can be auto-generated? (Adam is on vacation.) You can auto-generate a constructor by a [Constructor] IDL atribute (See https://trac.webkit.org/wiki/WebKitIDL#Constructor and WebKit IDL files that are using [Constructor] (e.g. WebSocket.idl)). In your case, the tricky part is that you need to check parameter values in the constructor. As I commented above, if you can raise DOM exceptions instead of JavaScript errors, you can just move the value checks into WebCore code. Then the [Constructor] IDL attribute will generate the constructor. Otherwise (if you really need to throw JavaScript errors for the invalid values), you can improve code generators so that they can auto-generate the value check code. Would you tell me the spec please? I'd like to check whether the constructor should throw DOM exceptions or JavaScript errors.
(In reply to comment #8) > (In reply to comment #7) > > > This looks like it can be auto generated. You'll just need to do the bounds checking in WebCore and use an ec to throw an exception. > > > > Adam, could you please provide some more details about *how* this can be auto-generated? > > (Adam is on vacation.) > > You can auto-generate a constructor by a [Constructor] IDL atribute (See https://trac.webkit.org/wiki/WebKitIDL#Constructor and WebKit IDL files that are using [Constructor] (e.g. WebSocket.idl)). > > In your case, the tricky part is that you need to check parameter values in the constructor. As I commented above, if you can raise DOM exceptions instead of JavaScript errors, you can just move the value checks into WebCore code. Then the [Constructor] IDL attribute will generate the constructor. Otherwise (if you really need to throw JavaScript errors for the invalid values), you can improve code generators so that they can auto-generate the value check code. > > Would you tell me the spec please? I'd like to check whether the constructor should throw DOM exceptions or JavaScript errors. Thanks Kentaro, this can throw DOM exceptions so hopefully I can take advantage of the technique you recommend. This will be going into the spec over the weekend...
Created attachment 178662 [details] Patch
Comment on attachment 178662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178662&action=review Looks great! No custom code:) > Source/WebCore/Modules/webaudio/OfflineAudioContext.idl:28 > + JSGenerateToJSObject, Is this needed?
Comment on attachment 178662 [details] Patch Attachment 178662 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15259233 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html inspector-protocol/nmi-webaudio-leak-test.html inspector-protocol/nmi-webaudio.html
Committed r137398: <http://trac.webkit.org/changeset/137398>
Rolled out the patch in r137422 because a bunch of web audio tests hit ASSERTs() in the Debug build. crogers: would you check it? http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=webaudio%2Fbiquad-highpass.html
Reopening to attach new patch.
Created attachment 179109 [details] Patch
(In reply to comment #14) > Rolled out the patch in r137422 because a bunch of web audio tests hit ASSERTs() in the Debug build. > > crogers: would you check it? > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=webaudio%2Fbiquad-highpass.html Sorry about that Kentaro, it turns out that I needed the "EventTarget" keyword in OfflineAudioContext.idl to match AudioContext.idl This fixes the ASSERTs in the Debug build layout tests. New patch uploaded with "EventTarget"
Comment on attachment 179109 [details] Patch Thanks for the fix.
Comment on attachment 179109 [details] Patch Clearing flags on attachment: 179109 Committed r137516: <http://trac.webkit.org/changeset/137516>
All reviewed patches have been landed. Closing bug.
Note that we sometimes hit the following assertion after this patch: STDERR: ASSERTION FAILED: !m_finishedNodes.size() STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/Modules/webaudio/AudioContext.cpp(198) : virtual WebCore::AudioContext::~AudioContext() STDERR: 1 0x7f27f144c1b6 WebCore::AudioContext::~AudioContext() STDERR: 2 0x7f27f1474430 WebCore::OfflineAudioContext::~OfflineAudioContext() STDERR: 3 0x7f27f1474468 WebCore::OfflineAudioContext::~OfflineAudioContext() STDERR: 4 0x7f27f144fbcc WTF::ThreadSafeRefCounted<WebCore::AudioContext>::deref() STDERR: 5 0x7f27f24235a8 WebCore::JSAudioContext::releaseImpl() STDERR: 6 0x7f27f2443370 WebCore::JSOfflineAudioContextOwner::finalize(JSC::Handle<JSC::Unknown>, void*) STDERR: 7 0x7f27eb741ac9 JSC::WeakBlock::finalize(JSC::WeakImpl*) STDERR: 8 0x7f27eb7414bf JSC::WeakBlock::sweep() STDERR: 9 0x7f27eb7406b6 JSC::WeakSet::sweep() STDERR: 10 0x7f27eb738965 JSC::MarkedBlock::sweep(JSC::MarkedBlock::SweepMode) STDERR: 11 0x7f27eb73b2d3 JSC::Sweep::operator()(JSC::MarkedBlock*) STDERR: 12 0x7f27eb73c5f5 void JSC::MarkedAllocator::forEachBlock<JSC::Sweep>(JSC::Sweep&) STDERR: 13 0x7f27eb73c111 JSC::Sweep::ReturnType JSC::MarkedSpace::forEachBlock<JSC::Sweep>(JSC::Sweep&) STDERR: 14 0x7f27eb73b93b JSC::Sweep::ReturnType JSC::MarkedSpace::forEachBlock<JSC::Sweep>() STDERR: 15 0x7f27eb73a641 JSC::MarkedSpace::sweep() STDERR: 16 0x7f27eb729b72 JSC::Heap::collect(JSC::Heap::SweepToggle) STDERR: 17 0x7f27eb729889 JSC::Heap::collectAllGarbage() STDERR: 18 0x7f27f233feac STDERR: 19 0x7f27f233ffa2 WebCore::GCController::gcTimerFired(WebCore::Timer<WebCore::GCController>*) STDERR: 20 0x7f27f2340232 WebCore::Timer<WebCore::GCController>::fired() STDERR: 21 0x7f27f1daf1aa WebCore::ThreadTimers::sharedTimerFiredInternal() STDERR: 22 0x7f27f1daf0cb WebCore::ThreadTimers::sharedTimerFired() STDERR: 23 0x7f27f286f625 STDERR: 24 0x7f27ee7dd33e _ecore_timer_expired_call STDERR: 25 0x7f27ee7dd50b _ecore_timer_expired_timers_call STDERR: 26 0x7f27ee7da421 STDERR: 27 0x7f27ee7daab7 ecore_main_loop_begin STDERR: 28 0x7f27f286dee7 WebCore::RunLoop::run() STDERR: 29 0x7f27f6353a84 WebProcessMainEfl STDERR: 30 0x400804 main STDERR: 31 0x7f27f53a876d __libc_start_main I filed bug 105870 to track the issue.
Comment on attachment 179109 [details] Patch Thanks for removing the custom bindings code!