[webkit-reviews] review denied: [Bug 202447] [GTK] Don't hardcode swipe navigation gesture style : [Attachment 380000] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Alexander Mikhaylenko
<alexm at gnome.org>'s request for review:
Bug 202447: [GTK] Don't hardcode swipe navigation gesture style
https://bugs.webkit.org/show_bug.cgi?id=202447

Attachment 380000: Patch

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




--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 380000
  --> https://bugs.webkit.org/attachment.cgi?id=380000
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