Bug 91371 – [EFL][WK2] Add vibration client

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Sudarsana Nagineni (babu)]

Implement Vibration client for WebKit2 EFL.

[Sudarsana Nagineni (babu)]

Created attachment 152531 [details]
Patch

This is dependent on patch in bug 91081.

[Sudarsana Nagineni (babu)]

Created attachment 153491 [details]
Patch

Implement vibration provider for WebKit2 EFL and also add an API to set vibration client callbacks by application. The callbacks set by application will be called back when the controller calls vibration APIs. This way application developers can handle the device feedback implementation in their application based on the platform support.

[Chris Dumez]

Comment on attachment 153491 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153491&action=review

> Source/WebKit2/UIProcess/API/efl/VibrationProvider.cpp:46
> +    _Ewk_Vibration_Client(Ewk_Vibration_Client_Vibrate_Cb vibrate, Ewk_Vibration_Client_Vibration_Cancel_Cb cancelVibration, void *userData)

Star on the wrong side.

> Source/WebKit2/UIProcess/API/efl/VibrationProvider.cpp:103
> +void VibrationProvider::setVibrationClientCallbacks(Ewk_Vibration_Client_Vibrate_Cb vibrate, Ewk_Vibration_Client_Vibration_Cancel_Cb cancelVibration, void *data)

star on the wrong side.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:96
> +void ewk_context_vibration_client_callbacks_set(const Ewk_Context* ewkContext, Ewk_Vibration_Client_Vibrate_Cb vibrate, Ewk_Vibration_Client_Vibration_Cancel_Cb cancel, void *data)

This is a setter, the ewkContext argument should not be const. + star on the wrong side for user data.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:63
> +typedef void (*Ewk_Vibration_Client_Vibrate_Cb)(uint64_t vibration_time, void *user_data);

I think we usually put the callback typdefs at the top of the file, before all the functions.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:74
> + * vibration in the clint appliation when the content ask for vibration.

"application", "client", "asks"

[Sudarsana Nagineni (babu)]

Created attachment 153500 [details]
Patch

Fix nits spotted by Chris. Thanks Chris!

[Chris Dumez]

Comment on attachment 153500 [details]
Patch

LGTM.

[Gyuyoung Kim]

Comment on attachment 153500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153500&action=review

> Source/WebKit2/UIProcess/API/efl/VibrationProvider.h:48
> +    VibrationProvider(WKVibrationRef);

Missing *explicit* keyword.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:98
> +    EINA_SAFETY_ON_NULL_RETURN(ewkContext);

I wonder if *cancel* and *data* parameters can be NULL. If other functions are not handle this, is it better to check if they are NULL ?

[Chris Dumez]

Comment on attachment 153500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153500&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:98
>> +    EINA_SAFETY_ON_NULL_RETURN(ewkContext);
> 
> I wonder if *cancel* and *data* parameters can be NULL. If other functions are not handle this, is it better to check if they are NULL ?

data is "user data" so it may be NULL for sure (it is even documented). It will work fine if the callbacks are NULL but it should be documented in the header. It is actually useful to stop listening for vibration events (you call the function again and pass NULL for the callbacks).

[Sudarsana Nagineni (babu)]

Comment on attachment 153500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153500&action=review

>>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:98
>>> +    EINA_SAFETY_ON_NULL_RETURN(ewkContext);
>> 
>> I wonder if *cancel* and *data* parameters can be NULL. If other functions are not handle this, is it better to check if they are NULL ?
> 
> data is "user data" so it may be NULL for sure (it is even documented). It will work fine if the callbacks are NULL but it should be documented in the header. It is actually useful to stop listening for vibration events (you call the function again and pass NULL for the callbacks).

You are right, okay to pass NULL for the callbacks. I'm going to update the documentation in the header.

[Sudarsana Nagineni (babu)]

Created attachment 153558 [details]
Patch

Fix review comments.

[Chris Dumez]

Comment on attachment 153558 [details]
Patch

LGTM.

[Gyuyoung Kim]

CC'ing Grzegorz, Could you take a look this patch ?

[Gyuyoung Kim]

Comment on attachment 153558 [details]
Patch

LGTM.

[Sudarsana Nagineni (babu)]

Created attachment 153865 [details]
Patch

Rebased.

[WebKit Review Bot]

Comment on attachment 153865 [details]
Patch

Clearing flags on attachment: 153865

Committed r123482: <http://trac.webkit.org/changeset/123482>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.