Summary: | [GTK] Implement connected and disconnected events of GAMEPAD API with libmanette | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michal Debski <m.debski> | ||||||||||||||||||||||
Component: | New Bugs | Assignee: | ChangSeok Oh <changseok> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | agomez, alex, annulen, aperez, bugs-noreply, cgarcia, changseok, clopez, don.olmstead, ews-watchlist, gyuyoung.kim, ltilve, pnormand, ryuan.choi, sergio, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 133850 | ||||||||||||||||||||||||
Bug Blocks: | 133847 | ||||||||||||||||||||||||
Attachments: |
|
Description
Michal Debski
2014-06-13 06:57:21 PDT
Created attachment 233594 [details]
Patch
Created attachment 395125 [details]
Patch
> src/meson.build:116:6:
> : Program(s) ['vapigen'] not found or not executable
The build bot seems to not have vala package..
Created attachment 395133 [details]
Patch
(In reply to ChangSeok Oh from comment #3) > > src/meson.build:116:6: > > : Program(s) ['vapigen'] not found or not executable > > The build bot seems to not have vala package.. Please disable that and the "introspection" options in the meson build. Created attachment 395149 [details]
Patch
Comment on attachment 395149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395149&action=review > Tools/gtk/jhbuild.modules:449 > + <meson id="manette" mesonargs="-Dintrospection=false"> -Dvapi=false too please :) Created attachment 395150 [details]
Patch
(In reply to Philippe Normand from comment #7) > Comment on attachment 395149 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395149&action=review > > > Tools/gtk/jhbuild.modules:449 > > + <meson id="manette" mesonargs="-Dintrospection=false"> > > -Dvapi=false too please :) Thanks! Created attachment 395240 [details]
Patch
Comment on attachment 395240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395240&action=review Any new Find modules should expose a target see https://github.com/WebKit/webkit/blob/master/Source/cmake/FindOpenJPEG.cmake for something you should be able to copy pasta for the most part. > Source/cmake/FindManette.cmake:49 > +find_package(PkgConfig) > +pkg_check_modules(PC_MANETTE manette-0.2) > + > +find_path(MANETTE_INCLUDE_DIRS > + NAMES libmanette.h > + HINTS ${PC_MANETTE_INCLUDEDIR} > + ${PC_MANETTE_INCLUDE_DIRS} > +) > + > +find_library(MANETTE_LIBRARIES > + NAMES manette-0.2 > + HINTS ${PC_MANETTE_LIBDIR} > + ${PC_MANETTE_LIBRARY_DIRS} > +) > + > +include(FindPackageHandleStandardArgs) > +find_package_handle_standard_args(Manette > + FOUND_VAR MANETTE_FOUND > + REQUIRED_VARS MANETTE_INCLUDE_DIRS MANETTE_LIBRARIES > +) Please add a proper target in CMake. Created attachment 396169 [details]
Patch
(In reply to Don Olmstead from comment #11) > Comment on attachment 395240 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395240&action=review > > Any new Find modules should expose a target see > https://github.com/WebKit/webkit/blob/master/Source/cmake/FindOpenJPEG.cmake > for something you should be able to copy pasta for the most part. > > > Source/cmake/FindManette.cmake:49 > > +find_package(PkgConfig) > > +pkg_check_modules(PC_MANETTE manette-0.2) > > + > > +find_path(MANETTE_INCLUDE_DIRS > > + NAMES libmanette.h > > + HINTS ${PC_MANETTE_INCLUDEDIR} > > + ${PC_MANETTE_INCLUDE_DIRS} > > +) > > + > > +find_library(MANETTE_LIBRARIES > > + NAMES manette-0.2 > > + HINTS ${PC_MANETTE_LIBDIR} > > + ${PC_MANETTE_LIBRARY_DIRS} > > +) > > + > > +include(FindPackageHandleStandardArgs) > > +find_package_handle_standard_args(Manette > > + FOUND_VAR MANETTE_FOUND > > + REQUIRED_VARS MANETTE_INCLUDE_DIRS MANETTE_LIBRARIES > > +) > > Please add a proper target in CMake. O.K. Comment on attachment 396169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396169&action=review This looks all good to me, module a few nits. It would not hurt to have someone else take a quick look and rubber-stamp it, though. > Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:35 > + , m_device(adoptGRef(static_cast<ManetteDevice*>(g_object_ref(device)))) You don't need to use adopGRef() + g_object_ref() by here, you can pass the pointer to the GRefPtr constructor and it will increase the reference count automatically. The following should be enough: ManetteGamePad(ManetteDevice* device, unsigned index): : PlatformGamepad(index) , m_device(device) { } =) > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:104 > + std::unique_ptr<ManetteGamepad> gamepad = makeUnique<ManetteGamepad>(device, index); You could use “auto” for the variable type here, because it is clear from the right side of the assignment which type it will have. > Source/cmake/FindManette.cmake:56 > +pkg_check_modules(PC_MANETTE manette-0.2) I would add the IMPORTED_TARGET flag here, and all lines from from 58 to 71 will not be needed. > Source/cmake/FindManette.cmake:87 > +endif () …and here use the PkgConfig::PC_MANETTE target: if (TARGET PkgConfig::PC_MANETTE AND NOT TARGET Manette::Manette) add_library(Manette::Manette INTERFACE IMPORTED GLOBAL) set_property(TARGET Manette::Manette INTERFACE_LINK_LIBRARIES PkgConfig::PC_MANETTE) endif () This is what we currently do in Source/cmake/FindGTK.cmake :) > Source/cmake/FindManette.cmake:91 > +if (Manette_FOUND) If the PkgConfig target is used, this if-block is not needed. Comment on attachment 396169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396169&action=review > Source/WebCore/platform/gamepad/manette/ManetteGamepad.h:38 > +class ManetteGamepad : public PlatformGamepad { We can mark the class final too. > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:50 > + if (provider) Can this really be nullptr? The provider is a singleton. > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:56 > + if (provider) Ditto. > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:61 > + : m_shouldDispatchCallbacks(false) This could be initialized in the header. > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:66 > + g_signal_connect(G_OBJECT(m_monitor.get()), "device-connected", G_CALLBACK(onDeviceConnected), this); > + g_signal_connect(G_OBJECT(m_monitor.get()), "device-disconnected", G_CALLBACK(onDeviceDisconnected), this); No need to cast GObject for g_signal_connect > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:76 > + if (shouldOpenAndScheduleManager) { We could early return instead. > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:166 > + auto i = m_gamepadVector.find(result.get()); i -> index > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:48 > + WEBCORE_EXPORT static ManetteGamepadProvider& singleton(); > + > + WEBCORE_EXPORT void startMonitoringGamepads(GamepadProviderClient&) final; > + WEBCORE_EXPORT void stopMonitoringGamepads(GamepadProviderClient&) final; WEBCORE_EXPORT macros aren't needed for GTK port. > Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:46 > + if (WEBKIT_IS_WEB_VIEW(widget) && gtk_widget_is_visible(widget)) > + return &webkitWebViewGetPage(WEBKIT_WEB_VIEW(widget)); In case of being a WebView and not visible, I guess we want to return nullptr, not iterate the web view children, no? > Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:58 > + ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer)); What is this? > Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:62 > + if (!GTK_IS_WINDOW(iter->data)) WebCore::widgetIsOnscreenToplevelWindow() Comment on attachment 396169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396169&action=review Thanks for your review! A new patch is coming. >> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:35 >> + , m_device(adoptGRef(static_cast<ManetteDevice*>(g_object_ref(device)))) > > You don't need to use adopGRef() + g_object_ref() by here, you can pass the > pointer to the GRefPtr constructor and it will increase the reference count > automatically. The following should be enough: > > ManetteGamePad(ManetteDevice* device, unsigned index): > : PlatformGamepad(index) > , m_device(device) > { > } > > =) Good to know. Thanks. >> Source/WebCore/platform/gamepad/manette/ManetteGamepad.h:38 >> +class ManetteGamepad : public PlatformGamepad { > > We can mark the class final too. O.K. >> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:50 >> + if (provider) > > Can this really be nullptr? The provider is a singleton. No. the if condition is removed. >> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:56 >> + if (provider) > > Ditto. Done. >> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:61 >> + : m_shouldDispatchCallbacks(false) > > This could be initialized in the header. Moved to the header. >> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:66 >> + g_signal_connect(G_OBJECT(m_monitor.get()), "device-disconnected", G_CALLBACK(onDeviceDisconnected), this); > > No need to cast GObject for g_signal_connect G_OBJECT removed. >> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:76 >> + if (shouldOpenAndScheduleManager) { > > We could early return instead. Done. >> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:104 >> + std::unique_ptr<ManetteGamepad> gamepad = makeUnique<ManetteGamepad>(device, index); > > You could use “auto” for the variable type here, because it is clear > from the right side of the assignment which type it will have. O.K. >> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:166 >> + auto i = m_gamepadVector.find(result.get()); > > i -> index Fixed. >> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:48 >> + WEBCORE_EXPORT void stopMonitoringGamepads(GamepadProviderClient&) final; > > WEBCORE_EXPORT macros aren't needed for GTK port. Removed. >> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:46 >> + return &webkitWebViewGetPage(WEBKIT_WEB_VIEW(widget)); > > In case of being a WebView and not visible, I guess we want to return nullptr, not iterate the web view children, no? You are right. Let me change the return statement to 'return gtk_widget_is_visible(widget) ? &webkitWebViewGetPage(WEBKIT_WEB_VIEW(widget)) : nullptr;' >> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:58 >> + ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer)); > > What is this? That is a security check in process level for mac port but I blindly copied it here. Let's remove it. >> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:62 >> + if (!GTK_IS_WINDOW(iter->data)) > > WebCore::widgetIsOnscreenToplevelWindow() Great! Thanks. >> Source/cmake/FindManette.cmake:56 >> +pkg_check_modules(PC_MANETTE manette-0.2) > > I would add the IMPORTED_TARGET flag here, and all lines from > from 58 to 71 will not be needed. Wow! I didn't know this macro. It folds many lines up. Thanks. >> Source/cmake/FindManette.cmake:87 >> +endif () > > …and here use the PkgConfig::PC_MANETTE target: > > if (TARGET PkgConfig::PC_MANETTE AND NOT TARGET Manette::Manette) > add_library(Manette::Manette INTERFACE IMPORTED GLOBAL) > set_property(TARGET Manette::Manette INTERFACE_LINK_LIBRARIES PkgConfig::PC_MANETTE) > endif () > > This is what we currently do in Source/cmake/FindGTK.cmake :) Done. >> Source/cmake/FindManette.cmake:91 >> +if (Manette_FOUND) > > If the PkgConfig target is used, this if-block is not needed. 👍 Created attachment 396900 [details]
Patch
@philn, could you bump up the version of libmanette to 0.2.4 for webkit flatpak sdk? https://ftp.gnome.org/pub/gnome/sources/libmanette/0.2/ (In reply to ChangSeok Oh from comment #18) > @philn, could you bump up the version of libmanette to 0.2.4 for webkit > flatpak sdk? https://ftp.gnome.org/pub/gnome/sources/libmanette/0.2/ It's already in the SDK. Make sure you use the latest version: $ webkit-flatpak --command=bash [📦 org.webkit.Webkit WebKit]$ pkg-config --modversion manette-0.2 0.2.4 (In reply to Philippe Normand from comment #19) > (In reply to ChangSeok Oh from comment #18) > > @philn, could you bump up the version of libmanette to 0.2.4 for webkit > > flatpak sdk? https://ftp.gnome.org/pub/gnome/sources/libmanette/0.2/ > > It's already in the SDK. Make sure you use the latest version: > > $ webkit-flatpak --command=bash > [📦 org.webkit.Webkit WebKit]$ pkg-config --modversion manette-0.2 > 0.2.4 How fast your are! Thanks. That said, the new patch is not guilty to the red window port. Any comment or r+? Created attachment 399815 [details]
Patch
Rebase
Comment on attachment 399815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399815&action=review > Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:31 > +#include "WebKitWebViewPrivate.h" Better use WebKitWebViewBase Comment on attachment 399815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399815&action=review >> Source/WebKit/UIProcess/Gamepad/gtk/UIGamepadProviderGtk.cpp:31 >> +#include "WebKitWebViewPrivate.h" > > Better use WebKitWebViewBase You meant WebKitWebViewBasePrivate? Created attachment 399882 [details]
Patch to land
Comment on attachment 399882 [details]
Patch to land
Thanks!
Committed r261965: <https://trac.webkit.org/changeset/261965> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399882 [details]. |