Bug 210453 – [GTK4] Adapt to cursor API changes

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

[Adrian Perez]

We need to account for two changes in Source/WebCore/platform/gtk/CursorGtk.cpp:

 - The parameters taken by gdk_cursor_new_from_name() have changed.
 - In GTK4 custom cursors are now created from GdkTexture objects.

[Adrian Perez]

Created attachment 396327 [details]
Patch

[Darin Adler]

Comment on attachment 396327 [details]
Patch

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

Not sure I should review a GTK-specific patch. Look fine to me, but I hav no idea how to find out if it’s right or not.

> Source/WebCore/platform/gtk/CursorGtk.cpp:41
> +    GRefPtr<GdkCursor> fallback = adoptGRef(gdk_cursor_new_from_name("default", nullptr));

I’d have used auto here, and maybe made a function so this can be shared with the code below.

> Source/WebCore/platform/gtk/CursorGtk.cpp:70
> +        auto w = cairo_image_surface_get_width(surface.get());
> +        auto h = cairo_image_surface_get_height(surface.get());

WebKit coding style would suggest using a word for this

> Source/WebCore/platform/gtk/CursorGtk.cpp:72
> +        GRefPtr<cairo_surface_t> converted = adoptGRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, w, h));
> +        GRefPtr<cairo_t> cr = adoptGRef(cairo_create(converted.get()));

I’d have used auto here, and maye not both local variables

WebKit style would suggest nouns or noun phrases for these variable names, not adjectives "converted" or abbreviations "cr".

> Source/WebCore/platform/gtk/CursorGtk.cpp:80
> +    GRefPtr<GdkCursor> fallback = adoptGRef(gdk_cursor_new_from_name("default", nullptr));
> +    GRefPtr<GdkTexture> texture = adoptGRef(gdk_memory_texture_new(cairo_image_surface_get_width(surface.get()), cairo_image_surface_get_height(surface.get()), format, cairo_image_surface_get_stride(surface.get())));

I’d have used auto here

[Carlos Garcia Campos]

Comment on attachment 396327 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        provide the "pointer" cursor as fallback.

Is it "pointer" or "default"?

> Source/WebCore/platform/gtk/CursorGtk.cpp:65
> +    default: {

Is this even possible? I would assert here, I think we can only receive CAIRO_FORMAT_ARGB32 (or CAIRO_FORMAT_RGB24) here, since ImageBufferCairo creates CAIRO_FORMAT_ARGB32 surfaces.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #3)
> Comment on attachment 396327 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396327&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        provide the "pointer" cursor as fallback.
> 
> Is it "pointer" or "default"?

It's "default", I'll fix it in the ChangeLog.

> > Source/WebCore/platform/gtk/CursorGtk.cpp:65
> > +    default: {
> 
> Is this even possible? I would assert here, I think we can only receive
> CAIRO_FORMAT_ARGB32 (or CAIRO_FORMAT_RGB24) here, since ImageBufferCairo
> creates CAIRO_FORMAT_ARGB32 surfaces.

You are right, I should have checked. Let's remove the conversion code
and assert here that the Cairo surface format is CAIRO_FORMAT_ARGB32 =)

[Adrian Perez]

Also thanks to Darin for chiming in — I'll use “auto” a bit more =)

[Adrian Perez]

Created attachment 396463 [details]
Patch v2

[Carlos Garcia Campos]

Comment on attachment 396463 [details]
Patch v2

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

> Source/WebCore/platform/gtk/CursorGtk.cpp:43
> +#if USE(GTK4)
> +static GRefPtr<GdkCursor> fallbackCursor()
> +{
> +    return adoptGRef(gdk_cursor_new_from_name("default", nullptr));
> +}
> +#endif // USE(GTK4)

Would it make sene to cache this? using NeverDestroyed? I guess it will be used a lot. I'm not sure how expensive it is, or whether it's already cached by GTK though.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #7)
> Comment on attachment 396463 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396463&action=review
> 
> > Source/WebCore/platform/gtk/CursorGtk.cpp:43
> > +#if USE(GTK4)
> > +static GRefPtr<GdkCursor> fallbackCursor()
> > +{
> > +    return adoptGRef(gdk_cursor_new_from_name("default", nullptr));
> > +}
> > +#endif // USE(GTK4)
> 
> Would it make sene to cache this? using NeverDestroyed? I guess it will be
> used a lot. I'm not sure how expensive it is, or whether it's already cached
> by GTK though.

After checking the code for gdk_cursor_new_from_name(), I think it may
be a good idea to cache the fallback pointer because the function creates
a new object each time. I'll add that bit before landing.

[Adrian Perez]

Created attachment 396520 [details]
Patch for landing

[EWS]

Committed r260123: <https://trac.webkit.org/changeset/260123>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396520 [details].