Created attachment 29915 [details] [review]
Add ephy_extensions_manager_unload()

I ran this function under GDB to test it. I came across a couple of problems:

1. I never ran g_object_unref() on the extension, and yet it seems to be
unreffed. Is that expected?

2. Reloading extensions gives "Two different plugins tried to register...."
What's the best way to work around this?

Created attachment 29916 [details] [review]
Make the gestures extension unload

Seems a lot of extensions are broken with Ephy 1.3, but I don't have time
tonight to fix them. The gestures extension actually doesn't crash, so I made
it detach from all tabs when closing to test.

gdb commands:

EPHY_LOG_MODULES=ephy-extensions-manager.c jhbuild run epiphany-gdb # where
epiphany-gdb is like Epiphany bug the $dist_bin/epiphany-bin has a "gdb" before it.

r

[ Note the address of the libgesturesextension.so extension ]

Ctrl-C

p ephy_extensions_manager_unload ((EphyExtensionsManager
*)ephy_shell_get_extensions_manager(ephy_shell), (EphyExtension *)0x[address])

c

...enjoy!

Never mind my #1 problem up there, I unref it in ephy_extension_info_free().
Man, I'm tired... :)

Created attachment 29975 [details] [review]
Wrapper to g_type_module_register_type()

This works around my above-mentioned problem of duplicate type names: it takes
the system time to generate random gibberish after the type name. This *could*
be considered a memory leak since we're leaving garbage around that we'll never
use... but I don't think there's a better way to load/unload extensions.

Extensions would have to call ephy_extension_register_type() instead of
g_type_module_register_type().

I'm not sure if ephy-extension.[ch] is the best place to put these functions;
if not, does anyone have a better suggestion? Maybe in the epiphany-extensions
source tree?

Created attachment 29976 [details] [review]
Make reloading the gestures extension work, too!

This Works, and I'm Proud Of It:

adam@hera:extensions$ EPHY_LOG_MODULES=ephy-extensions-manager.c jhbuild run
epiphany-gdb
GNU gdb 6.1-debian
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i386-linux"...Using host libthread_db library
"/lib/tls/i686/cmov/libthread_db.so.1".

(gdb) r
Starting program: /opt/gnome28/bin/epiphany-bin
[Thread debugging using libthread_db enabled]
[New Thread 1087112416 (LWP 8297)]
[ ephy-extensions-manager.c ] EphyExtensionsManager initialising
[ ephy-extensions-manager.c ] adding extensions of type EphySession
[ ephy-extensions-manager.c ] Loaded extension
/opt/gnome28/lib/epiphany-1.3/extensions/libgesturesextension.so at 0x819b440
[ ephy-extensions-manager.c ] adding extensions of type EphySession
[ ephy-extensions-manager.c ] multiplexing attach_window
[New Thread 1116670896 (LWP 8300)]
[New Thread 1129057200 (LWP 8301)]
[New Thread 1137503152 (LWP 8302)]
[New Thread 1146768304 (LWP 8303)]
[New Thread 1155161008 (LWP 8304)]
[New Thread 1163553712 (LWP 8305)]
[New Thread 1171946416 (LWP 8306)]

[[[at this point, mouse gestures work. Open and close a few windows/tabs]]]

Program received signal SIGINT, Interrupt.
[Switching to Thread 1087112416 (LWP 8297)]
0xffffe410 in __kernel_vsyscall ()
(gdb) p ephy_extensions_manager_unload ((EphyExtensionsManager
*)ephy_shell_get_extensions_manager(ephy_shell), (EphyExtension *)0x819b440)
$1 = void
(gdb) c
Continuing.

[[[at this point, middle click doesn't do gestures. Open/close windows/tabs]]]

[ ephy-extensions-manager.c ] multiplexing attach_window
Program received signal SIGINT, Interrupt.
0xffffe410 in __kernel_vsyscall ()
(gdb) p ephy_extensions_manager_load ((EphyExtensionsManager
*)ephy_shell_get_extensions_manager(ephy_shell),
"/opt/gnome28/lib/epiphany-1.3/extensions/libgesturesextension.so")
[Thread 1137503152 (LWP 8302) exited]
[Thread 1146768304 (LWP 8303) exited]
[Thread 1155161008 (LWP 8304) exited]
[Thread 1163553712 (LWP 8305) exited]
[Thread 1171946416 (LWP 8306) exited]
[ ephy-extensions-manager.c ] Loaded extension
/opt/gnome28/lib/epiphany-1.3/extensions/libgesturesextension.so at 0x86fa130
$2 = (EphyExtension *) 0x86fa130
(gdb) c
Continuing.

[[[Gestures work again! Have fun opening/closing tabs/windows.]]]

[[[Close the last window...]]]

[ ephy-extensions-manager.c ] multiplexing detach_window
[ ephy-extensions-manager.c ] multiplexing detach_window
[ ephy-extensions-manager.c ] EphyExtensionsManager finalising
[Thread 1116670896 (LWP 8300) exited]
[Thread 1129057200 (LWP 8301) exited]

Program exited normally.
(gdb) q
adam@hera:extensions$

Created attachment 30050 [details] [review]
Re-use module loaders

Okay, I figured out how module loaders work (I think). This will reload
extensions with the existing module loaders, so their
g_type_module_register_type() will succeed. Removes the need for that stupid
g_type_module_register_type() wrapper.

Created attachment 30051 [details] [review]
Updated gestures extension

Removed the calls to the stupid wrapper. This works with Attachment #30050 [details].

Comment on attachment 30050 [details] [review]
Re-use module loaders

>Index: lib/ephy-module-loader.c
>+const char *
>+ephy_module_loader_get_path (EphyModuleLoader *loader)
>+{
g_return_val_if_fail (EPHY_IS_MODULES_LOADER (loader), NULL);

>+	return loader->path;
>+}

>Index: src/ephy-extensions-manager.c
>===================================================================
>RCS file: /cvs/gnome/epiphany/src/ephy-extensions-manager.c,v
>retrieving revision 1.3
>diff -u -3 -p -r1.3 ephy-extensions-manager.c
>--- src/ephy-extensions-manager.c	17 Feb 2004 12:03:25 -0000	1.3
>+++ src/ephy-extensions-manager.c	30 Jul 2004 01:27:57 -0000
>@@ -25,18 +25,27 @@
> 
> #include "ephy-extensions-manager.h"
> 
>+#include "ephy-shell.h"
>+#include "ephy-session.h"

Hmmm. So now the extension manager depends on the session, which is an
extension which depends on the extension manager... a bit weird, but I guess
okay since it's only used after startup.

>+typedef struct
>+{
>+	EphyModuleLoader *loader;
>+	EphyExtension *extension;
>+	gboolean is_internal;
>+} EphyExtensionInfo;
Aren't internal extensions already distinguished by loader == NULL,
which makes is_internal redundant ?

>+static void
>+ephy_extension_info_free (EphyExtensionInfo *info)
>+{
>+	/* Don't unref the loader -- it must exist forever */
But info_free is only used in finalize, surely it's okay to unref
them when ephy is shutting down?

+static void
+ephy_extensions_manager_attach_windows (EphyExtension *extension)

+static void
+ephy_extensions_manager_detach_windows (EphyExtension *extension)

Those are identical except for the GFunc called,
you should unify them to a
windows_forall (GFunc func, EphyExtension *extension)
and use the _reverse functions for |func|.

> ephy_extensions_manager_load (EphyExtensionsManager *manager,
> 			      const char *filename)

>+	for (l = manager->priv->extension_infos; l != NULL; l = g_slist_next(l))
> 	{
>-		EphyModuleLoader *loader;
>+		info = l->data;
>+		loader = info->loader;
>+		loader_path = ephy_module_loader_get_path (loader);
> 
Aren't the internal extensions also in this list, with loader == NULL ?

>+static int
>+ephy_extension_info_has_path (EphyExtensionInfo *info,
>+			      const char *path)
>+{
>+	EphyModuleLoader *loader;
>+	const char *loader_path;
>+
>+	loader = info->loader;
>+
>+	loader_path = ephy_module_loader_get_path (loader);
>+
>+	return strcmp (loader_path, path);
>+}
>+
>+void
>+ephy_extensions_manager_unload (EphyExtensionsManager *manager,
>+				const char *filename)
>+{
>+	GSList *info_link;
>+	EphyExtensionInfo *info;
>+
>+	info_link = g_slist_find_custom (manager->priv->extension_infos,
>+					 filename,
>+					 (GCompareFunc) ephy_extension_info_has_path);
The internal extensions are in this list, with loader == NULL; so your compare
func has to take that into account.
>+
>+	g_return_if_fail (info_link != NULL);
>+
What's the semantics of this function: is it a programmatic error to call it on
a non-loaded filename, or simply a run-time error (in which case that should be
a if (info_link==NULL) return;)? Should be stated in the yet-to-write function
doc.

>@@ -172,8 +304,12 @@ ephy_extensions_manager_add (EphyExtensi
> 		return NULL;
> 	}
> 
>-	manager->priv->extensions =
>-		g_slist_append (manager->priv->extensions, extension);
>+	info = g_new (EphyExtensionInfo, 1);
>+	info->extension = extension;
>+	info->is_internal = TRUE;
info->loader = NULL;

>>+	/* Don't unref the loader -- it must exist forever */
>But info_free is only used in finalize, surely it's okay to unref
>them when ephy is shutting down?

In the documentation for g_type_module_unuse():

"However, the GTypeModule will not be freed, and types associated with the
GTypeModule are not unregistered. Once a GTypeModule is initialized, it must
exist forever."

...and sure enough, trying to unref the loader results in a crash.

Created attachment 30101 [details] [review]
Next attempt

Addresses all Christian's concerns. I think.

I've included documentation, and I have an accompanying section in
doc/reference/ to commit.

Comment on attachment 30101 [details] [review]
Next attempt

>+static void
>+ephy_extensions_manager_attach_windows (EphyExtension *extension)
>+{
>+	windows_forall ((GFunc) attach_window_reverse, extension);
>+}
>+
>+static void
>+ephy_extensions_manager_detach_windows (EphyExtension *extension)
>+{
>+	windows_forall ((GFunc) detach_window_reverse, extension);
>+}
Get rid of those since they're only called once each, and use windows_forall
there directly.



>+/**
>+ * ephy_extensions_manager_list:
>+ * @manager: an #EphyExtensionsManager
>+ *
>+ * Return value: a newly-allocated GList of #EphyExtensionInfo<!-- -->s. Each
>+ * represents an #EphyExtension.
>+ **/
>+GList *
>+ephy_extensions_manager_list(EphyExtensionsManager *manager)
>+{
>+	g_return_val_if_fail (EPHY_IS_EXTENSIONS_MANAGER (manager));
>+
>+	return g_list_copy (manager->priv->extension_infos);
priv->extensions_info is a GSList, can't use g_list_copy on it.

> EphyExtension *
> ephy_extensions_manager_load (EphyExtensionsManager *manager,

+	for (l = manager->priv->extension_infos; l != NULL; l =
g_slist_next(l))

We use l = l->next everywhere, so you should use that here, too.


>+		loader_path = ephy_module_loader_get_path (loader);
>+		g_return_val_if_fail (loader_path != NULL, NULL);
> 
>+		if (strcmp (loader_path, filename) == 0)
>+		{
Check if info->extension == NULL and just return it if it's != NULL.

>+			g_type_module_use (G_TYPE_MODULE (loader));
> 			extension = >ephy_extensions_manager_instantiate_extension
> 				(manager, loader);
>-
Need to store extension in info.

Created attachment 30111 [details] [review]
Another attempt

Did all the things Christian suggested.

Also, since Christian said already-loaded extensions should just make
ephy_extensions_manager_load() use the already-loaded one, I made
ephy_extensions_manager_unload() return (and not crash) if unloading an
already-unloaded extension.

To build an extensions manager, we need a few more things:

- A way to list extensions
- A way to get extensions' descriptions
- A way to specify which extensions to load at startup
- A GUI (which may be an extension, but IMO should definitely be a part of
Epiphany's source code, extension or not)

There's no way we do all that for 1.4 (feature freeze and all) so this patch
will have to wait for a 1.5 branch.

Somewhat unrelated to the patch I think the module_loader path should be a property.

+static void
+windows_forall (GFunc func, EphyExtension *extension)

foreach I guess

+static void
+attach_window_reverse (EphyWindow *window,

reverse is a bit confusing, I'd just remove it if we cant find anything better.

+static void
+ephy_extension_info_detach_window (EphyExtensionInfo *info,
+				   EphyWindow *window)

These gives the impression EphyExtensionInfo is an object.
detach_window_from_info or something.
The same for attach.

+	/* Don't unref the loader -- it must exist forever */

This comment is confusing imo. If we didnt ref the loader when putting it in the
struct then we dont need to unref it no matter what the loader is and how it
should behave.

Maybe we should put loaders in an hash path/loader ? You just seem to need
lookup and foreach.

Created attachment 31336 [details] [review]
Listen to GConf

I've submitted a new and quite different patch. This one stores its data in a
hash table of "filename" => (loader, extension). "filename" is the middle part
of "libFOOextension.so".

ephy_module_loader no longer loads the module on _new(). The module is only
loaded in EphyExtensionManager's static real_load().

Internal extensions aren't in the hash; they're in a separate GSList.

Christian has already looked at this, so Marco, could you please inspect it?
I'd like to commit it as I think it's a good start; now I can work on a UI and
on making extensions unload properly.

Mass reassigning of Epiphany bugs to epiphany-maint@b.g.o