Bug 210453 - [GTK4] Adapt to cursor API changes
Summary: [GTK4] Adapt to cursor API changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
Depends on:
Blocks: GTK4
  Show dependency treegraph
 
Reported: 2020-04-13 13:32 PDT by Adrian Perez
Modified: 2020-04-15 06:08 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.71 KB, patch)
2020-04-13 13:39 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch v2 (2.78 KB, patch)
2020-04-14 14:28 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch for landing (2.94 KB, patch)
2020-04-15 05:33 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2020-04-13 13:32:22 PDT
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.
Comment 1 Adrian Perez 2020-04-13 13:39:52 PDT
Created attachment 396327 [details]
Patch
Comment 2 Darin Adler 2020-04-13 16:45:20 PDT
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
Comment 3 Carlos Garcia Campos 2020-04-13 22:23:06 PDT
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.
Comment 4 Adrian Perez 2020-04-14 14:19:48 PDT
(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 =)
Comment 5 Adrian Perez 2020-04-14 14:20:33 PDT
Also thanks to Darin for chiming in — I'll use “auto” a bit more =)
Comment 6 Adrian Perez 2020-04-14 14:28:50 PDT
Created attachment 396463 [details]
Patch v2
Comment 7 Carlos Garcia Campos 2020-04-15 03:22:49 PDT
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.
Comment 8 Adrian Perez 2020-04-15 04:05:28 PDT
(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.
Comment 9 Adrian Perez 2020-04-15 05:33:08 PDT
Created attachment 396520 [details]
Patch for landing
Comment 10 EWS 2020-04-15 06:08:29 PDT
Committed r260123: <https://trac.webkit.org/changeset/260123>

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