Created attachment 215013 [details] [review]
util: Don't generate a backtrace on every G_LOG

We may not show the backtrace, but it's prohibitly expensive to generate,
so don't. If someone wants a backtrace they can use the appropriate G_DEBUG
environment variable plus GDB.

Created attachment 215014 [details] [review]
main: Don't set a custom log handler

Created attachment 215015 [details] [review]
main: Don't call g_type_init from meta_init

For the plugin system, GType has to have been initialized by now.

Created attachment 215016 [details] [review]
meta-plugin: Remove some cruft

Created attachment 215017 [details] [review]
mutter: Only allow one plugin to be loaded

Created attachment 215018 [details] [review]
meta-plugin: Kill off "features"

We already check that the plugin has the appropriate vfunc in the klass
structure, so we shouldn't need to check for the same data again with
a "features" long.

Created attachment 215019 [details] [review]
meta-plugin: Remove "disabled" feature

It's just code cruft that nobody's using

Created attachment 215020 [details] [review]
meta-plugin-manager: Only allow one plugin to be loaded

The "multiple plugins loaded at once" strategy was always a big fiction:
while it may be viable if you're super careful, it's fragile and requires
a bit of infrastructure that we would be better off without.

Note that for simplicity, we're keeping the MetaPluginManager, but it only
manages one plugin. A possible future cleanup would be to remove it entirely.

Review of attachment 215014 [details] [review]:

See also https://bugzilla.gnome.org/show_bug.cgi?id=622441

Review of attachment 215015 [details] [review]:

Looks right.

Review of attachment 215013 [details] [review]:

True, it is a bad idea to do this by default.  

Also, there is now http://gnu.wildebeest.org/blog/mjw/2012/05/24/pull-user-space-probe-instrumentation/

Review of attachment 215016 [details] [review]:

Pretty minor cruft...but sure.

::: src/compositor/meta-plugin.c
@@ -98,3 @@
 
-  if (G_OBJECT_CLASS (meta_plugin_parent_class)->constructed)
-      G_OBJECT_CLASS (meta_plugin_parent_class)->constructed (object);

While this works because we know our parent class is always GObject, we should be careful against just removing it everywhere.

Review of attachment 215017 [details] [review]:

::: src/core/mutter.c
@@ +56,3 @@
   },
   {
+    "mutter-plugin", 0, 0, G_OPTION_ARG_STRING,

I wonder if there's anyone out there using --mutter-plugins in a script; we could consider keeping the old command line argument for a bit.  Dunno.  Probably not worth it.

Gotta run, back later.

Review of attachment 215016 [details] [review]:

::: src/compositor/meta-plugin.c
@@ -98,3 @@
 
-  if (G_OBJECT_CLASS (meta_plugin_parent_class)->constructed)
-      G_OBJECT_CLASS (meta_plugin_parent_class)->constructed (object);

Are there any classes with a 0'd out constructed vfunc? I thought that if you didn't override a vfunc in a subclass, that GType would copy the function pointer value.

(In reply to comment #15)
> Review of attachment 215016 [details] [review]:
> 
> ::: src/compositor/meta-plugin.c
> @@ -98,3 @@
> 
> -  if (G_OBJECT_CLASS (meta_plugin_parent_class)->constructed)
> -      G_OBJECT_CLASS (meta_plugin_parent_class)->constructed (object);
> 
> Are there any classes with a 0'd out constructed vfunc? I thought that if you
> didn't override a vfunc in a subclass, that GType would copy the function
> pointer value.

But in this case we *are* overriding constructed.

Review of attachment 215018 [details] [review]:

Looks right.  (I wonder if Tizen uses a mutter plugin still?  Should we send them a mail?)

We figured it out in person. Apparently constructed used to be NULL a year ago:

http://git.gnome.org/browse/glib/commit/?id=634e9e43cfb8b0d88d0a6b4899d0e33c62c07458

(In reply to comment #17)
> Review of attachment 215018 [details] [review]:
> 
> Looks right.  (I wonder if Tizen uses a mutter plugin still?

Tizen does not use Clutter / Mutter.

Review of attachment 215019 [details] [review]:

One totally minor thing...

::: src/compositor/meta-plugin-manager.c
@@ +275,3 @@
+              retval = TRUE;
+              meta_plugin_manager_kill_window_effects (
+                                                       plugin_mgr,

I'm not sure if it's just a tabs/spaces visualization issue, but the indentation here looks weird now.  Just like it up normally since the nesting is smaller now?

Review of attachment 215020 [details] [review]:

::: src/compositor/meta-plugin-manager.c
@@ +218,3 @@
+          meta_plugin_manager_kill_window_effects (
+                                                   plugin_mgr,
+                                                   actor);

Same weird indentation from last patch.

@@ +299,3 @@
     return FALSE;
 
+  if (klass->xevent_filter && klass->xevent_filter (plugin, xev))

I wouldn't toss the whole comment here.  The first paragraph describes the issue of whether or not the plugin has an xevent filter, which is still relevant. 

(The second paragraph onwards though can go)

Attachment 215013 [details] pushed as f143fe3 - util: Don't generate a backtrace on every G_LOG
Attachment 215015 [details] pushed as 33e1017 - main: Don't call g_type_init from meta_init
Attachment 215016 [details] pushed as 7c1b734 - meta-plugin: Remove some cruft
Attachment 215017 [details] pushed as 80a70a4 - mutter: Only allow one plugin to be loaded
Attachment 215018 [details] pushed as 9fa5aa9 - meta-plugin: Kill off "features"
Attachment 215019 [details] pushed as 574c0c3 - meta-plugin: Remove "disabled" feature
Attachment 215020 [details] pushed as f9454e2 - meta-plugin-manager: Only allow one plugin to be loaded


Pushed everything besides the custom log handler commit.