Now that GTK+ has support for gestures, we can use it to handle touch events and allow to scroll, zoom and tap with the fingers.
Created attachment 240005 [details] Patch For now only drag, zoom and tap are supported, but at least it makes webkitgtk+ usable on touch screens
It doesn't apply because it depends on patch attached to bug #137735.
Created attachment 240009 [details] Rebased after r174817
Created attachment 240012 [details] Fix the build with GTK+ < 3.14
Comment on attachment 240012 [details] Fix the build with GTK+ < 3.14 View in context: https://bugs.webkit.org/attachment.cgi?id=240012&action=review I wonder if this allows us to pass any new tests. Otherwise I'd have to trust you on the gtk touch specific code. Have you double-checked the implementation with Garnacho? > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:898 > +#endif We should change this name as we need to make explicit the fact that it might create the GestureController. What about webkitWebViewBaseGetOrCreateGestureController ? > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:79 > +WebKit::GestureController& webkitWebViewBaseGestureController(WebKitWebViewBase*); See above. > Source/WebKit2/UIProcess/gtk/GestureController.cpp:44 > + return std::unique_ptr<GestureController>(new GestureController(page)); I think the preferred form is std::make_unique<GestureController>(page); > Source/WebKit2/UIProcess/gtk/GestureController.cpp:133 > + int delay; Nit, uint > Source/WebKit2/UIProcess/gtk/GestureController.cpp:137 > + }, std::chrono::milliseconds(delay)); We should convert this to microseconds after r174818
(In reply to comment #5) > Comment on attachment 240012 [details] > Fix the build with GTK+ < 3.14 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240012&action=review Thanks for the review. > I wonder if this allows us to pass any new tests. I haven't checked, will run touch tests. > Otherwise I'd have to > trust you on the gtk touch specific code. Have you double-checked the > implementation with Garnacho? Yes. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:898 > > +#endif > > We should change this name as we need to make explicit the fact that it > might create the GestureController. What about > webkitWebViewBaseGetOrCreateGestureController ? Why does it have to be explicit? The caller doesn't need to know anything since it's receiving a reference. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:79 > > +WebKit::GestureController& webkitWebViewBaseGestureController(WebKitWebViewBase*); > > See above. > > > Source/WebKit2/UIProcess/gtk/GestureController.cpp:44 > > + return std::unique_ptr<GestureController>(new GestureController(page)); > > I think the preferred form is std::make_unique<GestureController>(page); It doesn't build. > > Source/WebKit2/UIProcess/gtk/GestureController.cpp:133 > > + int delay; > > Nit, uint gtk-long-press-time is a guint. > > Source/WebKit2/UIProcess/gtk/GestureController.cpp:137 > > + }, std::chrono::milliseconds(delay)); > > We should convert this to microseconds after r174818 No, this is already in milliseconds.
(In reply to comment #6) > > > Source/WebKit2/UIProcess/gtk/GestureController.cpp:44 > > > + return std::unique_ptr<GestureController>(new GestureController(page)); > > > > I think the preferred form is std::make_unique<GestureController>(page); > > It doesn't build. I remember why now, because it requires the constructor to be public. It was private to make sure the class can only be created using create, but I'll make it public anyway to use make_unique
Created attachment 240022 [details] Addressed review comments Use unsigned instead of int and make_unique
Created attachment 240023 [details] Updated patch Just removed an unneeded header include in WebKitWebViewBase.cpp that I added by mistake.
(In reply to comment #5) > Comment on attachment 240012 [details] > Fix the build with GTK+ < 3.14 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240012&action=review > > I wonder if this allows us to pass any new tests. Otherwise I'd have to > trust you on the gtk touch specific code. Have you double-checked the > implementation with Garnacho? > This will not affect the tests anyway, because it requires GTK+ 3.14 and bots use 3.6.
Comment on attachment 240023 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=240023&action=review Let's give it a try! The only thing we need to address before landing is the misunderstanding related to the factory method and the constructor. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:894 > + priv->gestureController = GestureController::create(*priv->pageProxy); We should call the constructor here: priv->gestureController = std::make_unique<GestureController>(*priv->pageProxy); See comment below for the rationale. > Source/WebKit2/UIProcess/gtk/GestureController.cpp:52 > + Sorry if I didn't explain myself correctly, but this is not what I was asking about. This indeed looks very weird (a public factory and a public constructor). What I think we should do is to make the constructor public and use make_unique<> in the callers. That's something Anders is actively pushing for new code, see: https://bugs.webkit.org/show_bug.cgi?id=137765#c13 > Source/WebKit2/UIProcess/gtk/GestureController.h:47 > + static std::unique_ptr<GestureController> create(WebPageProxy&); We should get rid of this. > Source/WebKit2/UIProcess/gtk/GestureController.h:49 > + explicit GestureController(WebPageProxy&); And the explicit won't be required I think...
(In reply to comment #11) > Comment on attachment 240023 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240023&action=review > > Let's give it a try! The only thing we need to address before landing is the > misunderstanding related to the factory method and the constructor. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:894 > > + priv->gestureController = GestureController::create(*priv->pageProxy); > > We should call the constructor here: > > priv->gestureController = > std::make_unique<GestureController>(*priv->pageProxy); > > See comment below for the rationale. > > > Source/WebKit2/UIProcess/gtk/GestureController.cpp:52 > > + > > Sorry if I didn't explain myself correctly, but this is not what I was > asking about. This indeed looks very weird (a public factory and a public > constructor). What I think we should do is to make the constructor public > and use make_unique<> in the callers. That's something Anders is actively > pushing for new code, see: https://bugs.webkit.org/show_bug.cgi?id=137765#c13 Ok, understood.
Committed r174880: <http://trac.webkit.org/changeset/174880>