Created attachment 330102 [details] [review]
vfs: add g_vfs_register_uri_scheme()

Add a new API to allow clients to register a custom GFile implementation
handling a particular URI scheme.
This can be useful for tests, but also for cases where a different URI
scheme is desired to be used with another custom GFile backend.

As an additional cleanup, we can use this to register the "resource" URI
scheme too.

Based on a patch by Jasper St. Pierre <jstpierre@mecheye.net>.

How and where do you envision this being used ? Calling register_uri_scheme in an application doesn't seem super-interesting since it only adds support for this uri scheme to that application. Shouldn't this go into some sort of extension point, so that it can be desktop-wide ?

(In reply to Matthias Clasen from comment #2)
> How and where do you envision this being used ? Calling register_uri_scheme
> in an application doesn't seem super-interesting since it only adds support
> for this uri scheme to that application. Shouldn't this go into some sort of
> extension point, so that it can be desktop-wide ?

The idea is to give an application (or a library) the ability to register a custom URI handler for things that are shipped together with the application.
For instance, a library we developed here at Endless (don't have code available to point to right now unfortunately) would register a handler to load data from a database that's shipped with the application, and a (for example) CSS file loaded by GTK would be able to refer to resources using URIs with such scheme.

You can think of it as a generalization of what "resource" URIs mean today, except that the implementation would be left to the application/library.

For things that are desktop-wide, a GVfs backend is a better solution.

(In reply to Cosimo Cecchi from comment #3)

> For things that are desktop-wide, a GVfs backend is a better solution.

But there can be only one gvfs backend...

Review of attachment 330102 [details] [review]:

If we do something like this, GResourceFile should probably be ported over to use this mechanism.

::: gio/gvfs.c
@@ +213,3 @@
+      default_schemes = (* class->get_supported_uri_schemes) (vfs);
+      supported_schemes = g_ptr_array_new ();
+

It may be far-fetched, but do we consider the possibility that get_supported_uri_schemes could return different lists over time ?

@@ +226,3 @@
+    }
+
+  return priv->supported_schemes;

We currently support resource: uris, but g_vfs_get_supported_uris_schemes does not return "resource" as one of the supported schemas. Do you think that should be fixed at the same time ? Or do you think that that indicates that this function should not be changed ? The additional schemes are not, in fact, "supported by the gvfs" - they are supported by the caller who added a callback

@@ +306,3 @@
+ * replaced. It's possible to call this function with a %NULL @func to
+ * unregister handling of @scheme.
+ *

I think the silent replacement is questionable. That is going to make it difficult to use this from multiple independent places. It would be better to error out.

Created attachment 330106 [details] [review]
vfs: add g_vfs_register_uri_scheme()

Add a new API to allow clients to register a custom GFile implementation
handling a particular URI scheme.
This can be useful for tests, but also for cases where a different URI
scheme is desired to be used with another custom GFile backend.

As an additional cleanup, we can use this to register the "resource" URI
scheme too.

Based on a patch by Jasper St. Pierre <jstpierre@mecheye.net>.

(In reply to Matthias Clasen from comment #5)
> If we do something like this, GResourceFile should probably be ported over
> to use this mechanism.

Yeah -- I left the GResourceFile bits in gvfs.c, but they could easily be moved in a later cleanup.
 
> ::: gio/gvfs.c
> @@ +213,3 @@
> +      default_schemes = (* class->get_supported_uri_schemes) (vfs);
> +      supported_schemes = g_ptr_array_new ();
> +
> 
> It may be far-fetched, but do we consider the possibility that
> get_supported_uri_schemes could return different lists over time ?

Neither gvfs nor the implementations inside GLib support it, so I don't think it can really change at runtime behind GIO's back.

> @@ +226,3 @@
> +    }
> +
> +  return priv->supported_schemes;
> 
> We currently support resource: uris, but g_vfs_get_supported_uris_schemes
> does not return "resource" as one of the supported schemas. Do you think
> that should be fixed at the same time ? Or do you think that that indicates
> that this function should not be changed ? The additional schemes are not,
> in fact, "supported by the gvfs" - they are supported by the caller who
> added a callback

The documentation says "supported by @vfs", i.e. that instance, which would include those added programmatically. I think it's a mistake/oversight that the current implementation does not return "resource", so I changed it to return all the schemes added programmatically (which includes "resource").

> @@ +306,3 @@
> + * replaced. It's possible to call this function with a %NULL @func to
> + * unregister handling of @scheme.
> + *
> 
> I think the silent replacement is questionable. That is going to make it
> difficult to use this from multiple independent places. It would be better
> to error out.

Yeah, I agree, especially now that in this new patch I added handling for the parse name too.
I changed it to an explicit unregister API.

Review of attachment 330106 [details] [review]:

Otherwise this looks good to me.

::: gio/gvfs.c
@@ +255,3 @@
+  priv = g_vfs_get_instance_private (vfs);
+
+  if (!priv->supported_schemes)

This should probably use g_once()

@@ +377,3 @@
+ * matches what the custom #GFile implementation returned when g_file_get_parse_name()
+ * was previously called.
+ *

We should also mention that the implementation should not be blocking (i.e. do i/o).

@@ +404,3 @@
+  priv = g_vfs_get_instance_private (vfs);
+
+  closure = g_hash_table_lookup (priv->additional_schemes, scheme);

Do we care about this not being threadsafe?

Can the documentation for this method explicitly note that it does not register the scheme desktop-wide - that this is useful for handling URIs generated within the application. I read this bug mail and was immediately elated, and then saddened that it didn't do what I expected.

Created attachment 330232 [details] [review]
vfs: add g_vfs_register_uri_scheme()

(In reply to Alexander Larsson from comment #8)
> Review of attachment 330106 [details] [review] [review]:
> 
> Otherwise this looks good to me.
> 
> ::: gio/gvfs.c
> @@ +255,3 @@
> +  priv = g_vfs_get_instance_private (vfs);
> +
> +  if (!priv->supported_schemes)
> 
> This should probably use g_once()

No, because the supported schemes array is invalidated when g_vfs_register/unregister_uri_scheme() is called.

> @@ +377,3 @@
> + * matches what the custom #GFile implementation returned when
> g_file_get_parse_name()
> + * was previously called.
> + *
> 
> We should also mention that the implementation should not be blocking (i.e.
> do i/o).

Fixed

> @@ +404,3 @@
> +  priv = g_vfs_get_instance_private (vfs);
> +
> +  closure = g_hash_table_lookup (priv->additional_schemes, scheme);
> 
> Do we care about this not being threadsafe?

Good point.  I added locking around the hash table now, so it should be safe.

Review of attachment 330232 [details] [review]:

::: gio/gvfs.c
@@ +185,3 @@
+
+      G_LOCK (additional_schemes);
+      res = g_hash_table_iter_next (&iter, NULL, (gpointer *) &closure);

You can't release the lock and expect the hashtable iters to still ve valid!

@@ +214,3 @@
+
+  G_LOCK (additional_schemes);
+  closure = g_hash_table_lookup (priv->additional_schemes, scheme);

What if the closure is freed due to unregistering, while we call into it?

Created attachment 330261 [details] [review]
vfs: add g_vfs_register_uri_scheme()

For both of those comments, I wanted to avoid holding the lock while calling into the closures, because the closure could call back into g_file_new_for_uri() or g_file_parse_name().
I changed the code to use a GRecMutex instead now, and modified the tests to explicitly check for that too.

I have a deep distrust of recursive mutexes, they usually just hide a bad locking model. Can't you use a reader-writer lock instead? (And document that you can't unregister a scheme from inside a scheme callback.

Created attachment 330285 [details] [review]
vfs: add g_vfs_register_uri_scheme()

--

Now using a GRWLock.

Review of attachment 330285 [details] [review]:

Looks good to me now.

Thanks for the reviews!

Attachment 330285 [details] pushed as 375b4ca - vfs: add g_vfs_register_uri_scheme()