Bug 75156 – [EFL] Add the Vibration API to the WebKit-EFL

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

[Kihong Kwon]

Efl port implementation for the Vibration API

[Kihong Kwon]

Created attachment 120422 [details]
Efl port for the Vibration API feature.

[WebKit Review Bot]

Comment on attachment 120422 [details]
Efl port for the Vibration API feature.

Attachment 120422 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10955148

New failing tests:
http/tests/inspector/resource-tree/resource-tree-document-url.html

[Ryuan Choi]

Comment on attachment 120422 [details]
Efl port for the Vibration API feature.

> Source/WebKit/efl/CMakeListsEfl.txt:135
> +IF (ENABLE_VIBRATION)
> +  LIST(APPEND WebKit_SOURCES
> +    efl/WebCoreSupport/VibratorClientEfl.cpp
> +    efl/ewk/ewk_vibrator.cpp
> +  )
> +ENDIF ()

If I am right, our CMake files use 4 spaces indentation without this file.

I'm not sure whether we need to fix this file because of history.
If we need, I'll create a bug.

Anyway, I like that this patch follow our CMake indentation.

> Source/WebKit/efl/WebCoreSupport/VibratorClientEfl.cpp:33
> +{

how about adding ASSERT(m_view); ?

> Source/WebKit/efl/WebCoreSupport/VibratorClientEfl.h:20
> +*/
> +
> +

IMO, one empty line is enough.

> Source/WebKit/efl/ewk/ewk_view.cpp:614
> +#if ENABLE(VIBRATION)
> +    pageClients.vibratorClient = new WebCore::VibratorClientEfl(smartData->self);

Who remove vibratorClient ?

[Kihong Kwon]

(In reply to comment #3)
> (From update of attachment 120422 [details] [details])
> > Source/WebKit/efl/CMakeListsEfl.txt:135
> > +IF (ENABLE_VIBRATION)
> > +  LIST(APPEND WebKit_SOURCES
> > +    efl/WebCoreSupport/VibratorClientEfl.cpp
> > +    efl/ewk/ewk_vibrator.cpp
> > +  )
> > +ENDIF ()
> 
> If I am right, our CMake files use 4 spaces indentation without this file.
> 
> I'm not sure whether we need to fix this file because of history.
> If we need, I'll create a bug.
> 
> Anyway, I like that this patch follow our CMake indentation.
> 

If you make a bug for indentation problem about this file, It's good.
But, If I modify this from 2 spaces indentation to 4, It's ridiculous, because every other lines have 2 spaces indentation.
Is it no problem which is different between this patch and other lines in this file?

> > Source/WebKit/efl/WebCoreSupport/VibratorClientEfl.cpp:33
> > +{
> 
> how about adding ASSERT(m_view); ?
> 
> > Source/WebKit/efl/WebCoreSupport/VibratorClientEfl.h:20
> > +*/
> > +
> > +
> 
> IMO, one empty line is enough.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:614
> > +#if ENABLE(VIBRATION)
> > +    pageClients.vibratorClient = new WebCore::VibratorClientEfl(smartData->self);
> 
> Who remove vibratorClient ?

[Tomasz Morawski]

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

> ChangeLog:5
> +        http://dev.w3.org/2009/dap/vibration/

This line should be moved to new line. You could also add a short description about changes in the following files.

> Source/WebKit/efl/ChangeLog:5
> +        http://dev.w3.org/2009/dap/vibration/

This line could be moved to description section.

> Source/WebKit/efl/ChangeLog:14
> +

Missing some description about changes in the following files.

> Source/WebKit/efl/WebCoreSupport/VibratorClientEfl.cpp:36
> +VibratorClientEfl::~VibratorClientEfl()

What about moving this to header file (if you really need to have protected destructor)? Since it is an empty destructor?

> Source/WebKit/efl/WebCoreSupport/VibratorClientEfl.h:23
> +

This class should be inside of #if ENABLE(VIBRATION) block.

> Source/WebKit/efl/WebCoreSupport/VibratorClientEfl.h:41
> +

virtual keyword is missing. Anyway who and when is delete this object? Why this destructor is protected?

> Source/cmake/OptionsEfl.cmake:96
> +WEBKIT_FEATURE(ENABLE_VIBRATION "Enable vibration" DEFAULT ON)

I think this feature should be disabled by default. This is generic WebKit EFL profile that is mainly used on desktops.

[Tomasz Morawski]

(In reply to comment #5)
> virtual keyword is missing. Anyway who and when is delete this object? Why this destructor is protected?
OK, I see that that object is destroyed in 72010 patch. But the "destroyed" function is missing for VibratorClientEfl class.

[Kihong Kwon]

Created attachment 120441 [details]
Fixed patch for all comments.

[Tomasz Morawski]

(In reply to comment #7)
> Source/WebKit/efl/WebCoreSupport/VibratorClientEfl.h:43
> +    ~VibratorClientEfl() { }

Delete the destructor or use virtual keyword.

[Kihong Kwon]

Created attachment 120442 [details]
Fixed patch for Comment #8

[Kihong Kwon]

Created attachment 120723 [details]
Fix patch because of webcore patch.

[Kihong Kwon]

Created attachment 121259 [details]
Tidy code up.

[Kihong Kwon]

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

[Gyuyoung Kim]

Comment on attachment 127763 [details]
Patch (Adopt Bug 78085 style).

Attachment 127763 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11552038

[Kihong Kwon]

This patch is combined with bug 72010.