Bug 72010 (VibrationAPI)

Created attachment 114467 [details] Core part for the Vibration API

Attachment 114467 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/page/Navigator.cpp:296: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 114467 [details] Core part for the Vibration API View in context: https://bugs.webkit.org/attachment.cgi?id=114467&action=review > Source/WebCore/ChangeLog:9 > + Add a new API for the Vibration API(W3C) > + https://bugs.webkit.org/show_bug.cgi?id=72010 > + > + Reviewed by NOBODY (OOPS!). > + > + Test will be added after finishing. > + Please refer to the spec > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:65 > +#if ENABLE(VIBRATION) Maybe this should be USE, ask abarth on irc. > Source/WebCore/page/Navigator.cpp:295 > + for (int i = 0; i < (int)pattern.size(); i++) We usually avoid C casts, why is this needed

> Maybe this should be USE, ask abarth on irc. ENABLE is correct. This is a web-facing feature.

Comment on attachment 114467 [details] Core part for the Vibration API View in context: https://bugs.webkit.org/attachment.cgi?id=114467&action=review This patch is missing tests. At this point, we just need basic tests for showing that the API exists and you can call it (not necessarily that it does anything). > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:66 > +JSValue JSNavigator::webkitVibrate(ExecState* exec) This shouldn't need custom bindings. > Source/WebCore/page/Navigator.h:29 > +typedef Vector<long, 1> VibrationArray; Why have an inline capacity of 1? > Source/WebCore/page/Navigator.idl:64 > +#if defined(ENABLE_VIBRATION) && ENABLE_VIBRATION > + [Custom] void webkitVibrate(in [Optional=CallWithDefaultValue] Array pattern) > + raises(DOMException); > +#endif This API shouldn't be custom.

Created attachment 115748 [details] Add to support array, int and null parameter for Vibration API. Ex) vibrate([1000, 500, 2000]), vibrate(1000), vibrate([]), vibrate(0), vibrate()...

(In reply to comment #5) > (From update of attachment 114467 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114467&action=review > > This patch is missing tests. At this point, we just need basic tests for showing that the API exists and you can call it (not necessarily that it does anything). > > > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:66 > > +JSValue JSNavigator::webkitVibrate(ExecState* exec) > > This shouldn't need custom bindings. > > > Source/WebCore/page/Navigator.h:29 > > +typedef Vector<long, 1> VibrationArray; > > Why have an inline capacity of 1? Deleted. Vector<long> is right. > > > Source/WebCore/page/Navigator.idl:64 > > +#if defined(ENABLE_VIBRATION) && ENABLE_VIBRATION > > + [Custom] void webkitVibrate(in [Optional=CallWithDefaultValue] Array pattern) > > + raises(DOMException); > > +#endif > > This API shouldn't be custom. I'm not sure but I think there is no way to support the JSArray type without custom. If there is, could recommend me please? Thank you.

(In reply to comment #7) > I'm not sure but I think there is no way to support the JSArray type without custom. > If there is, could recommend me please? Maybe it wouldn't be too difficult to extend the binding generator code? I recently did this for V8 to add float[] and double[] returns. https://bugs.webkit.org/show_bug.cgi?id=71763 I'm not sure about JSC though.

(In reply to comment #8) > (In reply to comment #7) > > I'm not sure but I think there is no way to support the JSArray type without custom. > > If there is, could recommend me please? > > Maybe it wouldn't be too difficult to extend the binding generator code? I recently did this for V8 to add float[] and double[] returns. https://bugs.webkit.org/show_bug.cgi?id=71763 I'm not sure about JSC though. I think these issue can be handled with another bug~~:) Thank you for your comment.

Created attachment 119388 [details] WebCore patch for the vibration API

Created attachment 119395 [details] Patch for the vibration API

Created attachment 120289 [details] Patch for the vibration API

Created attachment 120432 [details] Patch for the vibration API

Comment on attachment 120432 [details] Patch for the vibration API View in context: https://bugs.webkit.org/attachment.cgi?id=120432&action=review Can't you create a mock object so that you can actually test this? Also shouldn't the user grand rights to the web site in order for it to allow vibrating? Just like geolocation, notifications etc? > Source/WebCore/page/VibratorClient.h:27 > + virtual void vibrate(const long& time) = 0; Did you consider how this will work together with suspension? > Source/WebCore/page/VibratorClient.h:28 > + virtual void cancelVibrate() = 0; cancelVibration? > Source/WebCore/page/VibratorClient.h:30 > + virtual bool isSupport() = 0; This shoulds weird in English. I dont think geolocation/devicemotion etc has such a method, but then again you dont delegate things to them > Source/WebCore/page/VibratorClient.h:31 > + virtual void destroyed() = 0; Isnt this called something else in geolocation etc? > Tools/ChangeLog:9 > + No new tests. Then tell why

(In reply to comment #14) > (From update of attachment 120432 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120432&action=review > > Can't you create a mock object so that you can actually test this? Also shouldn't the user grand rights to the web site in order for it to allow vibrating? Just like geolocation, notifications etc? > There are no callback and return value for this. So, if I make a test case for this, I think I have to make this way. ----------------------------------------------------------------------------------------------------------------------------------------- ... layoutTestController.setMockVibration([1000 500 2000]); navigator.vibrate([1000, 500, 2000]); if (layoutTestController.checkVibration()) testPassed("..."); else testFailed("..."); ... ----------------------------------------------------------------------------------------------------------------------------------------- But, I can't see a test case like this. Is this meaningful? There is no permission things in the spec. I agree with the vibration doesn't need to get a permission, because it is not a privacy feature and it can be operated on the visible page only. It makes the process more complicated. Do you think procedure to get a permission is needed for vibration? > > Source/WebCore/page/VibratorClient.h:27 > > + virtual void vibrate(const long& time) = 0; > > Did you consider how this will work together with suspension? > > > Source/WebCore/page/VibratorClient.h:28 > > + virtual void cancelVibrate() = 0; > > cancelVibration? > > > Source/WebCore/page/VibratorClient.h:30 > > + virtual bool isSupport() = 0; > > This shoulds weird in English. I dont think geolocation/devicemotion etc has such a method, but then again you dont delegate things to them > > > Source/WebCore/page/VibratorClient.h:31 > > + virtual void destroyed() = 0; > > Isnt this called something else in geolocation etc? > > > Tools/ChangeLog:9 > > + No new tests. > > Then tell why

Created attachment 120722 [details] Add suspend/resume method and test case.

Comment on attachment 120722 [details] Add suspend/resume method and test case. Attachment 120722 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11046150 New failing tests: fast/dom/navigator-vibrationAPI.html

Created attachment 120726 [details] Add skip test statement for the chromium port.

Created attachment 121257 [details] Tidy code up.

Comment on attachment 121257 [details] Tidy code up. r- because It looks the patch is no longer applicable to ToT. I think this is a perfect example of adopting Module/ system. You can follow what Intents and Gamepad is doing.

(In reply to comment #20) > (From update of attachment 121257 [details]) > r- because It looks the patch is no longer applicable to ToT. > I think this is a perfect example of adopting Module/ system. > You can follow what Intents and Gamepad is doing. By using the [Supplemental] IDL, I think that you can make this change without touching WebCore/page/Navigator.{h,cpp,idl} at all. [1] Create WebCore/Modules/Vibrator/. [2] Write WebCore/Modules/Vibrator/NavigatorVibrator.idl, like this: interface [Conditional=VIBRATOR, Supplemental=Navigator] NavigatorVibrator { [Custom] void webkitVibrate(in Array pattern) raises(DOMException); } [3] Write WebCore/Modules/Vibrator/NavigatorVibrator.{h,cpp}, which includes the change you've made on Navigator.{h,cpp} [4] Move WebCore/page/Vibrator.{h,cpp} and WebCore/page/VibratorClient.h to WebCore/Modules/Vibrator/. You can find similar examples here: https://bugs.webkit.org/attachment.cgi?id=121938&action=review https://bugs.webkit.org/show_bug.cgi?id=76092

Created attachment 124075 [details] Add NavigatorVibration class and move vibration files to the Modules dir.

Comment on attachment 124075 [details] Add NavigatorVibration class and move vibration files to the Modules dir. View in context: https://bugs.webkit.org/attachment.cgi?id=124075&action=review > LayoutTests/ChangeLog:8 > + * fast/dom/navigator-vibration.html: Added. fast/dom/navigator-vibration-expected.txt is missing? > Source/WebCore/ChangeLog:9 > + http://dev.w3.org/2009/dap/vibration/ > + Test: fast/dom/navigator-vibration.html I would like to see more explanations about this change. > Source/WebCore/ChangeLog:11 > + * CMakeLists.txt: Here you can also add explanations for each file (if you want). > Source/WebCore/Modules/vibration/NavigatorVibration.cpp:2 > + * Copyright (C) 2011 Samsung Electronics 2012? > Source/WebCore/Modules/vibration/NavigatorVibration.cpp:56 > + ec = 0; This may be unnecessary. > Source/WebCore/Modules/vibration/NavigatorVibration.h:2 > + * Copyright (C) 2011 Samsung Electronics 2012? > Source/WebCore/Modules/vibration/NavigatorVibration.idl:2 > + * Copyright (C) 2011 Samsung Electronics 2012? > Source/WebCore/Modules/vibration/NavigatorVibration.idl:26 > + [Custom] void webkitVibrate(in Int32Array pattern) raises(DOMException); Please use [JSCCustom] instead of [Custom], since you support JSC only. > Source/WebCore/Modules/vibration/Vibration.h:2 > + * Copyright (C) 2011 Samsung Electronics 2012? > Source/WebCore/Modules/vibration/VibrationClient.h:2 > + * Copyright (C) 2011 Samsung Electronics 2012?

Comment on attachment 124075 [details] Add NavigatorVibration class and move vibration files to the Modules dir. View in context: https://bugs.webkit.org/attachment.cgi?id=124075&action=review >> LayoutTests/ChangeLog:8 >> + * fast/dom/navigator-vibration.html: Added. > > fast/dom/navigator-vibration-expected.txt is missing? Yes. It will be added >> Source/WebCore/ChangeLog:9 >> + Test: fast/dom/navigator-vibration.html > > I would like to see more explanations about this change. OK. >> Source/WebCore/ChangeLog:11 >> + * CMakeLists.txt: > > Here you can also add explanations for each file (if you want). OK. >> Source/WebCore/Modules/vibration/NavigatorVibration.cpp:56 >> + ec = 0; > > This may be unnecessary. You are right. >> Source/WebCore/Modules/vibration/NavigatorVibration.h:2 >> + * Copyright (C) 2011 Samsung Electronics > > 2012? OK. >> Source/WebCore/Modules/vibration/NavigatorVibration.idl:26 >> + [Custom] void webkitVibrate(in Int32Array pattern) raises(DOMException); > > Please use [JSCCustom] instead of [Custom], since you support JSC only. OK.

Created attachment 124263 [details] Modify for the comment #23

Comment on attachment 124263 [details] Modify for the comment #23 Attachment 124263 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11297945 New failing tests: fast/dom/navigator-vibration.html

Created attachment 124267 [details] Add test case for ignoring for cr-linux.

Attachment 124267 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/platform/chromium/test_expectations.txt:108: Path does not exist. fast/dom/navigator-vibrationAPI.html [test/expectations] [2] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 124267 [details] Add test case for ignoring for cr-linux. 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()".

Created attachment 124279 [details] Fix style and cr-linux error

Comment on attachment 124267 [details] Add test case for ignoring for cr-linux. View in context: https://bugs.webkit.org/attachment.cgi?id=124267&action=review >> LayoutTests/ChangeLog:10 >> + * platform/chromium/test_expectations.txt: > > You need to add similar test_expectations.txt for AppleWebKit, AppleWin, GTK, QT, etc. OK. I will add for other ports. >> LayoutTests/fast/dom/navigator-vibration.html:22 >> +shouldBe("exceptionCodeForVibrate", "0"); > > If you want, you can just write > > shouldBe("navigator.webkitVibrate(1000); navigator.webkitVibrate([1000, 300, 500]);", "undefined"); Thank you very much for your comments. >> LayoutTests/fast/dom/navigator-vibration.html:32 >> +shouldBe("exceptionCodeForCancel", "0"); > > If you want, you can just write > > shouldBe("navigator.webkitVibrate(0); navigator.webkitVibrate([]);", "undefined"); Ditto. >> LayoutTests/fast/dom/navigator-vibration.html:41 >> +shouldBe("exceptionCodeForVibrateWithInvalidIntParam", "DOMException.INVALID_ACCESS_ERR"); > > You can just write > > shouldThrow('navigator.webkitVibrate("1000");'); Ditto. >> LayoutTests/fast/dom/navigator-vibration.html:50 >> +shouldBe("exceptionCodeForVibrateWithInvalidArrayParam", "DOMException.INVALID_ACCESS_ERR"); > > You can just write > > shouldThrow('navigator.webkitVibrate([1000, "200", 500]);'); Ditto. >> Tools/Scripts/build-webkit:325 >> + define => "ENABLE_VIBRATION", default => 0, value => \$vibrationSupport }, > > "default => 0" is OK? Maybe it should be "default => isEfl()". I want to enable this feature in the efl patch after landing this.

Created attachment 124419 [details] Add navigator-vibration.html to the skipped lists.

Comment on attachment 124419 [details] Add navigator-vibration.html to the skipped lists. 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

Comment on attachment 124419 [details] Add navigator-vibration.html to the skipped lists. View in context: https://bugs.webkit.org/attachment.cgi?id=124419&action=review > LayoutTests/fast/dom/navigator-vibration.html:15 > +shouldBe("navigator.webkitVibrate(1000); navigator.webkitVibrate([1000, 300, 500]);", "undefined"); > +shouldBe("navigator.webkitVibrate(0); navigator.webkitVibrate([]);", "undefined"); Sorry for my previous bad comments! These tests can be written better as follows, right? shouldBe("navigator.webkitVibrate(0)", "undefined"); shouldBe("navigator.webkitVibrate(1000)", "undefined"); shouldBe("navigator.webkitVibrate([])", "undefined"); shouldBe("navigator.webkitVibrate([1000, 300, 500])", "undefined"); In addition, it is a good idea to add the following test cases: navigator.webkitVibrate() // This should be equivalent to navigator.webkitVibrate(0) navigator.webkitVibrate(-1) navigator.webkitVibrate(4294967295) navigator.webkitVibrate(4294967296) navigator.webkitVibrate(1.23) >> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:90 >> + if (value.isUndefinedOrNull() || !value.isNumber()) { > > This might be "if (value.isUndefinedOrNull() || !value.isUInt32())". Maybe this should be just "if (!value.isUInt32())"?

Comment on attachment 124419 [details] Add navigator-vibration.html to the skipped lists. View in context: https://bugs.webkit.org/attachment.cgi?id=124419&action=review Thank you for your comment again...:) >> LayoutTests/fast/dom/navigator-vibration.html:13 >> + > > You can now remove these. OK. I missed it. >> LayoutTests/fast/dom/navigator-vibration.html:15 >> +shouldBe("navigator.webkitVibrate(0); navigator.webkitVibrate([]);", "undefined"); > > Sorry for my previous bad comments! These tests can be written better as follows, right? > > shouldBe("navigator.webkitVibrate(0)", "undefined"); > shouldBe("navigator.webkitVibrate(1000)", "undefined"); > shouldBe("navigator.webkitVibrate([])", "undefined"); > shouldBe("navigator.webkitVibrate([1000, 300, 500])", "undefined"); > > In addition, it is a good idea to add the following test cases: > > navigator.webkitVibrate() // This should be equivalent to navigator.webkitVibrate(0) > navigator.webkitVibrate(-1) > navigator.webkitVibrate(4294967295) > navigator.webkitVibrate(4294967296) > navigator.webkitVibrate(1.23) OK, but I can't check '4294967295' with JSC because it doesn't support unsigned long properly. Instead, I will check '2147483647'. Can I make it this way? >> 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) No problem. >> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:80 >> + pattern.append(value.asInt32()); > > This might be "value.asUInt32()" (, although asUInt32() just calls asInt32() internally.) OK. >> 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) > ...; > ...; > } Yes. You are better. >>> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:90 >>> + if (value.isUndefinedOrNull() || !value.isNumber()) { >> >> This might be "if (value.isUndefinedOrNull() || !value.isUInt32())". > > Maybe this should be just "if (!value.isUInt32())"? You are right. >> 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 Yes. >> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:96 >> + pattern.append(value.asInt32()); > > value.asUInt32() Yes. >> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:101 >> + setDOMException(exec, INVALID_ACCESS_ERR); > > NOT_SUPPORTED_ERR Yes.

Comment on attachment 124419 [details] Add navigator-vibration.html to the skipped lists. View in context: https://bugs.webkit.org/attachment.cgi?id=124419&action=review >>> LayoutTests/fast/dom/navigator-vibration.html:15 >>> +shouldBe("navigator.webkitVibrate(0); navigator.webkitVibrate([]);", "undefined"); >> >> Sorry for my previous bad comments! These tests can be written better as follows, right? >> >> shouldBe("navigator.webkitVibrate(0)", "undefined"); >> shouldBe("navigator.webkitVibrate(1000)", "undefined"); >> shouldBe("navigator.webkitVibrate([])", "undefined"); >> shouldBe("navigator.webkitVibrate([1000, 300, 500])", "undefined"); >> >> In addition, it is a good idea to add the following test cases: >> >> navigator.webkitVibrate() // This should be equivalent to navigator.webkitVibrate(0) >> navigator.webkitVibrate(-1) >> navigator.webkitVibrate(4294967295) >> navigator.webkitVibrate(4294967296) >> navigator.webkitVibrate(1.23) > > OK, but I can't check '4294967295' with JSC because it doesn't support unsigned long properly. > Instead, I will check '2147483647'. Can I make it this way? Oh, nice catch! That appears to be a bug of JSC. The spec says that unsigned long should be [0, 4294967295] (http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long). Would you please file the bug and then write a test case for 4294967295 with a comment like "This fails in JSC due to bug XXXXX"?

Comment on attachment 124419 [details] Add navigator-vibration.html to the skipped lists. View in context: https://bugs.webkit.org/attachment.cgi?id=124419&action=review >>>> LayoutTests/fast/dom/navigator-vibration.html:15 >>>> +shouldBe("navigator.webkitVibrate(0); navigator.webkitVibrate([]);", "undefined"); >>> >>> Sorry for my previous bad comments! These tests can be written better as follows, right? >>> >>> shouldBe("navigator.webkitVibrate(0)", "undefined"); >>> shouldBe("navigator.webkitVibrate(1000)", "undefined"); >>> shouldBe("navigator.webkitVibrate([])", "undefined"); >>> shouldBe("navigator.webkitVibrate([1000, 300, 500])", "undefined"); >>> >>> In addition, it is a good idea to add the following test cases: >>> >>> navigator.webkitVibrate() // This should be equivalent to navigator.webkitVibrate(0) >>> navigator.webkitVibrate(-1) >>> navigator.webkitVibrate(4294967295) >>> navigator.webkitVibrate(4294967296) >>> navigator.webkitVibrate(1.23) >> >> OK, but I can't check '4294967295' with JSC because it doesn't support unsigned long properly. >> Instead, I will check '2147483647'. Can I make it this way? > > Oh, nice catch! > > That appears to be a bug of JSC. The spec says that unsigned long should be [0, 4294967295] (http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long). Would you please file the bug and then write a test case for 4294967295 with a comment like "This fails in JSC due to bug XXXXX"? No problem. I will make a bug for this.

Created attachment 124495 [details] Fixed patch for the comment #34

Comment on attachment 124495 [details] Fixed patch for the comment #34 View in context: https://bugs.webkit.org/attachment.cgi?id=124495&action=review r+ for all the files other than Modules/vibration/*. Thank you for the patch! Would anyone review Modules/vibration/*? I am not familiar with what the vibration API should do. > Source/WebCore/bindings/js/JSNavigatorVibrationCustom.cpp:51 > + if (exec->hadException()) Can you add "setDOMException(exec, NOT_SUPPORTED_ERR)" before returning jsUndefined()?

Comment on attachment 124495 [details] Fixed patch for the comment #34 View in context: https://bugs.webkit.org/attachment.cgi?id=124495&action=review Thank you!~;) >> Source/WebCore/bindings/js/JSNavigatorVibrationCustom.cpp:51 >> + if (exec->hadException()) > > Can you add "setDOMException(exec, NOT_SUPPORTED_ERR)" before returning jsUndefined()? Sure.

Created attachment 124502 [details] Fixed patch for the comment #40

Comment on attachment 124502 [details] Fixed patch for the comment #40 View in context: https://bugs.webkit.org/attachment.cgi?id=124502&action=review > Source/WebCore/bindings/js/JSNavigatorVibrationCustom.cpp:30 > +JSValue JSNavigator::webkitVibrate(ExecState* exec) Do we really need custom bindings here?

Comment on attachment 124502 [details] Fixed patch for the comment #40 Rather than adding custom bindings in this patch and improving the code generator "in the future", we should improve the code generator first. There's nothing super magical about the bindings this API needs generated. We already have examples of generating bindings with primitive sequences.

Comment on attachment 124502 [details] Fixed patch for the comment #40 View in context: https://bugs.webkit.org/attachment.cgi?id=124502&action=review >> Source/WebCore/bindings/js/JSNavigatorVibrationCustom.cpp:30 >> +JSValue JSNavigator::webkitVibrate(ExecState* exec) > > Do we really need custom bindings here? It seems that this is written as custom code due to method overloading. It might require non-trivial change in code generators to support overloading.

> It seems that this is written as custom code due to method overloading. It might require non-trivial change in code generators to support overloading. Don't we already support overloading?

(In reply to comment #45) > > It seems that this is written as custom code due to method overloading. It might require non-trivial change in code generators to support overloading. > > Don't we already support overloading? If I am understanding it correctly, we just supported overloading for methods whose argument counts are different (by [Optional]). We do not yet support overloading for methods whose argument types are different. Supported: foo(int a) foo(int a, int b) Not supported: foo(int a) foo(DOMString b)

(In reply to comment #46) > (In reply to comment #45) > > > It seems that this is written as custom code due to method overloading. It might require non-trivial change in code generators to support overloading. > > > > Don't we already support overloading? > > If I am understanding it correctly, we just supported overloading for methods whose argument counts are different (by [Optional]). We do not yet support overloading for methods whose argument types are different. > > Supported: > foo(int a) > foo(int a, int b) > > Not supported: > foo(int a) > foo(DOMString b) Sorry, this observation was wrong. As Adam mentioned, code generators have already supported method overloading. (e.g. drawImage() in CanvasRenderingContext2D.idl)

I've add a new bug for supporting long[] in the IDL.(78210) And I'll implement a patch. Thank you for comments Adam and Kentaro.

Created attachment 127696 [details] Patch (Adopt Bug 78085 style).

Comment on attachment 127696 [details] Patch (Adopt Bug 78085 style). Doesn't EFL port enable this by default? I hope at least one build.webkit.org supported port to build this to keep it build-able.

Comment on attachment 127696 [details] Patch (Adopt Bug 78085 style). View in context: https://bugs.webkit.org/attachment.cgi?id=127696&action=review Considering that the API is now dynamically configurable, I think it's OK to enable this for at least one port. (It would be EFL.) It will greatly help this code being part of the tree greenness. > Source/WebCore/Modules/vibration/NavigatorVibration.cpp:57 > + Vibration::from(navigator->frame()->page())->vibrate(time); Please don't rely on the fact that the Vibrator object being provided and check the NULL case. That will allow us to ENABLE this safely without providing the Vibrator object from the API layer, and make it easy to keep this option ENABLE-d. > Source/WebCore/Modules/vibration/NavigatorVibration.cpp:75 > + Vibration::from(navigator->frame()->page())->vibrate(pattern); Ditto.

(In reply to comment #50) > (From update of attachment 127696 [details]) > Doesn't EFL port enable this by default? > I hope at least one build.webkit.org supported port to build this to keep it build-able. OK. But, I was going to add EFL port enable in the bug 75156. Do you think I need to merge bug 75156 and this?

Comment on attachment 127696 [details] Patch (Adopt Bug 78085 style). View in context: https://bugs.webkit.org/attachment.cgi?id=127696&action=review I added EFL port implementation to bug 75156. Thank you for your comment. >> Source/WebCore/Modules/vibration/NavigatorVibration.cpp:57 >> + Vibration::from(navigator->frame()->page())->vibrate(time); > > Please don't rely on the fact that the Vibrator object being provided and check the NULL case. > That will allow us to ENABLE this safely without providing the Vibrator object from the API layer, and make it easy to keep this option ENABLE-d. I think NULL case is checked in "if (!Vibration::isActive(navigator->frame()->page()))". If page is null or vabration is not created, exception code will be returned. Can you review this again please? >> Source/WebCore/Modules/vibration/NavigatorVibration.cpp:75 >> + Vibration::from(navigator->frame()->page())->vibrate(pattern); > > Ditto. Ditto.

(In reply to comment #52) > (In reply to comment #50) > > (From update of attachment 127696 [details] [details]) > > Doesn't EFL port enable this by default? > > I hope at least one build.webkit.org supported port to build this to keep it build-able. > > OK. But, I was going to add EFL port enable in the bug 75156. > Do you think I need to merge bug 75156 and this? Morrita, Kihong probably meant to ask if he needs to combine this patch and #75156, making them a single bug. I am not a reviewer nor committer, but wouldn't it be better to land the core patch and port patch separately? If you meant it to enable the Efl port by default, as you well know, the Efl port patch would cause build break as the necessary core patch is not there yet. Any advice?

> Kihong probably meant to ask if he needs to combine this patch and #75156, making them a single bug. This sounds good. I had a glance at Bug 75156. It's small enough and basically well-written. I'm happy to review these as a single patch. Thanks for your patience.

Created attachment 127786 [details] Patch.

Created attachment 127794 [details] Patch.

Comment on attachment 127794 [details] Patch. Thanks for update! It's time to land this...

Comment on attachment 127794 [details] Patch. Clearing flags on attachment: 127794 Committed r108272: <http://trac.webkit.org/changeset/108272>

All reviewed patches have been landed. Closing bug.

I appreciate all the reviews in this patch. My special thanks goes to Kenneth, Adam, Scott, Morrita, and Kentaro.