[webkit-reviews] review denied: [Bug 105058] Implement WebIDL-style string constants in WebAudio : [Attachment 179849] 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 179849: Patch
https://bugs.webkit.org/attachment.cgi?id=179849&action=review

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


> Source/WebCore/ChangeLog:12
> +	   PannerNode, BiquadFilterNode, OscillatorNode constants must support
WebIDL-style string constants.
> +	   Legacy support in the setters for the old integer values should be
supported.

Actually I don't fully understand why we want to support the old integer values
for setters only. What's the rationale for keeping the behavior for setters
only? Given that getters return strings anyway, it wouldn't make sense to
support integer values for setters.

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:88
> +    switch (m_type) {

How about changing the type of m_type from unsigned short to AtomicString,
because we're deprecating the unsigned short m_type?

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:91
> +	   break;

break is not needed. Ditto for other breaks.

> Source/WebCore/bindings/js/JSOscillatorNodeCustom.cpp:62
> +    if (value.isString()) {
> +	   ExceptionCode ec = 0;
> +	   imp->setType(value.toString(exec)->value(exec), ec);
> +	   if (ec)
> +	       setDOMException(exec, ec);
> +	   return;
> +    }
> +
> +#if ENABLE(LEGACY_WEB_AUDIO)
> +    if (value.isNumber()) {
> +	   uint32_t type = value.toUInt32(exec);
> +	   ExceptionCode ec = 0;
> +	   imp->setType(type, ec);
> +	   if (ec)
> +	       setDOMException(exec, ec);
> +	   return;
> +    }
> +#endif
> +
> +    setDOMException(exec, NOT_SUPPORTED_ERR);

Maybe this logic should be:

#if ENABLE(LEGACY_WEB_AUDIO)
  if (value.isNumber()) {
    ...;
  }
#endif

  String type = value.toString(exec)0->value(exec);
  if (exec->hadException()) {
    ...; // Depends on the spec.
    return;
  }
  ExceptionCode ec = 0;
  imp->setType(value.toString(exec)->value(exec), ec);
  ...;
}

Please confirm the spec.

> Source/WebCore/bindings/v8/custom/V8OscillatorNodeCustom.cpp:66
> +    if (value->IsString()) {
> +	   String type = toWebCoreString(value);
> +	   ExceptionCode ec = 0;
> +	   imp->setType(type, ec);
> +	   if (ec)
> +	       setDOMException(ec, info.GetIsolate());
> +	   return;
> +    }
> +
> +#if ENABLE(LEGACY_WEB_AUDIO)    
> +    if (value->IsNumber()) {
> +	   bool ok = false;
> +	   uint32_t type = toUInt32(value, ok);
> +	   if (ok) {
> +	       ExceptionCode ec = 0;
> +	       imp->setType(type, ec);
> +	       if (ec)
> +		   setDOMException(ec, info.GetIsolate());
> +	       return;
> +	   }
> +    }
> +#endif
> +
> +    setDOMException(NOT_SUPPORTED_ERR, info.GetIsolate());

Ditto.