[webkit-reviews] review denied: [Bug 72010] Add new API for Vibration API spec. : [Attachment 124267] Add test case for ignoring for cr-linux.

[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 124267: Add test case for ignoring for cr-linux.
https://bugs.webkit.org/attachment.cgi?id=124267&action=review

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


Actually I am not familiar with the Vibration API and cannot review the
implementation of the Vibration API (Modules/vibration/*, page/* and
bindings/js/JSNavigatorCustom.cpp). Could anyone review it?

> LayoutTests/ChangeLog:10
> +	   * platform/chromium/test_expectations.txt:

You need to add similar test_expectations.txt for AppleWebKit, AppleWin, GTK,
QT, etc.

> LayoutTests/fast/dom/navigator-vibration.html:22
> +try {
> +    navigator.webkitVibrate(1000);
> +    navigator.webkitVibrate([1000, 300, 500]);
> +}
> +catch (e) {
> +    exceptionCodeForVibrate = e.code;
> +}
> +
> +shouldBe("exceptionCodeForVibrate", "0");

If you want, you can just write

    shouldBe("navigator.webkitVibrate(1000); navigator.webkitVibrate([1000,
300, 500]);", "undefined");

> LayoutTests/fast/dom/navigator-vibration.html:32
> +try {
> +    navigator.webkitVibrate(0);
> +    navigator.webkitVibrate([]);
> +}
> +catch (e) {
> +    exceptionCodeForCancel = e.code;
> +}
> +
> +shouldBe("exceptionCodeForCancel", "0");

If you want, you can just write

    shouldBe("navigator.webkitVibrate(0); navigator.webkitVibrate([]);",
"undefined");

> LayoutTests/fast/dom/navigator-vibration.html:41
> +try {
> +    navigator.webkitVibrate("1000");
> +}
> +catch (e) {
> +    exceptionCodeForVibrateWithInvalidIntParam = e.code;
> +}
> +
> +shouldBe("exceptionCodeForVibrateWithInvalidIntParam",
"DOMException.INVALID_ACCESS_ERR");

You can just write

    shouldThrow('navigator.webkitVibrate("1000");');

> LayoutTests/fast/dom/navigator-vibration.html:50
> +try {
> +    navigator.webkitVibrate([1000, "200", 500]);
> +}
> +catch (e) {
> +    exceptionCodeForVibrateWithInvalidArrayParam = e.code;
> +}
> +
> +shouldBe("exceptionCodeForVibrateWithInvalidArrayParam",
"DOMException.INVALID_ACCESS_ERR");

You can just write

    shouldThrow('navigator.webkitVibrate([1000, "200", 500]);');

> LayoutTests/platform/chromium/test_expectations.txt:105
> +BUGWK72010 SKIP : fast/dom/navigator-vibrationAPI.html = FAIL

This should be "fast/dom/navigator-vibration.html".

> Tools/Scripts/build-webkit:325
> +	 define => "ENABLE_VIBRATION", default => 0, value =>
\$vibrationSupport },

"default => 0" is OK? Maybe it should be "default => isEfl()".