Bug 202447 – [GTK] Don't hardcode swipe navigation gesture style

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

[Alice Mikhaylenko]

See the patch.

[Alice Mikhaylenko]

Created attachment 379996 [details]
Patch

The upload and style check scripts don't seem to work right now, so created a patch with `git format-patch` and without the check.

It's mostly ready, though I want to replace the current shadow gradient in css with something less complicated first.

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alice Mikhaylenko]

Created attachment 379997 [details]
Patch

[Alice Mikhaylenko]

Created attachment 380000 [details]
Patch

Ok, now with the changed gradient.

[Alice Mikhaylenko]

Created attachment 380001 [details]
Screenshot

[Alice Mikhaylenko]

Created attachment 380003 [details]
Screenshot

Err, actual screenshot

[Carlos Garcia Campos]

Comment on attachment 380000 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380000&action=review

Thanks for the patch, I have a few comments but looks good.

> Source/WebKit/PlatformGTK.cmake:520
> +    "        <file alias=\"css/gtk.css\">gtk.css</file>\n"

I would make it explicit that this css has nothing to do with UA css, it's the theme css. Maybe:

<file alias=\"theme/gtk-theme.css\">gtk-theme.css</file>

What do you think?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1448
> +    gtk_widget_class_set_css_name(widgetClass, "webkitwebview");

This is only available since GTK 3.20 we need to add versions checks here.

> Source/WebKit/UIProcess/ViewGestureController.h:169
> +    GtkStyleContext* createStyleContext(const char*);

Why is this public?

> Source/WebKit/UIProcess/ViewGestureController.h:421
> +    GRefPtr<GtkCssProvider> m_cssProvider;

You need to include GRefPtr.h. It probably doesn't fail due to unified builds. We should also remove the gtk include from the header and forward delcare the gtk types

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:280
> +GtkStyleContext* ViewGestureController::createStyleContext(const char* name)

This could return a GRefPtr<GtkStyleContext> instead.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:286
> +    // FIXME: is there a GRefPtr-like helper here? GtkWidgetPath is not a GObject
> +    GtkWidgetPath* path = gtk_widget_path_copy(gtk_widget_get_path(widget));

You can use GRefPtr, but include WebCore/GRefPtrGtk.h

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:288
> +    int pos = gtk_widget_path_append_type(path, GTK_TYPE_WIDGET);

pos -> position

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:302
> +static RefPtr<cairo_pattern_t> createElementPattern(GRefPtr<GtkStyleContext> context, double w, double h)

I think it's better to pass the raw pointer here. Use width and height instead of w and h

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:304
> +    RefPtr<cairo_surface_t> surface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, w, h));

cairo expects integers, I wonder why we receive doubles that has been converted from integers

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:308
> +    gtk_render_background(context.get(), cr.get(), 0, 0, w, h);
> +    gtk_render_frame(context.get(), cr.get(), 0, 0, w, h);

I think it's fine to pass integers here even when it expects doubles.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:313
> +static double getElementWidth(GRefPtr<GtkStyleContext> context)

Same here, either pass the raw pointer or a ref instead of copying the GRefPtr. getElementWidth() -> elementWidth()

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:316
> +    gtk_style_context_get(context.get(), gtk_style_context_get_state(context.get()), "min-width", &width, NULL);

NULL -> nullptr

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:318
> +    return (double) width;

Use C++ style casts

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:358
> +    double width = m_webPageProxy.drawingArea()->size().width();
> +    double height = m_webPageProxy.drawingArea()->size().height();

DrawingAreaProxy::size() returns an IntSize, why do we need to implicit convert it to double? Instead of getting the size twice, we could get it once, and use .width() and .height() when needed.

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:419
> +        shadowOpacity = (remainingSwipeDistance / m_swipeShadowSize);

Parentheses are not needed here

[Alice Mikhaylenko]

Created attachment 380009 [details]
Patch

[Alice Mikhaylenko]

(In reply to Carlos Garcia Campos from comment #7)
> What do you think?

Sure.

> Why is this public?

No reason, that was a mistake :)

> DrawingAreaProxy::size() returns an IntSize, why do we need to implicit
> convert it to double? Instead of getting the size twice, we could get it
> once, and use .width() and .height() when needed.

Because caching was a late change and it called gtk_render_background() etc directly in draw(). Doesn't make sense anymore indeed

[Carlos Garcia Campos]

Comment on attachment 380009 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380009&action=review

LGTM, why didn't you ask for review this time?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1450
> +#if GTK_CHECK_VERSION(3, 20, 0)
> +    gtk_widget_class_set_css_name(widgetClass, "webkitwebview");
> +#endif

What happens when building with previous gtk?

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:31
> +#include "WebCore/GRefPtrGtk.h"

Use <WebCore/GRefPtrGtk.h>

> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:301
> +static cairo_pattern_t* createElementPattern(GRefPtr<GtkStyleContext> context, int width, int height)

Sorry, I meant using the raw pointer for the context parameter instead of copying the GRefPtr, the smart pointer was ok for the return value.

[Alice Mikhaylenko]

> What happens when building with previous gtk?

A lot of things would break. However... Isn't 3.22 the minimum, according to bug 199094? In which case the 3.20 guards arent' even needed.

> LGTM, why didn't you ask for review this time?

Ugh, I continue to forget to do it. Usually the upload script does it for me.

[Alice Mikhaylenko]

Created attachment 380031 [details]
Patch

[Alice Mikhaylenko]

Created attachment 380036 [details]
Patch

Oops, fixed style

[Carlos Garcia Campos]

(In reply to Alexander Mikhaylenko from comment #11)
> > What happens when building with previous gtk?
> 
> A lot of things would break. However... Isn't 3.22 the minimum, according to
> bug 199094? In which case the 3.20 guards arent' even needed.

Right! I forgot we had bumped it.

> > LGTM, why didn't you ask for review this time?
> 
> Ugh, I continue to forget to do it. Usually the upload script does it for me.

[WebKit Commit Bot]

Comment on attachment 380036 [details]
Patch

Clearing flags on attachment: 380036

Committed r250648: <https://trac.webkit.org/changeset/250648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.