Bug 91371

Summary:

[EFL][WK2] Add vibration client

Product:

WebKit

Reporter:

Sudarsana Nagineni (babu) <naginenis>

Component:

WebKit EFL

Assignee:

Sudarsana Nagineni (babu) <naginenis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

g.czajkowski, gyuyoung.kim, gyuyoung.kim, kihong.kwon, lucas.de.marchi, rakuco, sw0524.lee, vimff0, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

91077

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Sudarsana Nagineni (babu)

Reported 2012-07-16 03:30:06 PDT

Implement Vibration client for WebKit2 EFL.

Attachments
Patch (8.93 KB, patch) 2012-07-16 07:37 PDT, Sudarsana Nagineni (babu)	no flags	Details Formatted Diff Diff
Patch (12.51 KB, patch) 2012-07-20 06:37 PDT, Sudarsana Nagineni (babu)	no flags	Details Formatted Diff Diff
Patch (12.66 KB, patch) 2012-07-20 07:17 PDT, Sudarsana Nagineni (babu)	no flags	Details Formatted Diff Diff
Patch (12.83 KB, patch) 2012-07-20 12:10 PDT, Sudarsana Nagineni (babu)	no flags	Details Formatted Diff Diff
Patch (13.05 KB, patch) 2012-07-23 14:45 PDT, Sudarsana Nagineni (babu)	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Sudarsana Nagineni (babu)

Comment 1 2012-07-16 07:37:32 PDT

Created attachment 152531 [details] Patch This is dependent on patch in bug 91081.

Sudarsana Nagineni (babu)

Comment 2 2012-07-20 06:37:59 PDT

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 3 2012-07-20 06:49:20 PDT

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)

Comment 4 2012-07-20 07:17:45 PDT

Created attachment 153500 [details] Patch Fix nits spotted by Chris. Thanks Chris!

Chris Dumez

Comment 5 2012-07-20 07:20:37 PDT

Comment on attachment 153500 [details] Patch LGTM.

Gyuyoung Kim

Comment 6 2012-07-20 08:11:34 PDT

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 7 2012-07-20 08:17:24 PDT

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 8 2012-07-20 11:27:21 PDT

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)

Comment 9 2012-07-20 12:10:40 PDT

Created attachment 153558 [details] Patch Fix review comments.

Chris Dumez

Comment 10 2012-07-20 12:12:09 PDT

Comment on attachment 153558 [details] Patch LGTM.

Gyuyoung Kim

Comment 11 2012-07-21 17:43:26 PDT

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

Gyuyoung Kim

Comment 12 2012-07-23 01:33:39 PDT

Comment on attachment 153558 [details] Patch LGTM.