Bug 72010 - (VibrationAPI) Support for Vibration API
(VibrationAPI)
: Support for Vibration API
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: All All
: P3 Enhancement
Assigned To:
: http://www.w3.org/TR/2012/WD-vibratio...
:
: 78085 78210
: 75156 78947 84386
  Show dependency treegraph
 
Reported: 2011-11-10 03:39 PST by
Modified: 2012-04-19 14:46 PST (History)


Attachments
Core part for the Vibration API (3.60 KB, patch)
2011-11-10 04:05 PST, Kihong Kwon
abarth: review-
Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
WebCore patch for the vibration API (13.18 KB, patch)
2011-12-15 00:14 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Patch for the vibration API (13.70 KB, patch)
2011-12-15 01:03 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Patch for the vibration API (15.01 KB, patch)
2011-12-22 01:39 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Patch for the vibration API (15.10 KB, patch)
2011-12-22 20:47 PST, Kihong Kwon
no flags Review Patch | 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-
Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Tidy code up. (20.29 KB, patch)
2012-01-05 04:10 PST, Kihong Kwon
morrita: review-
Review Patch | 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-
Review Patch | 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-
Review Patch | 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-
Review Patch | Details | Formatted Diff | Diff
Fix style and cr-linux error (24.73 KB, patch)
2012-01-27 01:56 PST, Kihong Kwon
no flags Review Patch | 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-
Review Patch | Details | Formatted Diff | Diff
Fixed patch for the comment #34 (27.98 KB, patch)
2012-01-29 21:58 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Fixed patch for the comment #40 (27.92 KB, patch)
2012-01-30 00:14 PST, Kihong Kwon
abarth: review-
Review Patch | 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-
Review Patch | Details | Formatted Diff | Diff
Patch. (31.93 KB, patch)
2012-02-20 02:27 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff
Patch. (31.99 KB, patch)
2012-02-20 02:58 PST, Kihong Kwon
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-11-10 04:05:26 PST -------
Created an attachment (id=114467) [details]
Core part for the Vibration API
------- Comment #2 From 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 From 2011-11-10 06:41:28 PST -------
(From update of attachment 114467 [details])
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 From 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 From 2011-11-10 09:14:02 PST -------
(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?

> 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 From 2011-11-17 23:42:43 PST -------
Created an attachment (id=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 From 2011-11-17 23:46:22 PST -------
(In reply to comment #5)
> (From update of attachment 114467 [details] [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 From 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 From 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 From 2011-12-15 00:14:37 PST -------
Created an attachment (id=119388) [details]
WebCore patch for the vibration API
------- Comment #11 From 2011-12-15 01:03:00 PST -------
Created an attachment (id=119395) [details]
Patch for the vibration API
------- Comment #12 From 2011-12-22 01:39:58 PST -------
Created an attachment (id=120289) [details]
Patch for the vibration API
------- Comment #13 From 2011-12-22 20:47:55 PST -------
Created an attachment (id=120432) [details]
Patch for the vibration API
------- Comment #14 From 2011-12-23 03:27:19 PST -------
(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?

> 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 From 2011-12-26 18:49:26 PST -------
(In reply to comment #14)
> (From update of attachment 120432 [details] [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 From 2011-12-29 00:48:47 PST -------
Created an attachment (id=120722) [details]
Add suspend/resume method and test case.
------- Comment #17 From 2011-12-29 01:32:02 PST -------
(From update of attachment 120722 [details])
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 From 2011-12-29 02:20:32 PST -------
Created an attachment (id=120726) [details]
Add skip test statement for the chromium port.
------- Comment #19 From 2012-01-05 04:10:05 PST -------
Created an attachment (id=121257) [details]
Tidy code up.
------- Comment #20 From 2012-01-16 00:36:20 PST -------
(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.
------- Comment #21 From 2012-01-16 00:51:37 PST -------
(In reply to comment #20)
> (From update of attachment 121257 [details] [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 From 2012-01-26 01:12:21 PST -------
Created an attachment (id=124075) [details]
Add NavigatorVibration class and move vibration files to the Modules dir.
------- Comment #23 From 2012-01-26 08:59:18 PST -------
(From update of attachment 124075 [details])
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 From 2012-01-26 22:21:11 PST -------
(From update of attachment 124075 [details])
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 From 2012-01-26 22:27:00 PST -------
Created an attachment (id=124263) [details]
Modify for the comment #23
------- Comment #26 From 2012-01-26 23:24:39 PST -------
(From update of attachment 124263 [details])
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 From 2012-01-27 00:29:20 PST -------
Created an attachment (id=124267) [details]
Add test case for ignoring for cr-linux.
------- Comment #28 From 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 From 2012-01-27 01:05:59 PST -------
(From update of attachment 124267 [details])
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 From 2012-01-27 01:56:26 PST -------
Created an attachment (id=124279) [details]
Fix style and cr-linux error
------- Comment #31 From 2012-01-27 04:22:49 PST -------
(From update of attachment 124267 [details])
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 From 2012-01-27 18:32:17 PST -------
Created an attachment (id=124419) [details]
Add navigator-vibration.html to the skipped lists.
------- Comment #33 From 2012-01-27 19:39:28 PST -------
(From update of attachment 124419 [details])
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 From 2012-01-28 05:49:06 PST -------
(From update of attachment 124419 [details])
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 From 2012-01-29 20:22:56 PST -------
(From update of attachment 124419 [details])
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 From 2012-01-29 20:38:03 PST -------
(From update of attachment 124419 [details])
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 From 2012-01-29 21:02:07 PST -------
(From update of attachment 124419 [details])
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 From 2012-01-29 21:58:36 PST -------
Created an attachment (id=124495) [details]
Fixed patch for the comment #34
------- Comment #39 From 2012-01-29 22:06:52 PST -------
(From update of attachment 124495 [details])
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 From 2012-01-29 22:12:45 PST -------
(From update of attachment 124495 [details])
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 From 2012-01-30 00:14:28 PST -------
Created an attachment (id=124502) [details]
Fixed patch for the comment #40
------- Comment #42 From 2012-02-08 10:55:56 PST -------
(From update of attachment 124502 [details])
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 From 2012-02-08 10:58:01 PST -------
(From update of attachment 124502 [details])
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 From 2012-02-08 14:24:31 PST -------
(From update of attachment 124502 [details])
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 From 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 From 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 From 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 From 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 From 2012-02-17 23:20:52 PST -------
Created an attachment (id=127696) [details]
Patch (Adopt Bug 78085 style).
------- Comment #50 From 2012-02-19 17:57:50 PST -------
(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.
------- Comment #51 From 2012-02-19 18:05:37 PST -------
(From update of attachment 127696 [details])
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 From 2012-02-19 18:17:19 PST -------
(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?
------- Comment #53 From 2012-02-19 22:43:35 PST -------
(From update of attachment 127696 [details])
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 From 2012-02-20 00:17:57 PST -------
(In reply to comment #52)
> (In reply to comment #50)
> > (From update of attachment 127696 [details] [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 From 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 From 2012-02-20 02:27:40 PST -------
Created an attachment (id=127786) [details]
Patch.
------- Comment #57 From 2012-02-20 02:58:25 PST -------
Created an attachment (id=127794) [details]
Patch.
------- Comment #58 From 2012-02-20 15:50:01 PST -------
(From update of attachment 127794 [details])
Thanks for update! It's time to land this...
------- Comment #59 From 2012-02-20 17:04:58 PST -------
(From update of attachment 127794 [details])
Clearing flags on attachment: 127794

Committed r108272: <http://trac.webkit.org/changeset/108272>
------- Comment #60 From 2012-02-20 17:05:09 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #61 From 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.