Bug 105977 – Implement WebIDL-style string constants in WebAudio (part 2)

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Implement WebIDL-style string constants in WebAudio (part 2)

[Chris Rogers]

Created attachment 181123 [details]
Patch

[Adam Barth]

Comment on attachment 181123 [details]
Patch

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

> Source/WebCore/Modules/webaudio/BiquadFilterNode.cpp:47
> +        return "lowpass";

You can wrap these constants in ASCIILiteral if you want to make things slightly faster.

[WebKit Review Bot]

Comment on attachment 181123 [details]
Patch

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

[Chris Rogers]

Created attachment 181126 [details]
Patch

[Chris Rogers]

Comment on attachment 181123 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/BiquadFilterNode.cpp:47
>> +        return "lowpass";
> 
> You can wrap these constants in ASCIILiteral if you want to make things slightly faster.

I'm not super worried about efficiency on the getters and it seems like it might be a little less readable this way.  Do you have a strong preference?

[Chris Rogers]

Kentaro, would you mind having a look.  This one is very similar to the "part 1" patch:
https://bugs.webkit.org/show_bug.cgi?id=105058

Thanks!

[Adam Barth]

> Do you have a strong preference?

Nope!

[Kentaro Hara]

Comment on attachment 181126 [details]
Patch

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

LGTM

> Source/WebCore/bindings/v8/custom/V8BiquadFilterNodeCustom.cpp:40
> +    v8::Handle<v8::Object> holder = info.Holder();
> +    BiquadFilterNode* imp = V8BiquadFilterNode::toNative(holder);

Nit: You can just write 'info.Holder()' instead of putting it into 'holder'.

> Source/WebCore/bindings/v8/custom/V8BiquadFilterNodeCustom.cpp:46
> +        if (!ok || !imp->setType(type))

This should be 'ASSERT(ok)'. Since you are checking IsNumber() above, toUInt32() should not fail.

> Source/WebCore/bindings/v8/custom/V8PannerNodeCustom.cpp:40
> +    v8::Handle<v8::Object> holder = info.Holder();
> +    PannerNode* imp = V8PannerNode::toNative(holder);

Ditto. You can write 'info.Holder()'.

> Source/WebCore/bindings/v8/custom/V8PannerNodeCustom.cpp:46
> +        if (!ok || !imp->setPanningModel(model))

Ditto. 'ASSERT(ok)'.

> Source/WebCore/bindings/v8/custom/V8PannerNodeCustom.cpp:66
> +    v8::Handle<v8::Object> holder = info.Holder();
> +    PannerNode* imp = V8PannerNode::toNative(holder);

Ditto.

> Source/WebCore/bindings/v8/custom/V8PannerNodeCustom.cpp:72
> +        if (!ok || !imp->setDistanceModel(model))

Ditto.

[Kentaro Hara]

nbarth: Once you implement 'enum' in WebKit IDLs, you might want to clean up the custom bindings of this patch.

[Chris Rogers]

Committed r138849: <http://trac.webkit.org/changeset/138849>