Created attachment 73019 [details] [review]
patch

bill, pls. review it.

Comment on attachment 73019 [details] [review]
patch

Patch looks good, except that I'd prefer to see the Atk signals for document loading associated with the AtkDocument interface instead of AtkObject. (The other Atk signals are associated with AtkObject because ATK was written before glib supported GInterface signals).

See http://developer.gnome.org/doc/API/2.0/gobject/gtype-non-instantiable-classed.html for info on creating GSignals for GInterface classes...
I suppose we might not be able to change this for all of the ATK signals, because of bincompat rules, but for new interface types like AtkDocument I'd like to avoid putting new signals on AtkObject itself.

NEO: CORRECTION to my comment!  I re-read your patch, and I see that it is putting the signals in AtkDocument where we want them, thank you very much!  The ATK part looks exactly right to me.  I think that the only change to the patch needed is here in at-poke, and in the bridge:

+        { EVENT_TYPE_OBJECT,   "load-complete", NULL },
+        { EVENT_TYPE_OBJECT,   "reload", NULL },
+        { EVENT_TYPE_OBJECT,   "load-stopped", NULL },

I think we should add EVENT_TYPE_DOCUMENT here in at-poke, and change bridge.c so that the events emitted by AT-SPI are 

"document:load-complete" instead of "object:load-complete".  This means we shouldn't use add_signal_listener, or else we should modify add_signal_listener to take an additional string parameter so that the first substring in the event name is "document" instead of "object".

Thanks!

Bill

Created attachment 73071 [details] [review]
patch v2

currentlly there is no other info as url which is sent out with document signals. we'll add it as we know the exact info we need.

Neo, this patch looks great, thanks!  I'm building with it now and if all goes well I'll commit today.

Normally for such a change, across multiple modules (atk, at-spi, at-poke) we probably should file multiple bugs, one for each package/aspect.  Also the new signals in ATK need to be documented (due to Gnome documentation requirements for new API).

Actually I see a problem with the patch; it adds three methods to AtkDocument, and doesn't remove any of the pad functions.  That breaks binary compatibility!

So if we add these three signal handlers, we have to remove three pad functions.  Unfortunately we only have four pad functions left in AtkDocument.  This is a problem, especially since we also need a signals for document-attributes-changed (I think it would be OK to use this signal with a locale/lang attribute for locale changes as well).

I am not sure that signal handler methods are necessary in all cases for GSignal.  Do you know the answer to this?  I seem to recall that we don't always include signal handler methods for our GSignals in ATK...

looking closer, I don't see why we need the "class closure" (i.e. default signal handler) in this case. 

Created attachment 73089 [details] [review]
patch as applied to ATK

This patch addresses the ATK portion of the bug and is modified from Neo's patch.

Created attachment 73090 [details] [review]
patch as applied to AT-SPI, from Neo's original patch v2

Created attachment 73091 [details] [review]
at-poke patch, to connect at-poke to the Document signals.

Michael, is this patch OK to commit to at-poke?

(In reply to comment #9)
> looking closer, I don't see why we need the "class closure" (i.e. default
> signal handler) in this case. 
> 
yep, i agree.

bill, can you recommand me some material/books for atk/at-spi related coding, or even more general for glib/gobject coding?

currentlly i code with reading code and feel that i need charge with some related knowledge.

thanks in advance.

Bill - atk-poke patch looks fine; is there really nothing more we can do with the AtkDocument interface though in terms of a new side-pane ? :-)

Either way - looks good, it'd look better if we could #ifdef it for the specific ATK version so it continues to build with older atks.

document:attributes-changed is also added in firefox side.

bill, can you add it in atk/at-spi/at-poke side?

Are the first two patches in this bug obsolete by now? Ok to mark them as such?

first two patches marked obsolete.  Thanks for the reminder Kjartan!

reassign to Li Yuan to work on the at-poke patch to address Michael's comment #14.

Should we just modify the required version of atk in at-poke? I think people who use newest at-poke should use newest atk.

Unless there is a desparate need to require the latest, greatest atk/cspi etc. I'd like to try to avoid that - people use at-poke to test a11y on all manner of systems: particularly the stable, deployed & supported ones: SLED, RHEL etc. and these tend to have older versions.

So - we can easily add a compile conditional to do this (surely) ? :-) [ perhaps with some configure.in hackerage etc. ]

In yast2 recently I did:

configure.in:

verstxt=`y2tool version`
vers=`echo "$verstxt" | awk -F. '{ printf "%d", ($1 * 1000 + $2) * 1000 + $3;}'`
AC_DEFINE_UNQUOTED(YAST2_VERSION, $vers, [yast version for compile conditionals])

source code:

#if YAST2_VERSION >= 1002003
...
#endif

This should be adequate here surely - though you'll need to use pkg-config to get the version :-) [ and include config.h of course ]

Yes, it's easy. I just didn't realize that someone has this need. And I will modify the patch.

[Resetting QA Contact to newly introduced "at-spi-maint@gnome.bugs". 
Reason: So far it was impossible to watch changes in at-spi bug reports without following all the specific persons (Li Yuan, Bill Haneman, Jeff Wai, ...) and also their activity outside of at-spi reports.

IMPORTANT: Anyone interested in following all bug activity (including all maintainers) must watch the "at-spi-maint@gnome.bugs" dummy user by adding it to the 'Users to watch' list under Preferences->Email preferences. This is also the default procedure nowadays in GNOME when setting up new products.]

[Mass-resetting default assignee, see bug 705890. Please reclaim this bug report by setting the assignee to yourself if you still plan to work on this. Thanks!]

I'm going to go ahead and close this because it concerns a very old version.
Feel free to open a new bug if the problem persists.