[webkit-reviews] review granted: [Bug 105977] Implement WebIDL-style string constants in WebAudio (part 2) : [Attachment 181126] Patch

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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.