Created attachment 363468 [details] [review]
file: add g_file_load_bytes()

This adds a new g_file_load_bytes() helper API to make loading
data from a file as GBytes more convenient. Additionally, it
allows reuse of the embedded data for GResource based GFile
instead of making a copy to the heap.

Review of attachment 363468 [details] [review]:

Seems like a good idea.

::: gio/gfile.c
@@ +8059,3 @@
+
+/**
+ * g_file_load_bytes:

Needs to be listed in docs/reference/gio/gio-sections.txt and also needs some unit tests please.

@@ +8062,3 @@
+ * @file: a #GFile
+ * @cancellable: (nullable): a #GCancellable
+ * @etag_out: (out) (optional): a location to place the current entity tag for

Should also be (nullable), since the ETag can be NULL for `resource://` URIs.

@@ +8069,3 @@
+ *
+ * If @file is a resource:// based URI, the resulting bytes will reference the
+ * embedded resources instead of a copy.

Please add something like “otherwise, this is equivalent to calling g_file_load_contents() and g_bytes_new_take()”.

Perhaps also worth pointing out that no ETag is returned for `resource://` URIs.

@@ +8089,3 @@
+
+  g_return_val_if_fail (G_IS_FILE (file), NULL);
+  g_return_val_if_fail (!cancellable || G_IS_CANCELLABLE (cancellable), NULL);

Nitpick: Explicit NULL comparisons please:

g_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), NULL);

Plus also add:

g_return_val_if_fail (error == NULL || *error == NULL, NULL);

@@ +8103,3 @@
+      g_free (uri);
+
+      bytes = g_resources_lookup_data (unescaped, 0, error);

s/0/G_RESOURCE_LOOKUP_FLAGS_NONE/

@@ +8111,3 @@
+  /* contents is guaranteed to be \0 terminated */
+  if (g_file_load_contents (file, cancellable, &contents, &len, etag_out, error))
+    return g_bytes_new_take (contents, len);

g_steal_pointer (&contents) to make the ownership transfer more explicit.

Do we need an async version of this function?

Created attachment 363564 [details] [review]
file: add g_file_load_bytes()

This adds g_file_load_bytes() to make it more convenient to
load the contents of a GFile as GBytes.

It includes a special casing for gresources to increase the
chances that the GBytes directly references the embedded data
instead of copying to the heap.

(In reply to Benjamin Otte (Company) from comment #3)
> Do we need an async version of this function?

Yeah that makes sense. New patch includes an asynchronous form too.

Created attachment 363566 [details] [review]
file: add tests for g_file_load_bytes()

This adds a test for both g_file_load_bytes() and the
asynchronous form g_file_load_bytes_async().

Review of attachment 363564 [details] [review]:

Looks good to me, thanks.

Eventually, we might want to virtualise this in GFileIface, if other VFS backends want to provide a GBytes fast path. We can always do that later though: this is plenty fine enough for now.

Review of attachment 363566 [details] [review]:

Two code comments to add, but you can do that when pushing. The rest looks fine to me (yay, tests). Thanks!

::: gio/tests/file.c
@@ +1056,3 @@
+  fd = g_mkstemp (filename);
+  g_assert_cmpint (fd, !=, -1);
+  ret = write (fd, "test_load_bytes", 29);

I assume you’re deliberately writing about 2× as many bytes as there are in this string so that you can test the nul-termination of the GBytes which is returned? If so, please add a comment about that.

@@ +1107,3 @@
+  g_assert_cmpint (fd, !=, -1);
+  ret = write (fd, "test_load_bytes_async", 35);
+  g_assert_cmpint (ret, ==, 35);

Similarly here.

@@ +1114,3 @@
+
+  g_file_load_bytes_async (data.file, NULL, test_load_bytes_cb, &data);
+  g_main_loop_run (data.main_loop);

Note for future use (I’m not expecting you to change the patch to use this, since what you have works fine): the following pattern generally saves on declaring closures, gives you a reusable callback function, and makes the main iteration termination conditions more obvious:

static void
async_result_cb (GObject *obj, GAsyncResult *result, gpointer user_data)
{
  GAsyncResult **result_out = user_data;
  *result_out = g_object_ref (result);
}

static void
my_test (void)
{
  g_autoptr(GAsyncResult) result = NULL;
  …

  g_file_load_bytes_async (file, NULL, async_result_cb, &result);

  while (result == NULL)
    g_main_context_iteration (NULL, TRUE);

  bytes = g_file_load_bytes_finish (file, result, &error);
  g_assert_no_error (error);
  g_assert_nonnull (bytes);
  …
}

Oops, I had some different text there previously. I made the code more
obvious. We do want to check the tailing \0, and that's it.

Attachment 363564 [details] pushed as 2227918 - file: add g_file_load_bytes()
Attachment 363566 [details] pushed as ed78f30 - file: add tests for g_file_load_bytes()