Created attachment 76598 [details] [review]
patch to fix warnings and leaks

Ping?

I don't like exposing the private data in the registry.c structs/enums in registry.h.

Mostly it looks correct/OK, except where it results in new stuff being exposed in the headers...

Comment on attachment 76598 [details] [review]
patch to fix warnings and leaks

>Index: atk-bridge/bridge.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/atk-bridge/bridge.c,v
>retrieving revision 1.97
>diff -u -p -r1.97 bridge.c
>--- atk-bridge/bridge.c	7 Nov 2006 00:23:53 -0000	1.97
>+++ atk-bridge/bridge.c	14 Nov 2006 22:16:31 -0000
>@@ -147,7 +147,7 @@ extern void gnome_accessibility_module_i
> extern void gnome_accessibility_module_shutdown (void);
> 
> static void
>-spi_atk_bridge_init_event_type_consts ()
>+spi_atk_bridge_init_event_type_consts (void)

Yes; thanks Kjartan for all these explicit 'void' declarations.

...
>-	  ior = (char *) spi_atk_bridge_get_registry_ior ();
>+	  ior = spi_atk_bridge_get_registry_ior ();

OK...

> 
>-	  if (ior != NULL)
>+	  if (ior != NULL) {
> 	       registry = CORBA_ORB_string_to_object (bonobo_activation_orb_get (), 
> 						      ior, &ev);
>-	  else {
>+	       XFree (ior);

Oops!  Thanks Kjartan.

...
>-int
>-gtk_module_init (gint *argc, gchar **argv[])
>-{
>-	return atk_bridge_init (argc, argv);
>-}

???  Isn't this required?  atk-bridge needs to remain initializable as a gtk-module...  

> static void
> add_signal_listener (const char *signal_name)
> {
>@@ -591,7 +586,7 @@ spi_atk_bridge_exit_func (void)
>   if (!bonobo_is_initialized ())
>     {
>       DBG (1, g_warning ("Re-initializing bonobo\n"));
>-      g_assert (bonobo_init (0, NULL));
>+      g_assert (bonobo_init (NULL, NULL));

Yes.

>@@ -1279,7 +1274,7 @@ spi_atk_bridge_init_base (CORBA_any *any
>     const gchar *s = atk_object_get_name (obj);
>     *app = spi_accessible_new_return (atk_get_root (), FALSE, NULL);
>     *role = spi_role_from_atk_role (atk_object_get_role (obj));
>-    *name = s ? s : ""; /* string gets dup-ed in util.c spi_init_any_* */
>+    *name = (char *)s ? (char *)s : ""; /* string gets dup-ed in util.c spi_init_any_* */

Shouldn't this cast really be (CORBA_string *) ?

...
>diff -u -p -r1.15 spi_hypertext.c
>--- cspi/spi_hypertext.c	13 Sep 2002 13:08:58 -0000	1.15
>+++ cspi/spi_hypertext.c	14 Nov 2006 22:16:31 -0000
>@@ -94,7 +94,7 @@ AccessibleHypertext_getLink (AccessibleH
> {
>   AccessibleHyperlink *retval;
> 	
>-  cspi_return_val_if_fail (obj != NULL, FALSE);
>+  cspi_return_val_if_fail (obj != NULL, NULL);

OK...

>Index: cspi/spi_main.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/cspi/spi_main.c,v
>retrieving revision 1.44
>diff -u -p -r1.44 spi_main.c
>--- cspi/spi_main.c	27 Mar 2006 18:05:31 -0000	1.44
>+++ cspi/spi_main.c	14 Nov 2006 22:16:31 -0000
>@@ -35,7 +35,7 @@
> 
> #undef DEBUG_OBJECTS
> 
>-static CORBA_Environment ev = { 0 };
>+static CORBA_Environment ev = { NULL };

Not sure either of these are safe - since CORBA_Environment struct is
generated by the idl compiler...

...
>@@ -430,7 +430,7 @@ SPI_event_quit (void)
>  *
>  **/
> SPIBoolean
>-SPI_eventIsReady ()
>+SPI_eventIsReady (void)

Yes.

>Index: cspi/spi_registry.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/cspi/spi_registry.c,v
>retrieving revision 1.49
>diff -u -p -r1.49 spi_registry.c
>--- cspi/spi_registry.c	4 Sep 2003 21:26:11 -0000	1.49
>+++ cspi/spi_registry.c	14 Nov 2006 22:16:31 -0000
>@@ -197,7 +197,7 @@ SPI_deregisterGlobalEventListener (Acces
>  * Returns: an integer indicating the number of active virtual desktops.
>  **/
> int
>-SPI_getDesktopCount ()
>+SPI_getDesktopCount (void)
> {
>   int retval;
> 
>Index: cspi/bonobo/cspi-bonobo-listener.h
>===================================================================
>RCS file: /cvs/gnome/at-spi/cspi/bonobo/cspi-bonobo-listener.h,v
>retrieving revision 1.9
>diff -u -p -r1.9 cspi-bonobo-listener.h
>--- cspi/bonobo/cspi-bonobo-listener.h	10 Dec 2002 11:45:47 -0000	1.9
>+++ cspi/bonobo/cspi-bonobo-listener.h	14 Nov 2006 22:16:31 -0000
>@@ -64,6 +64,7 @@ void cspi_device_listener_add_cb (Access
> void cspi_device_listener_remove_cb (AccessibleDeviceListener  *al,
>                                      AccessibleDeviceListenerCB callback);
> void cspi_device_listener_unref (AccessibleDeviceListener *listener);
>+CORBA_Object cspi_device_listener_get_corba (AccessibleDeviceListener *listener);

OK.

> 
> 
> G_END_DECLS
>Index: cspi/bonobo/cspi-bonobo.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/cspi/bonobo/cspi-bonobo.c,v
>retrieving revision 1.17
>diff -u -p -r1.17 cspi-bonobo.c
>--- cspi/bonobo/cspi-bonobo.c	13 Jul 2006 14:00:14 -0000	1.17
>+++ cspi/bonobo/cspi-bonobo.c	14 Nov 2006 22:16:31 -0000
>@@ -118,10 +118,11 @@ CORBA_Object
> cspi_init (void)
> {
>   char *obj_id;
>+  char *text;
>   CORBA_Object registry;
>   CORBA_Environment ev;
> 
>-  if (!bonobo_init (0, NULL))
>+  if (!bonobo_init (NULL, NULL))

Yes.

>     {
>       g_error ("Could not initialize Bonobo");
>     }
>@@ -138,8 +139,10 @@ cspi_init (void)
> 
>   if (ev._major != CORBA_NO_EXCEPTION)
>     {
>+      text = bonobo_exception_get_text (&ev);
>       g_error ("AT-SPI error: during registry activation: %s\n",
>-	       bonobo_exception_get_text (&ev));
>+	       text);
>+      g_free (text);

Yes.

>     }
> 
>   if (registry == CORBA_OBJECT_NIL)
>Index: libspi/accessible.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/libspi/accessible.c,v
>retrieving revision 1.59
>diff -u -p -r1.59 accessible.c
>--- libspi/accessible.c	22 Aug 2006 15:06:42 -0000	1.59
>+++ libspi/accessible.c	14 Nov 2006 22:16:31 -0000
>@@ -34,7 +34,6 @@
> #define PARENT_TYPE SPI_TYPE_BASE
> 
> static gboolean spi_init_role_lookup_table (Accessibility_Role *role_table);
>-Accessibility_Role spi_accessible_role_from_atk_role (AtkRole role);

Would it be better to move this to util.c/util.h instead?

> 
> static gboolean
> spi_init_role_lookup_table (Accessibility_Role *role_table)
>Index: libspi/accessible.h
>===================================================================
>RCS file: /cvs/gnome/at-spi/libspi/accessible.h,v
>retrieving revision 1.15
>diff -u -p -r1.15 accessible.h
>--- libspi/accessible.h	13 Sep 2002 13:09:00 -0000	1.15
>+++ libspi/accessible.h	14 Nov 2006 22:16:31 -0000
>@@ -50,6 +50,7 @@ SpiAccessible           *spi_accessible_
> Accessibility_Accessible spi_accessible_new_return (AtkObject         *o,
> 						    gboolean           release_ref,
> 						    CORBA_Environment *ev);
>+Accessibility_Role spi_accessible_role_from_atk_role (AtkRole role);


(see above)

> G_END_DECLS
> 
>Index: libspi/document.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/libspi/document.c,v
>retrieving revision 1.3
>diff -u -p -r1.3 document.c
>--- libspi/document.c	22 Aug 2006 15:06:42 -0000	1.3
>+++ libspi/document.c	14 Nov 2006 22:16:31 -0000
>@@ -127,7 +127,6 @@ impl_getAttributes (PortableServer_Serva
>   atk_attribute_set_free (attributes);
> 
>   return retval;
>-  
> }
> 
> 
>Index: libspi/streamablecontent.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/libspi/streamablecontent.c,v
>retrieving revision 1.5
>diff -u -p -r1.5 streamablecontent.c
>--- libspi/streamablecontent.c	27 Jun 2006 15:00:57 -0000	1.5
>+++ libspi/streamablecontent.c	14 Nov 2006 22:16:31 -0000
>@@ -24,6 +24,7 @@
> 
> #include <config.h>
> #include <stdio.h>
>+#include <string.h>
> #include <libspi/accessible.h>
> #include <libspi/component.h>
> #include <libspi/streamablecontent.h>
>Index: libspi/util.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/libspi/util.c,v
>retrieving revision 1.12
>diff -u -p -r1.12 util.c
>--- libspi/util.c	31 Jul 2006 22:32:03 -0000	1.12
>+++ libspi/util.c	14 Nov 2006 22:16:31 -0000
>@@ -26,6 +26,7 @@
> #include <glib/gslist.h>
> #include <Accessibility.h>
> 
>+#include "accessible.h"

As I noted, perhaps this would be better moved here, and have accessible.c include util.h.

> #include "spi-private.h"
> 
> typedef struct {
>@@ -66,7 +67,7 @@ spi_re_entrant_list_delete_link (GList *
>   next = element->next;
>   first_item = (element->prev == NULL);
> 
>-  g_list_remove_link (NULL, element);
>+  element = g_list_remove_link (NULL, element);

Yes, thanks!


The login-helper changes seem good, thanks.

>Index: login-helper/login-helper.h
...
>Index: registryd/deviceeventcontroller.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/registryd/deviceeventcontroller.c,v
>retrieving revision 1.94
>diff -u -p -r1.94 deviceeventcontroller.c
>--- registryd/deviceeventcontroller.c	7 Nov 2006 16:51:19 -0000	1.94
>+++ registryd/deviceeventcontroller.c	14 Nov 2006 22:16:31 -0000
>@@ -478,7 +478,7 @@ spi_dec_mouse_check (SpiDEController *co
>   Accessibility_EventDetails *details;
>   CORBA_Environment ev;
>   int win_x_return,win_y_return;
>-  unsigned int mask_return;
>+  unsigned int mask_return = 0;


Yes.

>   Window root_return, child_return;
>   Display *display = spi_get_display ();
> 
>@@ -1677,7 +1677,7 @@ spi_controller_update_key_grabs (SpiDECo
> {
>   GList *l, *next;
>   gboolean   update_failed = FALSE;
>-  KeyCode keycode;
>+  KeyCode keycode = 0;

Yes.   
>   g_return_val_if_fail (controller != NULL, FALSE);
> 
>@@ -2435,10 +2435,7 @@ spi_device_event_controller_class_init (
> }
> 
> #ifdef HAVE_XEVIE
>-Bool isEvent(dpy,event,arg)
>-     Display *dpy;
>-     XEvent *event;
>-     char *arg;
>+Bool isEvent (Display *dpy, XEvent *event, char *arg)

This seems sort of pointless, but OK.

> {
>    return TRUE;
> }
>Index: registryd/deviceeventcontroller.h
>===================================================================
>RCS file: /cvs/gnome/at-spi/registryd/deviceeventcontroller.h,v
>retrieving revision 1.15
>diff -u -p -r1.15 deviceeventcontroller.h
>--- registryd/deviceeventcontroller.h	6 Aug 2003 16:24:01 -0000	1.15
>+++ registryd/deviceeventcontroller.h	14 Nov 2006 22:16:31 -0000
>@@ -59,7 +59,11 @@ typedef struct {
> 
> GType            spi_device_event_controller_get_type (void);
> SpiDEController *spi_device_event_controller_new      (SpiRegistry *registry);
>-
>+int _spi_controller_device_error_handler (Display *display, XErrorEvent *error);
>+#ifdef HAVE_XEVIE
>+Bool isEvent (Display *dpy, XEvent *event, char *arg);
>+gboolean handle_io (GIOChannel *source, GIOCondition condition, gpointer data);
>+#endif
> G_END_DECLS
> 
> #endif /* DEVICEEVENTCONTROLLER_H_ */
>Index: registryd/registry.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/registryd/registry.c,v
>retrieving revision 1.74
>diff -u -p -r1.74 registry.c
>--- registryd/registry.c	7 Nov 2006 16:51:20 -0000	1.74
>+++ registryd/registry.c	14 Nov 2006 22:16:31 -0000
>@@ -32,6 +32,7 @@
> #  include <stdio.h>
> #endif
> 
>+#include <gdk/gdk.h>

I'd rather get rid of gdk_init() from registry.c.  Why do we need it?

> #include <bonobo/bonobo-exception.h>
> #include "../libspi/spi-private.h"
> #include "registry.h"
>@@ -49,17 +50,6 @@ static GQuark _state_changed_focused_qua
> 
> int _dbg = 0;
> 
>-typedef enum {
>-  ETYPE_FOCUS,
>-  ETYPE_OBJECT,
>-  ETYPE_PROPERTY,
>-  ETYPE_WINDOW,
>-  ETYPE_TOOLKIT,
>-  ETYPE_KEYBOARD,
>-  ETYPE_MOUSE,
>-  ETYPE_LAST_DEFINED
>-} EventTypeCategory;
>-

This is where I have a problem with the patch.  This struct should not be exposed IMO, and there should be no need to expose it since it's only used within registry.c.

> typedef struct {
>   const char *event_name;
>   EventTypeCategory type_cat;
>@@ -67,12 +57,6 @@ typedef struct {
>   GQuark minor;  /* from string segment[1]+segment[2] */
>   GQuark detail; /* from string segment[3] (not concatenated) */
> } EventTypeStruct;
>-
>-typedef struct {
>-  Accessibility_EventListener listener;
>-  GQuark            event_type_quark;
>-  EventTypeCategory event_type_cat;
>-} SpiListenerStruct;
> 


Same here - please don't move this.


> static void
> spi_registry_set_debug (const char *debug_flag_string)
>Index: registryd/registry.h
>===================================================================
>RCS file: /cvs/gnome/at-spi/registryd/registry.h,v
>retrieving revision 1.19
>diff -u -p -r1.19 registry.h
>--- registryd/registry.h	7 Nov 2006 00:23:54 -0000	1.19
>+++ registryd/registry.h	14 Nov 2006 22:16:31 -0000
>@@ -40,6 +40,17 @@ G_BEGIN_DECLS
> #define SPI_IS_REGISTRY(o)       (G_TYPE_CHECK__INSTANCE_TYPE ((o), SPI_REGISTRY_TYPE))
> #define SPI_IS_REGISTRY_CLASS(k) (G_TYPE_CHECK_CLASS_TYPE ((k), SPI_REGISTRY_TYPE))
> 
>+typedef enum {
>+  ETYPE_FOCUS,
>+  ETYPE_OBJECT,
>+  ETYPE_PROPERTY,
>+  ETYPE_WINDOW,
>+  ETYPE_TOOLKIT,
>+  ETYPE_KEYBOARD,
>+  ETYPE_MOUSE,
>+  ETYPE_LAST_DEFINED
>+} EventTypeCategory;
>+

No.

> struct _SpiRegistry {
>   SpiListener      parent;
> 
>@@ -56,6 +67,12 @@ struct _SpiRegistry {
> };
> 
> typedef struct {
>+  Accessibility_EventListener listener;
>+  GQuark            event_type_quark;
>+  EventTypeCategory event_type_cat;
>+} SpiListenerStruct;
>+
>+typedef struct {

No.

>   SpiListenerClass parent_class;
> 
>   POA_Accessibility_Registry__epv epv;
>@@ -63,6 +80,8 @@ typedef struct {
> 
> GType        spi_registry_get_type (void);
> SpiRegistry *spi_registry_new      (void);
>+SpiListenerStruct *spi_listener_struct_new (Accessibility_EventListener listener, CORBA_Environment *ev);
>+void         spi_listener_struct_free (SpiListenerStruct *ls, CORBA_Environment *ev);

No.

> 
> G_END_DECLS
> 
>Index: registryd/ucs2keysym.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/registryd/ucs2keysym.c,v
>retrieving revision 1.2
>diff -u -p -r1.2 ucs2keysym.c
>--- registryd/ucs2keysym.c	19 Apr 2004 11:07:12 -0000	1.2
>+++ registryd/ucs2keysym.c	14 Nov 2006 22:16:31 -0000
>@@ -793,6 +793,9 @@ struct codepair {
>   { 0x0ef7, 0x318e }, /*               Hangul_AraeAE ㆎ HANGUL LETTER ARAEAE */
>  };
> 
>+KeySym ucs2keysym (long ucs);
>+long keysym2ucs(KeySym keysym);
>+

Why do we have to forward reference these?

> KeySym ucs2keysym (long ucs)
> {
>     int min = 0;
>Index: test/key-listener-test.c

All the changes below are good, thanks a lot!

Bill

>===================================================================
>RCS file: /cvs/gnome/at-spi/test/key-listener-test.c,v
>retrieving revision 1.11
>diff -u -p -r1.11 key-listener-test.c
>--- test/key-listener-test.c	9 Jan 2006 13:37:44 -0000	1.11
>+++ test/key-listener-test.c	14 Nov 2006 22:16:31 -0000
>@@ -114,7 +114,7 @@ main (int argc, char **argv)
> }
> 
> static void
>-simple_at_exit ()
>+simple_at_exit (void)
> {
>   SPI_deregisterAccessibleKeystrokeListener (command_key_listener, SPI_KEYMASK_ALT | SPI_KEYMASK_CONTROL);
>   AccessibleKeystrokeListener_unref         (command_key_listener);
>Index: test/login-helper-server-test.c
>===================================================================
>RCS file: /cvs/gnome/at-spi/test/login-helper-server-test.c,v
>retrieving revision 1.6
>diff -u -p -r1.6 login-helper-server-test.c
>--- test/login-helper-server-test.c	9 Jan 2006 13:37:45 -0000	1.6
>+++ test/login-helper-server-test.c	14 Nov 2006 22:16:31 -0000
>@@ -126,7 +126,7 @@ test_get_raise_windows (LoginHelper *hel
> 
> 
> void
>-test_set_wm_dock ()
>+test_set_wm_dock (void)
> {
>   Atom atom_type[1], atom_window_type;
> 
>@@ -149,7 +149,7 @@ test_set_wm_dock ()
> }
> 
> static void
>-test_post_window ()
>+test_post_window (void)
> {
>         mainwin = gtk_window_new (GTK_WINDOW_TOPLEVEL);
>

Attaching a new patch that should only contain the safe parts. I'll attach a log with the remaining compiler warnings so we can continue discussing those after the safe parts are out of the way.

Created attachment 80991 [details] [review]
patch with cleanups

Safe bits

Thanks Kjartan, looks good :-)

Commited everything but the atk-bridge part since I had added a few more fixes there. Attaching a new patch for that.

Created attachment 81105 [details] [review]
more fixes

Does the last patch here look ok too?

Most of them are OK.
I don't think
-  misc = atk_misc_get_instance();
+  misc = (AtkMisc *)atk_misc_get_instance();
can fix the problem. I'm thinking change atk_misc_threads_enter/leave to atk_misc_threads_enter\leave (const AtkMisc *misc)
Will this break API?

*** Bug 381714 has been marked as a duplicate of this bug. ***

*** Bug 410461 has been marked as a duplicate of this bug. ***

*** Bug 418302 has been marked as a duplicate of this bug. ***

Closing this as obsolete now...