Bug 137812 – [GTK] Add initial gestures support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 240005 [details]
Patch

For now only drag, zoom and tap are supported, but at least it makes webkitgtk+ usable on touch screens

[Carlos Garcia Campos]

It doesn't apply because it depends on patch attached to bug #137735.

[Carlos Garcia Campos]

Created attachment 240009 [details]
Rebased after r174817

[Carlos Garcia Campos]

Created attachment 240012 [details]
Fix the build with GTK+ < 3.14

[Sergio Villar Senin]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

Created attachment 240022 [details]
Addressed review comments

Use unsigned instead of int and make_unique

[Carlos Garcia Campos]

Created attachment 240023 [details]
Updated patch

Just removed an unneeded header include in WebKitWebViewBase.cpp that I added by mistake.

[Carlos Garcia Campos]

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

[Sergio Villar Senin]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Committed r174880: <http://trac.webkit.org/changeset/174880>