Bug 81049 – Allow AudioContext::create() to emit an ExceptionCode.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Allow AudioContext::create() to emit an ExceptionCode.

[Jer Noble]

Created attachment 131737 [details]
Patch

[Jer Noble]

Created attachment 131739 [details]
Patch

Remove extraneous includes.

[WebKit Review Bot]

Comment on attachment 131739 [details]
Patch

Attachment 131739 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11944937

[Chris Rogers]

looks like cr-linux breakage

[Jer Noble]

(In reply to comment #4)
> looks like cr-linux breakage

Yep.  I'll need to edit the V8 custom AudioContext code.

[Hajime Morrita]

Comment on attachment 131739 [details]
Patch

r- for clearing the pending review queue.

[Jer Noble]

Created attachment 132219 [details]
Patch

[Hajime Morrita]

Comment on attachment 132219 [details]
Patch

Looks sane. Please ensure bots get green before landing.

[Jer Noble]

(In reply to comment #8)
> (From update of attachment 132219 [details])
> Looks sane. Please ensure bots get green before landing.

Sure thing.  Thanks!

[Kentaro Hara]

Comment on attachment 132219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132219&action=review

> Source/WebCore/bindings/js/JSAudioContextCustom.cpp:75
> +            return throwVMError(exec, createSyntaxError(exec, "Error creating AudioContext"));

Is it really intended (i.e. speced) to throw SYNTAX_ERROR when AudioContext::create() throws exception? Auto-generated existing JSC constructors return JSValue::encode(JSValue()) in such cases.

[Kentaro Hara]

Comment on attachment 132219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132219&action=review

> Source/WebCore/bindings/v8/custom/V8AudioContextCustom.cpp:67
> +            return throwError(ec);

On the other hand, this behavior is aligning with auto-generated existing V8 constructors. In other words, at present, auto-generated JSC constructors return undefined when create() throws exception, and auto-generated V8 constructors return TYPE_ERROR when create() throws exception. We need to fix these behaviors according to the spec anyway (in another bug).

[Hajime Morrita]

Comment on attachment 132219 [details]
Patch

https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html
The spec says nothing about the constructor. By the way, If the custom-constructor is replaced by auto-generated code using optional or overload parameter, it would be ideal.

[Kentaro Hara]

(In reply to comment #12)
> By the way, If the custom-constructor is replaced by auto-generated code using optional or overload parameter, it would be ideal.

Yeah, I tried it before but gave up because the constructor needs to switch ::create() and ::createOfflineContext(), which is a "special" logic to AudioContext.

[Kentaro Hara]

(In reply to comment #11)
> (From update of attachment 132219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132219&action=review
> 
> > Source/WebCore/bindings/v8/custom/V8AudioContextCustom.cpp:67
> > +            return throwError(ec);
> 
> On the other hand, this behavior is aligning with auto-generated existing V8 constructors. In other words, at present, auto-generated JSC constructors return undefined when create() throws exception, and auto-generated V8 constructors return TYPE_ERROR when create() throws exception. We need to fix these behaviors according to the spec anyway (in another bug).

Sorry, I posted confusing comments. Please ignore my previous comments;-)

Correction: 

    - auto-generated JSC constructors:
        if (ec) {
            setDOMException(exec, ec);
            return JSValue::encode(JSValue());
        }

    - auto-generated V8 constructors:
        if (ec) {
            return throwError(ec);
        }

The above two both throws an exception indicated by 'ec', which makes sense.

So I would recommend that you align the behavior of the AudioContext constructor with those existing constructors, unless you have a reason to make it behave differently from others.

[Jer Noble]

Created attachment 132304 [details]
Patch

Makes sense.  Updating patch with suggestions above.

[Kentaro Hara]

Comment on attachment 132304 [details]
Patch

OK. Thanks for the patch!

[Jer Noble]

Committed r111022: <http://trac.webkit.org/changeset/111022>