[Webkit-unassigned] [Bug 202447] [GTK] Don't hardcode swipe navigation gesture style

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 2 02:09:31 PDT 2019


https://bugs.webkit.org/show_bug.cgi?id=202447

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #380000|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20191002/5e90e613/attachment.html>


More information about the webkit-unassigned mailing list