Bug 210453

Created attachment 396327 [details] Patch

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

(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 =)

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

Created attachment 396463 [details] Patch v2

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.

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

Created attachment 396520 [details] Patch for landing

Committed r260123: <https://trac.webkit.org/changeset/260123> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396520 [details].