Bug 72010 (VibrationAPI) - Support for Vibration API
Summary: Support for Vibration API
Status: RESOLVED FIXED
Alias: VibrationAPI
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Enhancement
Assignee: Nobody
URL: http://www.w3.org/TR/2012/WD-vibratio...
Keywords:
Depends on: 78085 78210
Blocks: DAP 84386 75156
  Show dependency treegraph
 
Reported: 2011-11-10 03:39 PST by Kihong Kwon
Modified: 2012-04-19 14:46 PDT (History)
23 users (show)

See Also:


Attachments
Core part for the Vibration API (3.60 KB, patch)
2011-11-10 04:05 PST, Kihong Kwon
abarth: review-
Details | Formatted Diff | Diff
Add to support array, int and null parameter for Vibration API. Ex) vibrate([1000, 500, 2000]), vibrate(1000), vibrate([]), vibrate(0), vibrate()... (6.20 KB, patch)
2011-11-17 23:42 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
WebCore patch for the vibration API (13.18 KB, patch)
2011-12-15 00:14 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch for the vibration API (13.70 KB, patch)
2011-12-15 01:03 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch for the vibration API (15.01 KB, patch)
2011-12-22 01:39 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch for the vibration API (15.10 KB, patch)
2011-12-22 20:47 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Add suspend/resume method and test case. (19.51 KB, patch)
2011-12-29 00:48 PST, Kihong Kwon
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Add skip test statement for the chromium port. (20.33 KB, patch)
2011-12-29 02:20 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Tidy code up. (20.29 KB, patch)
2012-01-05 04:10 PST, Kihong Kwon
morrita: review-
Details | Formatted Diff | Diff
Add NavigatorVibration class and move vibration files to the Modules dir. (21.18 KB, patch)
2012-01-26 01:12 PST, Kihong Kwon
haraken: review-
haraken: commit-queue-
Details | Formatted Diff | Diff
Modify for the comment #23 (23.99 KB, patch)
2012-01-26 22:27 PST, Kihong Kwon
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Add test case for ignoring for cr-linux. (24.73 KB, patch)
2012-01-27 00:29 PST, Kihong Kwon
haraken: review-
haraken: commit-queue-
Details | Formatted Diff | Diff
Fix style and cr-linux error (24.73 KB, patch)
2012-01-27 01:56 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Add navigator-vibration.html to the skipped lists. (27.35 KB, patch)
2012-01-27 18:32 PST, Kihong Kwon
haraken: review-
haraken: commit-queue-
Details | Formatted Diff | Diff
Fixed patch for the comment #34 (27.98 KB, patch)
2012-01-29 21:58 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Fixed patch for the comment #40 (27.92 KB, patch)
2012-01-30 00:14 PST, Kihong Kwon
abarth: review-
Details | Formatted Diff | Diff
Patch (Adopt Bug 78085 style). (23.71 KB, patch)
2012-02-17 23:20 PST, Kihong Kwon
morrita: review-
morrita: commit-queue-
Details | Formatted Diff | Diff
Patch. (31.93 KB, patch)
2012-02-20 02:27 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch. (31.99 KB, patch)
2012-02-20 02:58 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kihong Kwon 2011-11-10 03:39:10 PST
I'd like to add the Vibration API spec to the WebKit.
This is a first basic patch.
I'm going to finish this patch ASAP.
Comment 1 Kihong Kwon 2011-11-10 04:05:26 PST
Created attachment 114467 [details]
Core part for the Vibration API
Comment 2 WebKit Review Bot 2011-11-10 04:07:56 PST
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 3 Kenneth Rohde Christiansen 2011-11-10 06:41:28 PST
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
Comment 4 Adam Barth 2011-11-10 09:10:55 PST
> Maybe this should be USE, ask abarth on irc.

ENABLE is correct.  This is a web-facing feature.
Comment 5 Adam Barth 2011-11-10 09:14:02 PST
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.
Comment 6 Kihong Kwon 2011-11-17 23:42:43 PST
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()...
Comment 7 Kihong Kwon 2011-11-17 23:46:22 PST
(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.
Comment 8 Scott Graham 2011-11-18 08:31:30 PST
(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.
Comment 9 Kihong Kwon 2011-12-13 02:16:41 PST
(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.
Comment 10 Kihong Kwon 2011-12-15 00:14:37 PST
Created attachment 119388 [details]
WebCore patch for the vibration API
Comment 11 Kihong Kwon 2011-12-15 01:03:00 PST
Created attachment 119395 [details]
Patch for the vibration API
Comment 12 Kihong Kwon 2011-12-22 01:39:58 PST
Created attachment 120289 [details]
Patch for the vibration API
Comment 13 Kihong Kwon 2011-12-22 20:47:55 PST
Created attachment 120432 [details]
Patch for the vibration API
Comment 14 Kenneth Rohde Christiansen 2011-12-23 03:27:19 PST
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
Comment 15 Kihong Kwon 2011-12-26 18:49:26 PST
(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
Comment 16 Kihong Kwon 2011-12-29 00:48:47 PST
Created attachment 120722 [details]
Add suspend/resume method and test case.
Comment 17 WebKit Review Bot 2011-12-29 01:32:02 PST
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
Comment 18 Kihong Kwon 2011-12-29 02:20:32 PST
Created attachment 120726 [details]
Add skip test statement for the chromium port.
Comment 19 Kihong Kwon 2012-01-05 04:10:05 PST
Created attachment 121257 [details]
Tidy code up.
Comment 20 Hajime Morrita 2012-01-16 00:36:20 PST
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.
Comment 21 Kentaro Hara 2012-01-16 00:51:37 PST
(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
Comment 22 Kihong Kwon 2012-01-26 01:12:21 PST
Created attachment 124075 [details]
Add NavigatorVibration class and move vibration files to the Modules dir.
Comment 23 Kentaro Hara 2012-01-26 08:59:18 PST
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 24 Kihong Kwon 2012-01-26 22:21:11 PST
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.
Comment 25 Kihong Kwon 2012-01-26 22:27:00 PST
Created attachment 124263 [details]
Modify for the comment #23
Comment 26 WebKit Review Bot 2012-01-26 23:24:39 PST
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
Comment 27 Kihong Kwon 2012-01-27 00:29:20 PST
Created attachment 124267 [details]
Add test case for ignoring for cr-linux.
Comment 28 WebKit Review Bot 2012-01-27 00:31:19 PST
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 29 Kentaro Hara 2012-01-27 01:05:59 PST
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()".
Comment 30 Kihong Kwon 2012-01-27 01:56:26 PST
Created attachment 124279 [details]
Fix style and cr-linux error
Comment 31 Kihong Kwon 2012-01-27 04:22:49 PST
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.
Comment 32 Kihong Kwon 2012-01-27 18:32:17 PST
Created attachment 124419 [details]
Add navigator-vibration.html to the skipped lists.
Comment 33 Kentaro Hara 2012-01-27 19:39:28 PST
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 34 Kentaro Hara 2012-01-28 05:49:06 PST
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 35 Kihong Kwon 2012-01-29 20:22:56 PST
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 36 Kentaro Hara 2012-01-29 20:38:03 PST
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 37 Kihong Kwon 2012-01-29 21:02:07 PST
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.
Comment 38 Kihong Kwon 2012-01-29 21:58:36 PST
Created attachment 124495 [details]
Fixed patch for the comment #34
Comment 39 Kentaro Hara 2012-01-29 22:06:52 PST
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 40 Kihong Kwon 2012-01-29 22:12:45 PST
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.
Comment 41 Kihong Kwon 2012-01-30 00:14:28 PST
Created attachment 124502 [details]
Fixed patch for the comment #40
Comment 42 Adam Barth 2012-02-08 10:55:56 PST
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 43 Adam Barth 2012-02-08 10:58:01 PST
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 44 Kentaro Hara 2012-02-08 14:24:31 PST
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.
Comment 45 Adam Barth 2012-02-08 14:25:28 PST
> 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?
Comment 46 Kentaro Hara 2012-02-08 15:40:52 PST
(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)
Comment 47 Kentaro Hara 2012-02-08 20:49:29 PST
(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)
Comment 48 Kihong Kwon 2012-02-09 00:27:31 PST
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.
Comment 49 Kihong Kwon 2012-02-17 23:20:52 PST
Created attachment 127696 [details]
Patch (Adopt Bug 78085 style).
Comment 50 Hajime Morrita 2012-02-19 17:57:50 PST
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 51 Hajime Morrita 2012-02-19 18:05:37 PST
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.
Comment 52 Kihong Kwon 2012-02-19 18:17:19 PST
(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 53 Kihong Kwon 2012-02-19 22:43:35 PST
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.
Comment 54 Soo-Hyun Choi 2012-02-20 00:17:57 PST
(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?
Comment 55 Hajime Morrita 2012-02-20 01:50:07 PST
> 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.
Comment 56 Kihong Kwon 2012-02-20 02:27:40 PST
Created attachment 127786 [details]
Patch.
Comment 57 Kihong Kwon 2012-02-20 02:58:25 PST
Created attachment 127794 [details]
Patch.
Comment 58 Hajime Morrita 2012-02-20 15:50:01 PST
Comment on attachment 127794 [details]
Patch.

Thanks for update! It's time to land this...
Comment 59 WebKit Review Bot 2012-02-20 17:04:58 PST
Comment on attachment 127794 [details]
Patch.

Clearing flags on attachment: 127794

Committed r108272: <http://trac.webkit.org/changeset/108272>
Comment 60 WebKit Review Bot 2012-02-20 17:05:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 61 Kihong Kwon 2012-02-20 18:33:33 PST
I appreciate all the reviews in this patch. My special thanks goes to Kenneth, Adam, Scott, Morrita, and Kentaro.