See Deprecation Notes for more detail: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#DeprecationNotes PannerNode, BiquadFilterNode, OscillatorNode constants must support WebIDL-style string constants. Legacy support in the setters for the old integer values should be supported.
Kentaro, do you have any suggestions for how to implement both string and integer values in the setters for these attributes? I'm assuming this will require custom setters and we have no way to auto-generate? If so, do you have any suggestions for examples of custom bindings in WebKit which solve a similar problem to get me started - thanks for any advice!
(In reply to comment #1) > Kentaro, do you have any suggestions for how to implement both string and integer values in the setters for these attributes? I'm assuming this will require custom setters and we have no way to auto-generate? If so, do you have any suggestions for examples of custom bindings in WebKit which solve a similar problem to get me started - thanks for any advice! Hmm, that sounds like a bunch of work. For now, let me describe just the overall idea. I can support you more in detail if you have more specific questions. (1) Currently the WebKit IDL doesn't support 'enum'. So you need to implement 'enum' in code generators. (2) You need to implement DOM attribute overloading. Specifically, you need to improve code generators so that the following IDL works: attribute unsigned short type; attribute BiquadFilterType type;
(In reply to comment #2) > (In reply to comment #1) > > Kentaro, do you have any suggestions for how to implement both string and integer values in the setters for these attributes? I'm assuming this will require custom setters and we have no way to auto-generate? If so, do you have any suggestions for examples of custom bindings in WebKit which solve a similar problem to get me started - thanks for any advice! > > Hmm, that sounds like a bunch of work. For now, let me describe just the overall idea. I can support you more in detail if you have more specific questions. Kentaro, thanks for the info. > > (1) Currently the WebKit IDL doesn't support 'enum'. So you need to implement 'enum' in code generators. > I'm not so worried about supporting 'enum'. We already deal with string constants in several other places, such as the .responseType attribute of XMLHttpRequest. I'm not sure (for now at least) that we have to "formally" use enum. > (2) You need to implement DOM attribute overloading. Specifically, you need to improve code generators so that the following IDL works: > > attribute unsigned short type; > attribute BiquadFilterType type; One tricky aspect here is that the return value of the attribute will be a BiquadFilterType (effectively a DOMString). So how do we specify that the 2nd one (for BiquadFilterType) is to be used for the return value, while both should be allowed for setters? As an alternative, isn't it possible to avoid changing the code generators and write the custom bindings code by hand? I don't know which way would be harder and more time-consuming. But I do understand in principle how to write them by hand. What do you think?
(In reply to comment #3) > > (1) Currently the WebKit IDL doesn't support 'enum'. So you need to implement 'enum' in code generators. > > > > I'm not so worried about supporting 'enum'. We already deal with string constants in several other places, such as the .responseType attribute of XMLHttpRequest. I'm not sure (for now at least) that we have to "formally" use enum. OK, that sounds reasonable, though we want to support an enum eventually to make the WebKit IDL comformed to the spec. > > (2) You need to implement DOM attribute overloading. Specifically, you need to improve code generators so that the following IDL works: > > > > attribute unsigned short type; > > attribute BiquadFilterType type; > > One tricky aspect here is that the return value of the attribute will be a BiquadFilterType (effectively a DOMString). So how do we specify that the 2nd one (for BiquadFilterType) is to be used for the return value, while both should be allowed for setters? You can switch behaviors depending on types. For example, look at bindings/v8/custom/V8BlobCustom.cpp. V8BlobCustom.cpp switches behaviors depending on IsObject(), IsArray(), etc. > As an alternative, isn't it possible to avoid changing the code generators and write the custom bindings code by hand? I don't know which way would be harder and more time-consuming. But I do understand in principle how to write them by hand. What do you think? Custom bindings would be less time-consuming for you. However, custom bindings are strongly discouraged (and we've been trying to kill custom bindings). Unless you have a strong reason to use custom bindings, you should try best to solve your problem by improving code generators. For how many DOM attributes are you going to use the DOM attribute overloading?
(In reply to comment #4) > (In reply to comment #3) > > > (1) Currently the WebKit IDL doesn't support 'enum'. So you need to implement 'enum' in code generators. > > > > > > > I'm not so worried about supporting 'enum'. We already deal with string constants in several other places, such as the .responseType attribute of XMLHttpRequest. I'm not sure (for now at least) that we have to "formally" use enum. > > OK, that sounds reasonable, though we want to support an enum eventually to make the WebKit IDL comformed to the spec. > > > > (2) You need to implement DOM attribute overloading. Specifically, you need to improve code generators so that the following IDL works: > > > > > > attribute unsigned short type; > > > attribute BiquadFilterType type; > > > > One tricky aspect here is that the return value of the attribute will be a BiquadFilterType (effectively a DOMString). So how do we specify that the 2nd one (for BiquadFilterType) is to be used for the return value, while both should be allowed for setters? > > You can switch behaviors depending on types. For example, look at bindings/v8/custom/V8BlobCustom.cpp. V8BlobCustom.cpp switches behaviors depending on IsObject(), IsArray(), etc. > > > As an alternative, isn't it possible to avoid changing the code generators and write the custom bindings code by hand? I don't know which way would be harder and more time-consuming. But I do understand in principle how to write them by hand. What do you think? > > Custom bindings would be less time-consuming for you. However, custom bindings are strongly discouraged (and we've been trying to kill custom bindings). Unless you have a strong reason to use custom bindings, you should try best to solve your problem by improving code generators. > > For how many DOM attributes are you going to use the DOM attribute overloading? It's for the four attributes described here: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#DeprecationNotes I'm not quite sure how it can be possible to make this automatically generated, could you please provide some more details about your idea? For each string value, there will be a corresponding integer value, and that correspondance must be mapped somehow. I'm not sure how that information/mapping would look like in terms of the IDL. Thanks for your help.
(In reply to comment #4) > (In reply to comment #3) > > > (1) Currently the WebKit IDL doesn't support 'enum'. So you need to implement 'enum' in code generators. > > > > > > > I'm not so worried about supporting 'enum'. We already deal with string constants in several other places, such as the .responseType attribute of XMLHttpRequest. I'm not sure (for now at least) that we have to "formally" use enum. > > OK, that sounds reasonable, though we want to support an enum eventually to make the WebKit IDL comformed to the spec. > > > > (2) You need to implement DOM attribute overloading. Specifically, you need to improve code generators so that the following IDL works: > > > > > > attribute unsigned short type; > > > attribute BiquadFilterType type; > > > > One tricky aspect here is that the return value of the attribute will be a BiquadFilterType (effectively a DOMString). So how do we specify that the 2nd one (for BiquadFilterType) is to be used for the return value, while both should be allowed for setters? > > You can switch behaviors depending on types. For example, look at bindings/v8/custom/V8BlobCustom.cpp. V8BlobCustom.cpp switches behaviors depending on IsObject(), IsArray(), etc. Sure, I understand how to switch behaviors for the setters. The main question is how to specify the "getter" return value, since specifying the attribute twice with two different types makes this ambiguous.
(In reply to comment #6) > Sure, I understand how to switch behaviors for the setters. The main question is how to specify the "getter" return value, since specifying the attribute twice with two different types makes this ambiguous. hmm, maybe custom bindings would make sense in this case, because (1) it will anyway mess up code generators, (2) DOM attribute overloading is not a mechanism in the spec, and (3) the mechanism will be used by Web Audio only.
Created attachment 179849 [details] Patch
I've tried out custom bindings to see how they would look. This first patch handles the custom bindings for OscillatorNode. I can add the other ones if this approach looks reasonable.
Comment on attachment 179849 [details] Patch 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.
Created attachment 180028 [details] Patch
Comment on attachment 179849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179849&action=review >> Source/WebCore/ChangeLog:12 >> + 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. The reason is so that we don't break all of the Web Audio pages (out in the wild) that use these attributes. Since they're currently setting these attributes to integers, these pages would break if we suddenly switched to only accepting strings in the setters. We consider that this breaking behavior would be severe, so need to carefully support both. The getters are very much less commonly used than the setters, so although getting the attribute value has the potential to break pages since it will now return a string, we feel this will hurt very few pages using Web Audio. >> 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? For efficiency (memory-wise and execution-wise for some of these constants), I think it's better to keep track of the value as an integer. This is also how we handle the .responseType attribute in XMLHttpRequest::responseType() >> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:91 >> + break; > > break is not needed. Ditto for other breaks. Fixed >> Source/WebCore/bindings/js/JSOscillatorNodeCustom.cpp:62 >> + 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. I've switched the order as you suggest. I also added in the check for exec->hadException(), although I'm unsure why it's needed since we can already check/know that this is a string since we were calling value.isString(). Are you suggesting we remove the value.isString() check?? This is what I've assumed from your code above. >> Source/WebCore/bindings/v8/custom/V8OscillatorNodeCustom.cpp:66 >> + setDOMException(NOT_SUPPORTED_ERR, info.GetIsolate()); > > Ditto. I've made the same change of order as you suggested for JS bindings, and I've removed the check for value->IsString(). I'm assuming that in the case where the value cannot be converted to a string, then it will be equal to the empty string "", in which case imp->setType(...) will see this as an invalid value and the exception will be returned.
(In reply to comment #12) > We consider that this breaking behavior would be severe, so need to carefully support both. The getters are very much less commonly used than the setters, so although getting the attribute value has the potential to break pages since it will now return a string, we feel this will hurt very few pages using Web Audio. Makes sense. Thanks for the clarification! > >> 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? > > For efficiency (memory-wise and execution-wise for some of these constants), I think it's better to keep track of the value as an integer. This is also how we handle the .responseType attribute in XMLHttpRequest::responseType() The implementation of XMLHttpRequest::responseType() is just a legacy. My co-worker is trying to implement 'enum' in WebKit IDLs and replace the integers with AtomicStrings. As long as we use an AtomicString, I don't think it will cause any memory-wise or execution-wise problem. > >> Source/WebCore/bindings/js/JSOscillatorNodeCustom.cpp:62 > >> + 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. > > I've switched the order as you suggest. I also added in the check for exec->hadException(), although I'm unsure why it's needed since we can already check/know that this is a string since we were calling value.isString(). Are you suggesting we remove the value.isString() check?? This is what I've assumed from your code above. I was assuming that removing the value.isString() check would be the expected behavior, but I'm not sure. I'd be happy you could check the spec.
Comment on attachment 180028 [details] Patch 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.
(In reply to comment #13) > (In reply to comment #12) > > We consider that this breaking behavior would be severe, so need to carefully support both. The getters are very much less commonly used than the setters, so although getting the attribute value has the potential to break pages since it will now return a string, we feel this will hurt very few pages using Web Audio. > > Makes sense. Thanks for the clarification! > > > >> 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? > > > > For efficiency (memory-wise and execution-wise for some of these constants), I think it's better to keep track of the value as an integer. This is also how we handle the .responseType attribute in XMLHttpRequest::responseType() > > The implementation of XMLHttpRequest::responseType() is just a legacy. My co-worker is trying to implement 'enum' in WebKit IDLs and replace the integers with AtomicStrings. As long as we use an AtomicString, I don't think it will cause any memory-wise or execution-wise problem. > > > >> Source/WebCore/bindings/js/JSOscillatorNodeCustom.cpp:62 > > >> + 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. > > > > I've switched the order as you suggest. I also added in the check for exec->hadException(), although I'm unsure why it's needed since we can already check/know that this is a string since we were calling value.isString(). Are you suggesting we remove the value.isString() check?? This is what I've assumed from your code above. > > I was assuming that removing the value.isString() check would be the expected behavior, but I'm not sure. I'd be happy you could check the spec. Actually I'm the spec editor, but I'm not sure I understand what the distinction is. I'm expecting either an integer value or a string value will be assigned to these attributes. Is this code supposed to handle more cases, where non-string objects could potentially be converted to strings which would be meaningful here? Are there such cases with Web APIs where this is useful? A concrete example using some other API might be helpful to help me understand, so I can see what the idea is - thanks :)
(In reply to comment #15) > Actually I'm the spec editor, but I'm not sure I understand what the distinction is. I'm expecting either an integer value or a string value will be assigned to these attributes. Is this code supposed to handle more cases, where non-string objects could potentially be converted to strings which would be meaningful here? Are there such cases with Web APIs where this is useful? A concrete example using some other API might be helpful to help me understand, so I can see what the idea is - thanks :) I rechecked the Web IDL spec (http://www.w3.org/TR/WebIDL/#idl-enums)... sorry, you're right. Then a right way to implement it would be: #if LEGACY if (value.isInteger()) { ...; } #endif if (value.isString()) { String type = value.toString(exec)0->value(exec); if (type == "sine" || type == "square" || ...) { imp->setType(type); return; } } throwVMTypeError(...); return; What do you think?
Created attachment 180242 [details] Patch
(In reply to comment #16) > (In reply to comment #15) > > Actually I'm the spec editor, but I'm not sure I understand what the distinction is. I'm expecting either an integer value or a string value will be assigned to these attributes. Is this code supposed to handle more cases, where non-string objects could potentially be converted to strings which would be meaningful here? Are there such cases with Web APIs where this is useful? A concrete example using some other API might be helpful to help me understand, so I can see what the idea is - thanks :) > > I rechecked the Web IDL spec (http://www.w3.org/TR/WebIDL/#idl-enums)... sorry, you're right. > > Then a right way to implement it would be: > > #if LEGACY > if (value.isInteger()) { > ...; > } > #endif > > if (value.isString()) { > String type = value.toString(exec)0->value(exec); > if (type == "sine" || type == "square" || ...) { > imp->setType(type); > return; > } > } > throwVMTypeError(...); > return; > > > What do you think? Sounds good to me - patch updated. The m_type I still feel is better as an integer value, since these values (not the OscillatorNode, but the other ones) have concurrency issues and are accessed by another thread (real-time audio thread). Although the AtomicString footprint might be small, it is still inefficient and more cumbersome to deal with internally where these values are used. I've discussed this with Ken Russell and he agrees, so leaving this part as is.
Comment on attachment 180242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180242&action=review Thanks for updating the patch. Looks almost OK to me. I understood that m_type should be integers, which sounds reasonable to me. > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:115 > + if (type == "sine") > + setType(SINE); > + else if (type == "square") > + setType(SQUARE); > + else if (type == "sawtooth") > + setType(SAWTOOTH); > + else if (type == "triangle") > + setType(TRIANGLE); > + else > + return false; Would you do this validation in JSOscillatorNodeCustom.cpp and V8OscillatorNodeCustom.cpp, because the validation is a concept defined in the Web IDL spec and thus should be resolved in the binding layer not in the WebCore layer? The signature of OscillatorNode::setType should be 'void OscillatorNode::setType(const String& type)'. I know it will lead to code duplication (i.e. we need to write 'if (type == ...)' in three places), but I think it would be the right thing to do. (Though the right thing to do is to auto-generate the binding code:-)
Comment on attachment 180242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180242&action=review > Source/WebCore/bindings/v8/custom/V8OscillatorNodeCustom.cpp:48 > + throwError(v8TypeError, "Illegal OscillatorNode type", info.GetIsolate()); Would you make the error message identical between JSC and V8? > Source/WebCore/bindings/v8/custom/V8OscillatorNodeCustom.cpp:59 > + throwError(v8TypeError, "Illegal OscillatorNode type", info.GetIsolate()); Ditto. > LayoutTests/webaudio/oscillator-basic-expected.txt:12 > +PASS Oscillator correctly set to SINE type using legacy integer value. > +PASS Oscillator correctly set to SQUARE type using legacy integer value. > +PASS Oscillator correctly set to SAWTOOTH type using legacy integer value. > +PASS Oscillator correctly set to TRIANGLE type using legacy integer value. Would you add a test case that throws a TypeError?
Created attachment 180559 [details] Patch
(In reply to comment #19) > (From update of attachment 180242 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180242&action=review > > Thanks for updating the patch. Looks almost OK to me. I understood that m_type should be integers, which sounds reasonable to me. > > > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:115 > > + if (type == "sine") > > + setType(SINE); > > + else if (type == "square") > > + setType(SQUARE); > > + else if (type == "sawtooth") > > + setType(SAWTOOTH); > > + else if (type == "triangle") > > + setType(TRIANGLE); > > + else > > + return false; > > Would you do this validation in JSOscillatorNodeCustom.cpp and V8OscillatorNodeCustom.cpp, because the validation is a concept defined in the Web IDL spec and thus should be resolved in the binding layer not in the WebCore layer? The signature of OscillatorNode::setType should be 'void OscillatorNode::setType(const String& type)'. > > I know it will lead to code duplication (i.e. we need to write 'if (type == ...)' in three places), but I think it would be the right thing to do. (Though the right thing to do is to auto-generate the binding code:-) Fixed.
(In reply to comment #20) > (From update of attachment 180242 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180242&action=review > > > Source/WebCore/bindings/v8/custom/V8OscillatorNodeCustom.cpp:48 > > + throwError(v8TypeError, "Illegal OscillatorNode type", info.GetIsolate()); > > Would you make the error message identical between JSC and V8? JSC error message has been fixed to be consistent. > > > Source/WebCore/bindings/v8/custom/V8OscillatorNodeCustom.cpp:59 > > + throwError(v8TypeError, "Illegal OscillatorNode type", info.GetIsolate()); > > Ditto. JSC error message has been fixed to be consistent. > > > LayoutTests/webaudio/oscillator-basic-expected.txt:12 > > +PASS Oscillator correctly set to SINE type using legacy integer value. > > +PASS Oscillator correctly set to SQUARE type using legacy integer value. > > +PASS Oscillator correctly set to SAWTOOTH type using legacy integer value. > > +PASS Oscillator correctly set to TRIANGLE type using legacy integer value. > > Would you add a test case that throws a TypeError? Fixed: added two new tests to check that illegal string value and illegal type (non-string object) throws TypeError specifically. Kentaro, thanks for having a look so far. If this looks OK, then I'd like to land this patch and address the other attributes in a "part 2" followup patch.
Comment on attachment 180559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180559&action=review LGTM. Thanks for the patch! > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:113 > + else if (type == "triangle") > + setType(TRIANGLE); Let's add: else ASSERT_NOT_REACHED();
Committed r138631: <http://trac.webkit.org/changeset/138631>