Re: [g-a-devel]at-spi patch ...
- From: Bill Haneman <bill haneman sun com>
- To: Michael Meeks <michael ximian com>
- Cc: Darren Kenny sun com, Marc Mulcahy <marc mulcahy sun com>, accessibility mailing list <gnome-accessibility-devel gnome org>
- Subject: Re: [g-a-devel]at-spi patch ...
- Date: 16 Aug 2002 13:14:22 +0100
On Mon, 2002-08-12 at 18:08, Michael Meeks wrote:
> So,
>
> May I commit this ?
Hi Michael:
The patch looks good, I only have one issue which I'll mention below.
The 'onloan' concept looks to be a nice way to avoid having to ref/unref
every event source, causing the local cache to expand without end, etc.
and avoids the ugly failure of direct pointer comparisons for event
sources. I take it that the change to two bitfields (on_loan,
ref_count) from is considered bincompatible, since the ref_count field
was not exposed?
In case you were wondering about my use of local 'retval' variables in
some places (which you removed), I found them useful when using gdb. No
matter.
The only actual issue I have is with the commenting-out of the key
listener tests in test-simple. As you might guess, they "work for me",
thus I'd rather get to the bottom of why they are failing for you than
comment out an important regression test.
Please commit, but understand that I intend to re-enable keylistener
testing by default, and try to squash the bug you are seeing.
Regards,
Bill
> Brief Synopsis:
>
> The 'on_loan' bitflag is used to denote the fact that we have a live
> object reference for the lifetime of an incoming event notification.
> This is used to save lots of roundtrips dupping and freeing it [ or
> leaking and doing whacked out things on 'ref' as before ]. It cleans and
> simplifies the code, and makes it much more efficient.
>
> While we handle the case where an object already on loan is re-loaned
> and returned by a re-enterant event notification cleanly, this path is
> not efficient - since it should almost never happen. It could be made
> more efficient were that a likely case, by adding a 'loan_count' or
> somesuch. We also promote loaned references where possible to non-loaned
> to improve remote traffic efficiency.
>
> The patch also slightly redresses the copyright issue, and fixes a few
> other bugs / bits of brokenness that were lurking around. It would be a
> shame IMHO to ship a version with Gnome 2.0.1 that was known broken.
>
> May I commit ?
>
> Michael.
>
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/at-spi/ChangeLog,v
> retrieving revision 1.233
> diff -u -p -u -r1.233 ChangeLog
> --- ChangeLog 12 Aug 2002 09:49:38 -0000 1.233
> +++ ChangeLog 12 Aug 2002 17:06:40 -0000
> @@ -1,3 +1,34 @@
> +2002-08-12 Michael Meeks <michael ximian com>
> +
> + * test/test-simple.c (global_listener_cb): bin bogosity.
> + (test_keylisteners): disable, still doesn't work reliably,
> + certainly not on my system anyway.
> +
> + * atk-bridge/bridge.c (spi_atk_bridge_key_listener):
> + don't leak a reference on the DEC. This round-trip
> + fetching of the DEC per keystroke sucks, it should be
> + cached.
> +
> + * cspi/spi-private.h,
> + * cspi/cspi-lowlevel.h,
> + * cspi/bonobo/cspi-bonobo-listener.[ch],
> + * cspi/bonobo/cspi-bonobo.c: get the copyright
> + notices better - there is still a large amount of
> + work in at-spi falsely attributed solely to Sun.
> +
> + * cspi/spi_main.c (cspi_object_ref): kill bogus
> + hash lookup, just increment the ref.
> + (SPI_freeString): make explicit the fact that we
> + handle NULL strings just fine.
> + (report_leaked_ref): obey coding standards.
> + (cspi_object_hash, cspi_object_equal): kill retval.
> + (cspi_object_release): only release if not on loan.
> + (cspi_object_get_ref): add 'loan' concept, bin 'do_ref'.
> + (cspi_object_borrow, cspi_object_return): impl.
> +
> + * cspi/bonobo/cspi-bonobo-listener.c (cspi_event):
> + use cspi_object_borrow / return.
> +
> 2002-08-12 Darren Kenny <darren kenny sun com>
>
> * cspi/bonobo/cspi-bonobo-listener.c:
> Index: atk-bridge/bridge.c
> ===================================================================
> RCS file: /cvs/gnome/at-spi/atk-bridge/bridge.c,v
> retrieving revision 1.43
> diff -u -p -u -r1.43 bridge.c
> --- atk-bridge/bridge.c 20 Jun 2002 21:37:58 -0000 1.43
> +++ atk-bridge/bridge.c 12 Aug 2002 17:06:40 -0000
> @@ -575,6 +575,7 @@ spi_atk_bridge_key_listener (AtkKeyEvent
> result = Accessibility_DeviceEventController_notifyListenersSync (
> controller, &key_event, &ev);
>
> + bonobo_object_release_unref (controller, &ev);
> CORBA_exception_free (&ev);
> }
>
> Index: cspi/cspi-lowlevel.h
> ===================================================================
> RCS file: /cvs/gnome/at-spi/cspi/cspi-lowlevel.h,v
> retrieving revision 1.4
> diff -u -p -u -r1.4 cspi-lowlevel.h
> --- cspi/cspi-lowlevel.h 20 Jun 2002 12:10:34 -0000 1.4
> +++ cspi/cspi-lowlevel.h 12 Aug 2002 17:06:40 -0000
> @@ -2,7 +2,7 @@
> * AT-SPI - Assistive Technology Service Provider Interface
> * (Gnome Accessibility Project; http://developer.gnome.org/projects/gap)
> *
> - * Copyright 2001 Sun Microsystems Inc.
> + * Copyright 2002 Ximian, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Library General Public
> Index: cspi/spi-private.h
> ===================================================================
> RCS file: /cvs/gnome/at-spi/cspi/spi-private.h,v
> retrieving revision 1.8
> diff -u -p -u -r1.8 spi-private.h
> --- cspi/spi-private.h 13 Jun 2002 22:31:29 -0000 1.8
> +++ cspi/spi-private.h 12 Aug 2002 17:06:40 -0000
> @@ -2,7 +2,9 @@
> * AT-SPI - Assistive Technology Service Provider Interface
> * (Gnome Accessibility Project; http://developer.gnome.org/projects/gap)
> *
> - * Copyright 2001 Sun Microsystems Inc.
> + * Copyright 2002 Ximian, Inc.
> + * 2002 Sun Microsystems Inc.
> + *
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Library General Public
> @@ -33,7 +35,8 @@
> struct _Accessible {
> CORBA_Object objref;
> /* And some other bits */
> - guint ref_count;
> + guint on_loan : 1;
> + guint ref_count : 30;
> };
>
> #define CSPI_OBJREF(a) (((Accessible *)(a))->objref)
> @@ -41,13 +44,13 @@ struct _Accessible {
> CORBA_Environment *cspi_ev (void);
> SPIBoolean cspi_exception (void);
> Accessibility_Registry cspi_registry (void);
> -Accessible *cspi_object_add (CORBA_Object corba_object);
> -Accessible *cspi_object_add_ref (CORBA_Object corba_object,
> - gboolean do_ref);
> -void cspi_object_ref (Accessible *accessible);
> -void cspi_object_unref (Accessible *accessible);
> -SPIBoolean cspi_accessible_is_a (Accessible *obj,
> - const char *interface_name);
> +Accessible *cspi_object_add (CORBA_Object corba_object);
> +void cspi_object_ref (Accessible *accessible);
> +void cspi_object_unref (Accessible *accessible);
> +Accessible *cspi_object_borrow (CORBA_Object corba_object);
> +void cspi_object_return (Accessible *accessible);
> +SPIBoolean cspi_accessible_is_a (Accessible *accessible,
> + const char *interface_name);
>
> #define cspi_return_if_fail(val) \
> if (!(val)) \
> Index: cspi/spi_main.c
> ===================================================================
> RCS file: /cvs/gnome/at-spi/cspi/spi_main.c,v
> retrieving revision 1.26
> diff -u -p -u -r1.26 spi_main.c
> --- cspi/spi_main.c 20 Jun 2002 12:10:34 -0000 1.26
> +++ cspi/spi_main.c 12 Aug 2002 17:06:40 -0000
> @@ -39,11 +39,8 @@ static guint
> cspi_object_hash (gconstpointer key)
> {
> CORBA_Object object = (CORBA_Object) key;
> - guint retval;
> -
> - retval = CORBA_Object_hash (object, 0, &ev);
>
> - return retval;
> + return CORBA_Object_hash (object, 0, &ev);
> }
>
> static gboolean
> @@ -51,15 +48,12 @@ cspi_object_equal (gconstpointer a, gcon
> {
> CORBA_Object objecta = (CORBA_Object) a;
> CORBA_Object objectb = (CORBA_Object) b;
> - gboolean retval;
> -
> - retval = CORBA_Object_is_equivalent (objecta, objectb, &ev);
>
> - return retval;
> + return CORBA_Object_is_equivalent (objecta, objectb, &ev);
> }
>
> static void
> -cspi_object_release (gpointer value)
> +cspi_object_release (gpointer value)
> {
> Accessible *a = (Accessible *) value;
>
> @@ -67,7 +61,10 @@ cspi_object_release (gpointer value)
> g_print ("releasing %p => %p\n", a, a->objref);
> #endif
>
> - cspi_release_unref (a->objref);
> + if (!a->on_loan)
> + {
> + cspi_release_unref (a->objref);
> + }
>
> memset (a, 0xaa, sizeof (Accessible));
> a->ref_count = -1;
> @@ -78,18 +75,18 @@ cspi_object_release (gpointer value)
> }
>
> SPIBoolean
> -cspi_accessible_is_a (Accessible *obj,
> +cspi_accessible_is_a (Accessible *accessible,
> const char *interface_name)
> {
> SPIBoolean retval;
> Bonobo_Unknown unknown;
>
> - if (obj == NULL)
> + if (accessible == NULL)
> {
> return FALSE;
> }
>
> - unknown = Bonobo_Unknown_queryInterface (CSPI_OBJREF (obj),
> + unknown = Bonobo_Unknown_queryInterface (CSPI_OBJREF (accessible),
> interface_name, cspi_ev ());
>
> if (ev._major != CORBA_NO_EXCEPTION)
> @@ -135,7 +132,9 @@ Accessibility_Registry
> cspi_registry (void)
> {
> if (!cspi_ping (registry))
> - registry = cspi_init ();
> + {
> + registry = cspi_init ();
> + }
> return registry;
> }
>
> @@ -157,38 +156,14 @@ cspi_exception (void)
> return retval;
> }
>
> -Accessible *
> -cspi_object_new (CORBA_Object corba_object)
> -{
> - Accessible *ref;
> -
> - if (corba_object == CORBA_OBJECT_NIL)
> - {
> - ref = NULL;
> - }
> - else if (!cspi_check_ev ("pre method check: add"))
> - {
> - ref = NULL;
> - }
> - else
> - {
> - ref = malloc (sizeof (Accessible));
> -// ref->objref = CORBA_Object_duplicate (corba_object, cspi_ev ());
> - ref->objref = corba_object;
> - ref->ref_count = 1;
> - }
> -
> - return ref;
> -}
> -
> -Accessible *
> -cspi_object_add (CORBA_Object corba_object)
> -{
> - return cspi_object_add_ref (corba_object, FALSE);
> -}
> -
> -Accessible *
> -cspi_object_add_ref (CORBA_Object corba_object, gboolean do_ref)
> +/*
> + * This method swallows the corba_object BonoboUnknown
> + * reference, and returns an Accessible associated with it.
> + * If the reference is loaned, it means it is only valid
> + * between a borrow / return pair.
> + */
> +static Accessible *
> +cspi_object_get_ref (CORBA_Object corba_object, gboolean on_loan)
> {
> Accessible *ref;
>
> @@ -206,26 +181,30 @@ cspi_object_add_ref (CORBA_Object corba_
> {
> g_assert (ref->ref_count > 0);
> ref->ref_count++;
> - if (!do_ref)
> - cspi_release_unref (corba_object);
> + if (!on_loan)
> + {
> + if (ref->on_loan) /* Convert to a permanant ref */
> + {
> + ref->on_loan = FALSE;
> + }
> + else
> + {
> + cspi_release_unref (corba_object);
> + }
> + }
> #ifdef DEBUG_OBJECTS
> g_print ("returning cached %p => %p\n", ref, ref->objref);
> #endif
> }
> else
> {
> - ref = cspi_object_new (corba_object);
> -
> + ref = malloc (sizeof (Accessible));
> + ref->objref = corba_object;
> + ref->ref_count = 1;
> + ref->on_loan = on_loan;
> #ifdef DEBUG_OBJECTS
> g_print ("allocated %p => %p\n", ref, corba_object);
> #endif
> - if (do_ref) {
> -#ifdef JAVA_BRIDGE_BUG_IS_FIXED
> - g_assert (CORBA_Object_is_a (corba_object,
> - "IDL:Bonobo/Unknown:1.0", cspi_ev ()));
> -#endif
> - Bonobo_Unknown_ref (corba_object, cspi_ev ());
> - }
> g_hash_table_insert (cspi_get_live_refs (), ref->objref, ref);
> }
> }
> @@ -233,17 +212,42 @@ cspi_object_add_ref (CORBA_Object corba_
> return ref;
> }
>
> +Accessible *
> +cspi_object_add (CORBA_Object corba_object)
> +{
> + return cspi_object_get_ref (corba_object, FALSE);
> +}
> +
> +Accessible *
> +cspi_object_borrow (CORBA_Object corba_object)
> +{
> + return cspi_object_get_ref (corba_object, TRUE);
> +}
> +
> +void
> +cspi_object_return (Accessible *accessible)
> +{
> + g_return_if_fail (accessible != NULL);
> +
> + if (!accessible->on_loan ||
> + accessible->ref_count == 1)
> + {
> + cspi_object_unref (accessible);
> + }
> + else /* Convert to a permanant ref */
> + {
> + accessible->on_loan = FALSE;
> + accessible->objref = cspi_dup_ref (accessible->objref);
> + accessible->ref_count--;
> + }
> +}
> +
> void
> cspi_object_ref (Accessible *accessible)
> {
> g_return_if_fail (accessible != NULL);
>
> - if (g_hash_table_lookup (cspi_get_live_refs (), accessible->objref) == NULL) {
> - accessible->objref = cspi_dup_ref (accessible->objref);
> - g_hash_table_insert (cspi_get_live_refs (), accessible->objref, accessible);
> - } else {
> - accessible->ref_count++;
> - }
> + accessible->ref_count++;
> }
>
> void
> @@ -372,15 +376,25 @@ SPI_nextEvent (SPIBoolean waitForEvent)
> static void
> report_leaked_ref (gpointer key, gpointer val, gpointer user_data)
> {
> - Accessible *a = (Accessible *) val;
> - char *name, *role;
> - name = Accessible_getName (a);
> - if (cspi_exception ()) name = NULL;
> - role = Accessible_getRoleName (a);
> - if (cspi_exception ()) role = NULL;
> - fprintf (stderr, "leaked object %s, role %s\n", (name) ? name : "<?>",
> - (role) ? role : "<?>");
> - if (name) SPI_freeString (name);
> + char *name, *role;
> + Accessible *a = (Accessible *) val;
> +
> + name = Accessible_getName (a);
> + if (cspi_exception ())
> + {
> + name = NULL;
> + }
> +
> + role = Accessible_getRoleName (a);
> + if (cspi_exception ())
> + {
> + role = NULL;
> + }
> +
> + fprintf (stderr, "leaked %d references to object %s, role %s\n",
> + a->ref_count, name ? name : "<?>", role ? role : "<?>");
> +
> + SPI_freeString (name);
> }
>
>
> @@ -437,11 +451,15 @@ SPI_exit (void)
> *
> * Free a character string returned from an at-spi call. Clients of
> * at-spi should use this function instead of free () or g_free().
> + * A NULL string @s will be silently ignored.
> * This API should not be used to free strings
> * from other libraries or allocated by the client.
> **/
> void
> SPI_freeString (char *s)
> {
> - CORBA_free (s);
> + if (s)
> + {
> + CORBA_free (s);
> + }
> }
> Index: cspi/spi_text.c
> ===================================================================
> RCS file: /cvs/gnome/at-spi/cspi/spi_text.c,v
> retrieving revision 1.18
> diff -u -p -u -r1.18 spi_text.c
> --- cspi/spi_text.c 13 May 2002 07:48:57 -0000 1.18
> +++ cspi/spi_text.c 12 Aug 2002 17:06:40 -0000
> @@ -23,8 +23,7 @@
> #include <cspi/spi-private.h>
>
> static Accessibility_TEXT_BOUNDARY_TYPE
> -get_accessible_text_boundary_type (
> - AccessibleTextBoundaryType type)
> +get_accessible_text_boundary_type (AccessibleTextBoundaryType type)
> {
> switch (type)
> {
> Index: cspi/bonobo/cspi-bonobo-listener.c
> ===================================================================
> RCS file: /cvs/gnome/at-spi/cspi/bonobo/cspi-bonobo-listener.c,v
> retrieving revision 1.14
> diff -u -p -u -r1.14 cspi-bonobo-listener.c
> --- cspi/bonobo/cspi-bonobo-listener.c 12 Aug 2002 09:49:38 -0000 1.14
> +++ cspi/bonobo/cspi-bonobo-listener.c 12 Aug 2002 17:06:40 -0000
> @@ -2,7 +2,7 @@
> * AT-SPI - Assistive Technology Service Provider Interface
> * (Gnome Accessibility Project; http://developer.gnome.org/projects/gap)
> *
> - * Copyright 2001 Sun Microsystems Inc.
> + * Copyright 2002 Ximian Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Library General Public
> @@ -91,7 +91,7 @@ cspi_event (SpiEventListener *listene
> GList *l;
> CSpiEventListener *clistener = (CSpiEventListener *) listener;
> AccessibleEvent aevent;
> - Accessible *source = cspi_object_add (cspi_dup_ref(event->source));
> + Accessible *source = cspi_object_borrow (event->source);
>
> aevent.type = event->type;
> aevent.source = source;
> @@ -106,7 +106,7 @@ cspi_event (SpiEventListener *listene
> eh->cb.event (&aevent, eh->user_data);
> }
>
> - cspi_object_unref( source );
> + cspi_object_return (source);
> }
>
> static void
> Index: cspi/bonobo/cspi-bonobo-listener.h
> ===================================================================
> RCS file: /cvs/gnome/at-spi/cspi/bonobo/cspi-bonobo-listener.h,v
> retrieving revision 1.6
> diff -u -p -u -r1.6 cspi-bonobo-listener.h
> --- cspi/bonobo/cspi-bonobo-listener.h 13 May 2002 07:49:03 -0000 1.6
> +++ cspi/bonobo/cspi-bonobo-listener.h 12 Aug 2002 17:06:40 -0000
> @@ -2,7 +2,7 @@
> * AT-SPI - Assistive Technology Service Provider Interface
> * (Gnome Accessibility Project; http://developer.gnome.org/projects/gap)
> *
> - * Copyright 2001 Sun Microsystems Inc.
> + * Copyright 2002 Ximian, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Library General Public
> Index: cspi/bonobo/cspi-bonobo.c
> ===================================================================
> RCS file: /cvs/gnome/at-spi/cspi/bonobo/cspi-bonobo.c,v
> retrieving revision 1.5
> diff -u -p -u -r1.5 cspi-bonobo.c
> --- cspi/bonobo/cspi-bonobo.c 20 Jun 2002 12:10:35 -0000 1.5
> +++ cspi/bonobo/cspi-bonobo.c 12 Aug 2002 17:06:40 -0000
> @@ -3,6 +3,7 @@
> * (Gnome Accessibility Project; http://developer.gnome.org/projects/gap)
> *
> * Copyright 2001 Sun Microsystems Inc.
> + * 2002 Ximian Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Library General Public
> Index: docs/reference/cspi/tmpl/spi_accessible.sgml
> ===================================================================
> RCS file: /cvs/gnome/at-spi/docs/reference/cspi/tmpl/spi_accessible.sgml,v
> retrieving revision 1.9
> diff -u -p -u -r1.9 spi_accessible.sgml
> --- docs/reference/cspi/tmpl/spi_accessible.sgml 17 Apr 2002 00:20:54 -0000 1.9
> +++ docs/reference/cspi/tmpl/spi_accessible.sgml 12 Aug 2002 17:06:40 -0000
> @@ -20,6 +20,7 @@ Accessible Objects
> </para>
>
> @objref:
> + on_loan:
> @ref_count:
>
> <!-- ##### FUNCTION Accessible_ref ##### -->
> Index: test/test-simple.c
> ===================================================================
> RCS file: /cvs/gnome/at-spi/test/test-simple.c,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 test-simple.c
> --- test/test-simple.c 19 Jun 2002 13:47:11 -0000 1.29
> +++ test/test-simple.c 12 Aug 2002 17:06:40 -0000
> @@ -650,13 +650,6 @@ global_listener_cb (const AccessibleEven
>
> fprintf (stderr, "Fielded focus event ...\n");
>
> - /*
> - * must ref before doing "direct pointer" identity comparisons,
> - * e.g. "accessible == a".
> - * Alternatively, one can use cspi_object_equal (a, b)
> - */
> - Accessible_ref (event->source);
> -
> if (!do_poke) {
> desktop = SPI_getDesktop (0);
> application = Accessible_getChildAtIndex (desktop, 0);
> @@ -677,8 +670,6 @@ global_listener_cb (const AccessibleEven
>
> print_tree = TRUE;
> validate_accessible (event->source, TRUE, TRUE);
> -
> - Accessible_unref (event->source);
> }
>
> static SPIBoolean
> @@ -689,8 +680,10 @@ key_listener_cb (const AccessibleKeystro
>
> *s = *stroke;
>
> - if (s->type == SPI_KEY_PRESSED) key_press_received = TRUE;
> - else if (s->type == SPI_KEY_RELEASED) key_release_received = TRUE;
> + if (s->type == SPI_KEY_PRESSED)
> + key_press_received = TRUE;
> + else if (s->type == SPI_KEY_RELEASED)
> + key_release_received = TRUE;
>
> return TRUE;
> }
> @@ -699,6 +692,7 @@ key_listener_cb (const AccessibleKeystro
> static void
> test_keylisteners (void)
> {
> +#ifdef BILL_MAKES_THIS_WORK_RELIABLY
> int i;
> AccessibleKeystroke stroke;
> AccessibleKeystrokeListener *key_listener;
> @@ -742,6 +736,7 @@ test_keylisteners (void)
> g_assert (SPI_generateMouseEvent (-50, -50, "rel"));
> g_assert (SPI_generateMouseEvent (-50, -50, "rel"));
> g_assert (SPI_generateMouseEvent (-1, -1, "b1c"));
> +#endif
> }
>
> int
> --
> mmeeks gnu org <><, Pseudo Engineer, itinerant idiot
>
> _______________________________________________
> Gnome-accessibility-devel mailing list
> Gnome-accessibility-devel gnome org
> http://mail.gnome.org/mailman/listinfo/gnome-accessibility-devel
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
