[webkit-reviews] review denied: [Bug 72010] Add new API for Vibration API spec. : [Attachment 124419] Add navigator-vibration.html to the skipped lists.

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Kihong Kwon
<kihong.kwon at samsung.com>'s request for review:
Bug 72010: Add new API for Vibration API spec.
https://bugs.webkit.org/show_bug.cgi?id=72010

Attachment 124419: Add navigator-vibration.html to the skipped lists.
https://bugs.webkit.org/attachment.cgi?id=124419&action=review

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


Thank you for the difficult patch, and I am sorry for many iterative reviews...


> LayoutTests/fast/dom/navigator-vibration.html:13
> +var exceptionCodeForVibrate = 0;
> +var exceptionCodeForCancel = 0;
> +var exceptionCodeForVibrateWithInvalidIntParam = 0;
> +var exceptionCodeForVibrateWithInvalidArrayParam = 0;
> +

You can now remove these.

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:71
> +JSValue JSNavigator::webkitVibrate(ExecState* exec)

Would you please make a new file "JSNavigatorVibrationCustom.cpp" and write
this method in the file? (c.f. JSDOMWindowWebSocketCustom.cpp,
JSDOMWindowWebAudioCustom.cpp)

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:80
> +	   pattern.append(value.asInt32());

This might be "value.asUInt32()" (, although asUInt32() just calls asInt32()
internally.)

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:81
> +    else if (JSObject* object = toJSSequence(exec, value, length)) {

I guess we need to fallback to NOT_SUPPORTED_ERR if toJSSequence() returns an
exception. So the logic would be

if (value.isUInt32()) {
    ...;
} else {
    JSObject* object = toJSSequence(...);
    if (exec->hadException()) {
	exec->clearException();
	setDOMException(exec, NOT_SUPPORTED_ERR);
	return jsUndefined();
    }
    if (!length)
	...;
    ...;
}

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:90
> +	       if (value.isUndefinedOrNull() || !value.isNumber()) {

This might be "if (value.isUndefinedOrNull() || !value.isUInt32())".

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:91
> +		   setDOMException(exec, INVALID_ACCESS_ERR);

This should be NOT_SUPPORTED_ERR, according to the spec:
http://dev.w3.org/2009/dap/vibration/#vibration-interface

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:96
> +		   pattern.append(value.asInt32());

value.asUInt32()

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:101
> +	   setDOMException(exec, INVALID_ACCESS_ERR);

NOT_SUPPORTED_ERR