Created attachment 102315 [details] [review]
all-inclusive patch

Attached patch removes almost all dependencies on gnome-vfs, introducing dep on glib >= 2.15.0.
There are two places which are not ported yet: ephy-main.c, due to gnome_vfs_shutdown () (which I guess will be the last thing to remove) and ephy-bookmarks.c, due to the Avahi/Zeroconf stuff, which I had no time for now to look at.
Also some other comments: I had to write a quite simple ephy_string_get_host_name (url) to replace gnome_vfs_uri_get_host_name (). If it's not optimal, feel free to improve it; in my testing everything works fine as without patching, but there may be some corner cases I did not take into accout.
Also, if you want a splitted per-file patch (or per-git-commit) just ask :)

I forgot to say I also marked for translatable a string in ephy-download.c, which wasn't before.

Thanks, this looks great!

Some comments:

+  if (host != NULL) {
+    g_free (host);

g_free is NULL-safe.

+    if (G_IS_FILE_MONITOR (priv->monitor))
+    {
+      g_file_monitor_cancel (G_FILE_MONITOR (priv->monitor));
+    }
+    if (G_IS_DIRECTORY_MONITOR (priv->monitor))

|else if| ?

ephy_base_embed_directory_monitor_cb (GDirectoryMonitor *monitor,
+                                      GFile *child,
+                                      GFile *other_child,
+                                      gint event_type,
+                                      EphyBaseEmbed *embed)

glong event_type, since it's an enum (or GFileMonitorEvent). Same below.

+  }
+  if (file_type == G_FILE_TYPE_REGULAR)

|else if| ?

+		path = g_strconcat (g_file_get_path (dir), G_DIR_SEPARATOR_S,
+				    g_file_info_get_edit_name (file_info),

Why the edit-name, not the name of the file ?

+					       "standard::type,standard::edit-name,"

same question.

+	file_info = g_file_enumerator_next_file (file_enum, NULL, NULL);

Does this return a reference? The docs aren't clear on that, perhaps file a gio bug for that.

+	if (scheme != NULL)
 	{
-		gnome_vfs_uri_unref (vfs_uri);
+		g_free (scheme);
+	}
+	if (host_name != NULL)
+	{
+		g_free (host_name);

g_free is NULL-safe.

+user_css_file_monitor_changed_cb (GFileMonitor *file_monitor,
+				  GFile *file,
+				  GFile *other_file,
+				  gint event_type,
+				  MozillaEmbedSingle *single)

see above.

+#include <gio/gdesktopappinfo.h>
+#include <gio/gio.h>

gdesktopappinfo.h comes from gio-unix, right? Is there no global header like gio/gio.h for that? Maybe worth filing a gio bug...

-	expanded = gnome_vfs_expand_initial_tilde (download_dir);
-	g_free (download_dir);
-
-	return expanded;
+	return download_dir;

That's a behaviour change; return ephy_file_expand_initial_tilde here ?

+	app = g_desktop_app_info_new (filename);
+	file = g_file_new_for_path (parameter);
+	list = g_list_append (list, file);
+	
+	return ephy_file_launch_application (G_APP_INFO (app), list, user_time, widget);
 }

Doesn't this leak |app| ?

+		parent = g_file_get_parent (file);
 		/* TODO find a way to make nautilus scroll to the actual file */
 		ret = ephy_file_launch_handler ("x-directory/normal", 
-						gnome_vfs_uri_get_path (parent_uri), 
+						parent, 
 						user_time);

Opening the file manager at a given file is a common operation; gio should really offet API for it. File a gio bug ?

 failed:
-	g_free (tmp_file);
+	g_free (tmp_file_path);
 
 	return ret;

Don't you leak the 2 GFile objects here ?

+		address = g_strconcat (
+				       scheme,
+				       "://",
+				       host_name,
+				       NULL);

No need for (\n here, just put the 1st arg on the same line.

+	handler = eel_gconf_get_string ("/desktop/gnome/url-handlers/mailto/command");

Need to check the enabled key in the same dir too before using the command.
(And the same below in window-commands.c.)

--

The code for reading/writing with libxml is ok, but there really should be common code somewhere in gnome (libgnome?) or for c&p, to make libxml read/write from a GFile or GInput/OutputStream...

Created attachment 102328 [details] [review]
fix roundup

Christian: attached patch applies on top of the previous one and is a roundup of fixes for your comments.
Some notes:
- you were right about GFileEnumerator returning refs, I will file a bug about that. Also I was missing a call to g_file_enumerator_close () when finish.
- I used GFileMonitorEvent instead of gint, though Devhelp documentation of glib 2.15.0 has the prototype of the "changed" callback with gint. I will file a bug about that too, if it's not modified in glib trunk.
- There's no unique header to include for the unix part of GIO.

gio change from today:
+  - GDirectoryMonitor has been removed; GFileMonitor can monitor
+    files and directories now

Created attachment 102361 [details] [review]
fix roundup v2

Attached patch applies on top of the other two and:
- updates to new GFileMonitor API
- bump dep on glib >= 2.15.1.
- fix some other minor flaws I found around.

(In reply to comment #3)
 
> Why the edit-name, not the name of the file ?
> 
> +                                             
> "standard::type,standard::edit-name,"
> 
> same question.

(I missed this question yesterday)
I used it because it seemed to me the safest option (as the "name" attribute could be in an unknown encoding according to gio docs), but I think we might use "copy-name" instead, as it is said to be always UTF-8. What do you think?

You're using the filename not for display but for working with the file (deleting it), so you should use the real name.

Created attachment 102388 [details] [review]
fix roundup v3

Use name instead of edit-name (applies on top of the other patches).

(In reply to comment #3)

> The code for reading/writing with libxml is ok, but there really should be
> common code somewhere in gnome (libgnome?) or for c&p, to make libxml
> read/write from a GFile or GInput/OutputStream...

I thought the same thing when reading that part of code. Is there a bug filed somewhere/anyone you know working on such a thing?

I think libgsf can do that (libgsf module in gnome svn), and it just got gio support.... However I'm not sure depending on that huge lib is the best thing to do...

Looks good so far.

libgsf is listed on the external dependencies page, but in version 1.14.5 while the gio support is only in .6 and probably needs trunk for gio trunk...

One other thing: since bookmarks im/export uses GFile now, we should be able to allow remote files (set local-only = FALSE) for their filechoosers, and use get_uri instead of get_filename.

 		path = g_strconcat (g_file_get_path (dir), G_DIR_SEPARATOR_S,
-				    g_file_info_get_edit_name (file_info),

Use g_build_filename.

The further changes (libgsf?, and allowing non-local files which probably depends on that) can be done when they're ready, shouldn't block this.

Xan, could you take a look over the patches too?

Created attachment 102392 [details] [review]
fix roundup v4

Use g_build_filename ().

So, a summary of what's missing:
- to remove completely the dep on gnome-vfs we need to port all the Avahi stuff in ephy-bookmarks.c (IMHO this has priority so we can stop linking to gnome-vfs).
- replace the libxml parts with libgsf using their gsf-gvfs backend.

(In reply to comment #14)
> - to remove completely the dep on gnome-vfs we need to port all the Avahi stuff
> in ephy-bookmarks.c (IMHO this has priority so we can stop linking to
> gnome-vfs).

Definitely has priority, yes.

Some minor nits:

+  if (g_str_has_prefix (address, "file://"))
+  {
+    return g_strdup (address + 7);
+  }
+  return ephy_string_get_host_name (address);

K&R braces (happens in more places in the same file), maybe an else?



+	if (mime_type == NULL || type != G_FILE_TYPE_REGULAR)
+	{
+		return;
+	}
+	else
+	{

Mmm, that is really strange, can you invert the logic and get rid of the else?

-	mime_description = gnome_vfs_mime_get_description (mMimeType.get());
+	mime_description = g_content_type_get_description (mMimeType.get());

you need to free mime_description now I think.

         char *filename;
-        filename = gnome_vfs_unescape_string (default_name, NULL);
+        filename = g_uri_unescape_string (default_name, NULL);
 
Can we assign in the declaration? :)

-		uri = gnome_vfs_get_uri_from_local_path (priv->user_css_file);
+		file = g_file_new_for_path (priv->user_css_file);

Missing g_object_unref for file. (I think you are missing this in most places. Either that or I'm missing something :))

Created attachment 102430 [details] [review]
fix roundup v5

Attached patch plugs mem leaks and style issues as for Xan comment.

Created attachment 102431 [details] [review]
avahi migration

Attached patch migrates ephy-bookmarks.c to avahi-gobject...works just fine in my (quite limited) testing.

Note that if you get a warning about a wrong GaProtocol when running Ephy with the avahi patch, it should be a bug in avahi-gobject using a wrong enum type for a ParamSpec (I'll report the bug to Sjoerd).

I talked with Sjoerd and the warning is fixed in Avahi SVN now (though the warning is non fatal, so it works fine also with 0.6.22).

Created attachment 102620 [details] [review]
all-inclusive patch v2

All-in-one patch.
With this I removed last bits of gnome-vfs from configure.ac and ephy-main.c, making Ephy completely gnome-vfs free. I also removed HAVE_GFVS checks for "gio-standalone" in configure.ac, as that module does not exist anymore.

+static char*
 get_title_from_address (const char *address)
 {
-  GnomeVFSURI *uri;
-  char *title;
-
-  if (address == NULL) return NULL;

Not sure if that NULL-check was ever needed... it's probably ok to remove it.

Does ephy_base_embed_update_file_monitor leak the |file| ?

+	type = g_file_info_get_file_type (file_info);
+	mime_type = g_file_info_get_content_type (file_info);
+	
+	if (mime_type != NULL && type == G_FILE_TYPE_REGULAR)

Why do we need to get the mime type here?

+	file_enum = g_file_enumerate_children (dir,
+					       "standard::type,standard::name,"
+					       "standard::content-type",
+					       0, NULL, NULL);

Let's use the defines here. I.e.:
G_FILE_ATTRIBUTE_STANDARD_TYPE "," 
G_FILE_ATTRIBUTE_STANDARD_NAME ","
G_FILE_ATTRIBUTE_STANDARD_CONTENT_TYPE

+	/* FIXME: better use g_app_info_get_default_for_uri_scheme () when it is
+	 * implemented.
+	 */

File a ephy bug so we don't forget about this :)

With those nits fixed, please commit. Thanks very much for the patches! 

Created attachment 102764 [details] [review]
committed patch

Attaching here the patch that was committed.

Closing as FIXED.

------------------------------------------------------------------------
r7858 | cosimoc | 2008-01-13 21:42:01 +0100 (dom, 13 gen 2008) | 4 lines

Drop gnome-vfs dependency. Now Epiphany depends on glib >= 2.15.1.
Also, optional Zeroconf support depends on Avahi >= 0.6.22.
Bug #507152.

*diego fanboys cosimo