Support for Vibration API

• Component: Platform
• Status: RESOLVED FIXED

[Kihong Kwon]

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.

[Kihong Kwon]

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

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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

[Adam Barth]

> Maybe this should be USE, ask abarth on irc.

ENABLE is correct.  This is a web-facing feature.

[Adam Barth]

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.

[Kihong Kwon]

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()...

[Kihong Kwon]

(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.

[Scott Graham]

(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.

[Kihong Kwon]

(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.

[Kihong Kwon]

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

[Kihong Kwon]

Created attachment 119395 [details]
Patch for the vibration API

[Kihong Kwon]

Created attachment 120289 [details]
Patch for the vibration API

[Kihong Kwon]

Created attachment 120432 [details]
Patch for the vibration API

[Kenneth Rohde Christiansen]

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

[Kihong Kwon]

(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

[Kihong Kwon]

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

[WebKit Review Bot]

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

[Kihong Kwon]

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

[Kihong Kwon]

Created attachment 121257 [details]
Tidy code up.

[Hajime Morrita]

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.

[Kentaro Hara]

(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

[Kihong Kwon]

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

[Kentaro Hara]

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?

[Kihong Kwon]

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.

[Kihong Kwon]

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

[WebKit Review Bot]

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

[Kihong Kwon]

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

[WebKit Review Bot]

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.

[Kentaro Hara]

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()".

[Kihong Kwon]

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

[Kihong Kwon]

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.

[Kihong Kwon]

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

[Kentaro Hara]

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

[Kentaro Hara]

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())"?

[Kihong Kwon]

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.

[Kentaro Hara]

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"?

[Kihong Kwon]

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.

[Kihong Kwon]

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

[Kentaro Hara]

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()?

[Kihong Kwon]

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.

[Kihong Kwon]

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

[Adam Barth]

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?

[Adam Barth]

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.

[Kentaro Hara]

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.

[Adam Barth]

> 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?

[Kentaro Hara]

(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)

[Kentaro Hara]

(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)

[Kihong Kwon]

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.

[Kihong Kwon]

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

[Hajime Morrita]

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.

[Hajime Morrita]

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.

[Kihong Kwon]

(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?

[Kihong Kwon]

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.

[Soo-Hyun Choi]

(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?

[Hajime Morrita]

> 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.

[Kihong Kwon]

Created attachment 127786 [details]
Patch.

[Kihong Kwon]

Created attachment 127794 [details]
Patch.

[Hajime Morrita]

Comment on attachment 127794 [details]
Patch.

Thanks for update! It's time to land this...

[WebKit Review Bot]

Comment on attachment 127794 [details]
Patch.

Clearing flags on attachment: 127794

Committed r108272: <http://trac.webkit.org/changeset/108272>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kihong Kwon]

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