Created attachment 360929 [details] [review]
gsk: Fix serialiaztion of cairo node

Review of attachment 360929 [details] [review]:

::: gsk/gskrendernodeimpl.c
@@ +1761,3 @@
+      {
+        memcpy (mem_surface + i * width * 4, data + i * stride, width * 4);
+      }

We generally omit {} for single-statement bodies like this. But if you wanted to keep them, they should be indented by 2, like this:

  for (...)
    {
      memcpy...
    }

@@ +1771,3 @@
+                                                       mem_surface,
+                                                       width * height,
+                                                       sizeof (guint32)));

As we discussed on telegram, g_variant_new_fixed_array copies the data, so we will have to free mem_surface to avoid leaking it, since we allocated it ourselves.

Created attachment 361058 [details] [review]
gsk: Fix serialization of cairo node

Created attachment 361059 [details] [review]
gsk: Move gsk_cairo_node_new_for_surface into public API

Created attachment 361060 [details] [review]
Create tests for cairo node

Review of attachment 361058 [details] [review]:

other than that, looks good. Feel free to push with that change

::: gsk/gskrendernodeimpl.c
@@ +1756,3 @@
+      data = cairo_image_surface_get_data (self->surface);
+
+      mem_surface = (guchar *) g_slice_alloc (width * height * 4);

I would just use g_malloc/g_free here. g_slice is really for small repetitive allocations, not for big one-off ones.

Review of attachment 361059 [details] [review]:

sure

Review of attachment 361060 [details] [review]:

::: tests/rendernode-create-tests.c
@@ +445,3 @@
+
+      height = g_random_int_range (0, 100);
+      width = g_random_int_range (0, 100);

Not sure if width == 0 or height == 0 is a good case to test for. Does that actually work ?

(In reply to Matthias Clasen from comment #8)
> Review of attachment 361060 [details] [review] [review]:
> 
> ::: tests/rendernode-create-tests.c
> @@ +445,3 @@
> +
> +      height = g_random_int_range (0, 100);
> +      width = g_random_int_range (0, 100);
> 
> Not sure if width == 0 or height == 0 is a good case to test for. Does that
> actually work ?

No. That's wasn't a good idea. I changed it to (1,100) range and pushed.

74a677a79 - gsk: Fix serialization of cairo node
a933c7c4b - gsk: Move gsk_cairo_node_new_for_surface into public API
74f8fc80d - Create tests for cairo node 

Thanks Matthias for all your help and reviews.