Bug 75156

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

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

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

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.

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

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

(In reply to comment #7) > Source/WebKit/efl/WebCoreSupport/VibratorClientEfl.h:43 > + ~VibratorClientEfl() { } Delete the destructor or use virtual keyword.

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

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

Created attachment 121259 [details] Tidy code up.

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

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

This patch is combined with bug 72010.