Bug 75156 - [EFL] Add the Vibration API to the WebKit-EFL
Summary: [EFL] Add the Vibration API to the WebKit-EFL
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL: http://dev.w3.org/2009/dap/vibration/
Keywords:
Depends on: VibrationAPI
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-22 18:18 PST by Kihong Kwon
Modified: 2012-03-02 00:27 PST (History)
9 users (show)

See Also:


Attachments
Efl port for the Vibration API feature. (9.94 KB, patch)
2011-12-22 18:39 PST, Kihong Kwon
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fixed patch for all comments. (10.23 KB, patch)
2011-12-23 00:47 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Fixed patch for Comment #8 (10.24 KB, patch)
2011-12-23 01:29 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Fix patch because of webcore patch. (9.78 KB, patch)
2011-12-29 00:54 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Tidy code up. (9.75 KB, patch)
2012-01-05 04:15 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff
Patch (Adopt Bug 78085 style). (9.61 KB, patch)
2012-02-19 22:42 PST, Kihong Kwon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kihong Kwon 2011-12-22 18:18:01 PST
Efl port implementation for the Vibration API
Comment 1 Kihong Kwon 2011-12-22 18:39:35 PST
Created attachment 120422 [details]
Efl port for the Vibration API feature.
Comment 2 WebKit Review Bot 2011-12-22 19:33:11 PST
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
Comment 3 Ryuan Choi 2011-12-22 19:36:53 PST
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 ?
Comment 4 Kihong Kwon 2011-12-22 20:38:47 PST
(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 ?
Comment 5 Tomasz Morawski 2011-12-22 23:56:03 PST
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.
Comment 6 Tomasz Morawski 2011-12-23 00:06:54 PST
(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.
Comment 7 Kihong Kwon 2011-12-23 00:47:09 PST
Created attachment 120441 [details]
Fixed patch for all comments.
Comment 8 Tomasz Morawski 2011-12-23 01:15:24 PST
(In reply to comment #7)
> Source/WebKit/efl/WebCoreSupport/VibratorClientEfl.h:43
> +    ~VibratorClientEfl() { }

Delete the destructor or use virtual keyword.
Comment 9 Kihong Kwon 2011-12-23 01:29:32 PST
Created attachment 120442 [details]
Fixed patch for Comment #8
Comment 10 Kihong Kwon 2011-12-29 00:54:35 PST
Created attachment 120723 [details]
Fix patch because of webcore patch.
Comment 11 Kihong Kwon 2012-01-05 04:15:16 PST
Created attachment 121259 [details]
Tidy code up.
Comment 12 Kihong Kwon 2012-02-19 22:42:50 PST
Created attachment 127763 [details]
Patch (Adopt Bug 78085 style).
Comment 13 Gyuyoung Kim 2012-02-19 23:20:54 PST
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
Comment 14 Kihong Kwon 2012-02-20 16:10:59 PST
This patch is combined with bug 72010.