[webkit-reviews] review denied: [Bug 105058] Implement WebIDL-style string constants in WebAudio : [Attachment 180028] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Chris Rogers
<crogers at google.com>'s request for review:
Bug 105058: Implement WebIDL-style string constants in WebAudio
https://bugs.webkit.org/show_bug.cgi?id=105058

Attachment 180028: Patch
https://bugs.webkit.org/attachment.cgi?id=180028&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=180028&action=review


> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:116
> +    else
> +	   ec = NOT_SUPPORTED_ERR;

According to the spec (http://www.w3.org/TR/WebIDL/#idl-enums), this error
should be caught in the binding layer and should throw a TypeError. However, we
cannot do it because we're not yet supporting 'enum' in the WebKit IDL. (My
co-worker is implementing it.) For the time being, could you use
context->addConsoleMessage(...) just like XMLHttpRequest::responseType() does?
We'll fix the behavior once we implement 'enum'.

> Source/WebCore/Modules/webaudio/OscillatorNode.h:65
> +    void setType(unsigned, ExceptionCode&);
> +    void setType(const String&, ExceptionCode&);

So ExceptionCode& is not needed.

> Source/WebCore/Modules/webaudio/OscillatorNode.idl:39
> +	   setter raises(DOMException);

'setter raises(DOMException)' is not needed. According to the spec, the setter
should not throw a DOMException to begin with.

> Source/WebCore/bindings/js/JSOscillatorNodeCustom.cpp:56
> +	   setDOMException(exec, NOT_SUPPORTED_ERR);

Shouldn't this be a JavaScript TypeError?
http://www.w3.org/TR/WebIDL/#idl-enums

> Source/WebCore/bindings/v8/custom/V8OscillatorNodeCustom.cpp:54
> +	   setDOMException(NOT_SUPPORTED_ERR, info.GetIsolate());

Ditto.