Launchpad has imported 43 comments from the remote bug at https://bugzilla.mozilla.org/show_bug.cgi?id=693343.
If you reply to an imported comment from within Launchpad, your comment will be sent to the remote bug automatically. Read more about Launchpad's inter-bugtracker facilities at https://help.launchpad.net/InterBugTracking. ------------------------------------------------------------------------ On 2011-10-10T17:20:25+00:00 Mgorse wrote: Currently, Firefox determines that accessibility is enabled if GNOME_ACCESSIBILITY is set to 1 in the environment or if /desktop/gnome/interface/accessibility is set to True in gconf. The latter check successfully determines whether accessibility is enabled for GNOME 2 but not for GNOME 3. Currently, a distribution would need to, ie, patch the firefox loader script to set GNOME_ACCESSIBILITY=1 for Firefox to be accessible under GNOME 3. There is now a gsettings key (toolkit-accessibility in the org.gnome.desktop.interface schema), but this is specific to GNOME, and so using it may not be optimal with AT-SPI becoming usable in XFCE and eventually KDE. In order to handle this, the AT-SPI bus launcher (org.a11y.Bus) has a org.a11y.Status.IsEnabled property on the bus object (/org/a11y/bus). Under GNOME, this is a proxy for the GSettings key. Xfce will likely have code in the future to ensure that this property is set correctly. I would recommend that Firefox should check this property first, falling back to gconf if there is an error checking the property, but I'm not sure how best to handle this in the code. Currently, there is a system pref (config.use_system_prefs.accessibility) which maps to the gconf key. There is also code in several places that checks the value of GNOME_ACCESSIBILITY. So I guess we want to somehow have custom handling for this system prefernce, if that is doable, or write a function somewhere to test whether a11y is enabled (making the D-Bus call and falling back to checking the system pref on error) and use that in the places where it is needed to check the state of accessibility. Advice welcome. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/2 ------------------------------------------------------------------------ On 2011-10-21T20:12:13+00:00 Mgorse wrote: Created attachment 568753 Proposed patch. Check org.a11y.Status.IsEnabled to determine whether accessibility is enabled before checking gconf. This is adding more code that is in two places; I don't know if there is now a better way to handle it (see bug 390761). An alternate approach would be to have mozilla.sh call dbus-send and set GNOME_ACCESSIBILITY=1 if the dbus property is set to true and GNOME_ACCESSIBILITY is unset. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/3 ------------------------------------------------------------------------ On 2011-10-25T05:05:39+00:00 Ginn-chen-r wrote: I don't know how much time dbus will take. I hope it will not slow down startup time. Since libxul is always enabled now, i.e. nsWindows.cpp and nsApplicationAccessibleWrap.cpp are linked into libxul.so, I think you may not need to copy the code twice. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/4 ------------------------------------------------------------------------ On 2011-10-25T12:02:21+00:00 Trevor Saunders wrote: (In reply to Ginn Chen from comment #2) > I don't know how much time dbus will take. I hope it will not slow down > startup time. I'm a little concerned about this too, but I suspect doing the dbus call off the main thread will be enough work that we should get real numbers before deciding to do it. > Since libxul is always enabled now, i.e. nsWindows.cpp and > nsApplicationAccessibleWrap.cpp are linked into libxul.so, I think you may > not need to copy the code twice. I suspect this is correct, but I'm ok with not cleaning up the existing dupplicated code in this bug. What I'd suggest is put the function to check the dbus status in the a11y namespace and make it non-static and use in the widget check. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/5 ------------------------------------------------------------------------ On 2011-10-25T12:15:00+00:00 Trevor Saunders wrote: Comment on attachment 568753 Proposed patch. >+static bool >+test_a11y_dbus (bool *out) I think I'd use IsDBusA11yEnabled() >+{ >+ // XXX following code is copied from widget/src/gtk2/nsWindow.cpp >+ // we should put it somewhere that can be used from both modules >+ // see bug 390761 >+ bool retval = FALSE; >+#ifdef MOZ_ENABLE_DBUS >+ DBusConnection *bus; >+ DBusMessage *message = NULL, *reply = NULL; >+ DBusMessageIter iter, iter_variant, iter_struct; >+ dbus_bool_t d_result; >+ DBusError error; this isn't ansi c89 ;) please declare these closer to where they are used. >+ reply = dbus_connection_send_with_reply_and_block(bus, message, 1000, >&error); >+ if (!reply || >+ dbus_message_get_type (reply) != DBUS_MESSAGE_TYPE_METHOD_RETURN || >+ strcmp (dbus_message_get_signature (reply), "v") != 0) >+ goto exit; blank line btw what is "v" as a dbus signature? Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/6 ------------------------------------------------------------------------ On 2011-10-25T18:38:49+00:00 Roc-ocallahan wrote: Comment on attachment 568753 Proposed patch. Review of attachment 568753: ----------------------------------------------------------------- ::: accessible/src/atk/nsApplicationAccessibleWrap.cpp @@ +618,5 @@ > +test_a11y_dbus (bool *out) > +{ > + // XXX following code is copied from widget/src/gtk2/nsWindow.cpp > + // we should put it somewhere that can be used from both modules > + // see bug 390761 Why not fix this now? You can just make the widget version nonstatic and call it from here. @@ +625,5 @@ > + DBusConnection *bus; > + DBusMessage *message = NULL, *reply = NULL; > + DBusMessageIter iter, iter_variant, iter_struct; > + dbus_bool_t d_result; > + DBusError error; This is C++ code, just declare these where they're first assigned wherever possible @@ +627,5 @@ > + DBusMessageIter iter, iter_variant, iter_struct; > + dbus_bool_t d_result; > + DBusError error; > + const char *iface = "org.a11y.Status"; > + const char *member = "IsEnabled"; static const char iface[] = ...; static const char member[] = ...; @@ +636,5 @@ > + goto exit; > + > + message = dbus_message_new_method_call ("org.a11y.Bus", "/org/a11y/bus", > + > "org.freedesktop.DBus.Properties", > + "Get"); How fast is this? We're calling this on every widget creation, could this be slow? Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/7 ------------------------------------------------------------------------ On 2011-10-25T20:32:31+00:00 Trevor Saunders wrote: (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5) > Comment on attachment 568753 [diff] [details] [review] > Proposed patch. > > Review of attachment 568753 [diff] [details] [review]: > ----------------------------------------------------------------- > > ::: accessible/src/atk/nsApplicationAccessibleWrap.cpp > @@ +618,5 @@ > > +test_a11y_dbus (bool *out) > > +{ > > + // XXX following code is copied from widget/src/gtk2/nsWindow.cpp > > + // we should put it somewhere that can be used from both modules > > + // see bug 390761 > > Why not fix this now? You can just make the widget version nonstatic and > call it from here. > > @@ +625,5 @@ > > + DBusConnection *bus; > > + DBusMessage *message = NULL, *reply = NULL; > > + DBusMessageIter iter, iter_variant, iter_struct; > > + dbus_bool_t d_result; > > + DBusError error; > > This is C++ code, just declare these where they're first assigned wherever > possible > > @@ +627,5 @@ > > + DBusMessageIter iter, iter_variant, iter_struct; > > + dbus_bool_t d_result; > > + DBusError error; > > + const char *iface = "org.a11y.Status"; > > + const char *member = "IsEnabled"; > > static const char iface[] = ...; > static const char member[] = ...; > > @@ +636,5 @@ > > + goto exit; > > + > > + message = dbus_message_new_method_call ("org.a11y.Bus", > > "/org/a11y/bus", > > + > > "org.freedesktop.DBus.Properties", > > + "Get"); > > How fast is this? We're calling this on every widget creation, could this be > slow? oh? it probably isn't the fastest thing in the world, but I'm not really sure. If that code runs every time we create a widget (which I assume we do a lot) we should probably get rid of the gconf check there too. I'm not sure how fast gconf is, but I can't see it being terriffic. The really correct solution here would be to call the dbus method once on startup, and then ask dbus to send us a signal when it changes, but I for one have no idea how easy that will be to do (I don't know anything about how gecko currently interacts with dbus) Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/8 ------------------------------------------------------------------------ On 2011-10-27T00:35:11+00:00 Mgorse wrote: (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5) > > + const char *iface = "org.a11y.Status"; > > + const char *member = "IsEnabled"; > > static const char iface[] = ...; > static const char member[] = ...; If I do this, then I get a seg fault, whether I preface the reference with & or not. I need a char ** to pass to dbus_message_append_args. > @@ +636,5 @@ > > + goto exit; > > + > > + message = dbus_message_new_method_call ("org.a11y.Bus", > > "/org/a11y/bus", > > + > > "org.freedesktop.DBus.Properties", > > + "Get"); > > How fast is this? We're calling this on every widget creation, could this be > slow? On my laptop (2.4ghz Core 2 Duo), it takes 0.6ms for the call plus an additional millisecond the first time a connection to the session bus is established. I presume it would take longer on a slower machine. The current code in widget/src/gtk2 records the result in a static variable, so there it would only make the call once, but, regardless, I agree that it is better to only have the code in one place if possible, so I'm testing with the duplicate code removed. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/9 ------------------------------------------------------------------------ On 2011-10-27T03:56:31+00:00 Mgorse wrote: Created attachment 569898 Revised patch. Remove duplicate code; add method in nsIAccessibilityService to test whether a11y is enabled. Also, unref the dbus connection when we're done with it. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/10 ------------------------------------------------------------------------ On 2011-11-07T09:31:35+00:00 Trevor Saunders wrote: Created attachment 572408 patch Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/11 ------------------------------------------------------------------------ On 2011-11-07T15:23:05+00:00 Bolterbugz wrote: Comment on attachment 572408 patch Review of attachment 572408: ----------------------------------------------------------------- Please don't forget to add mgorse as an author if you've built on his work. ::: accessible/src/base/nsAccessibilityService.cpp @@ +1845,5 @@ > } > + > +#ifdef MOZ_ACCESSIBILITY_ATK > +bool > +ShouldA11yBeEnabled() Would probably be better to move this into our atk layer no? ::: accessible/src/base/nsAccessibilityService.h @@ +58,5 @@ > > +/** > + * Is platform accessibility enabled. > + */ > +bool ShouldA11yBeEnabled(); Despite the name is Linux/dbus specific right? Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/12 ------------------------------------------------------------------------ On 2011-11-07T15:53:19+00:00 Bolterbugz wrote: Try server doesn't seem to have debus? ../../../dist/system_wrappers/dbus/dbus.h:3:28: fatal error: dbus/dbus.h: No such file or directory Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/13 ------------------------------------------------------------------------ On 2011-11-07T18:39:00+00:00 Trevor Saunders wrote: Created attachment 572525 patch Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/14 ------------------------------------------------------------------------ On 2011-11-07T21:03:25+00:00 Hub-g wrote: (In reply to Trevor Saunders (:tbsaunde) from comment #12) > Created attachment 572525 [diff] [details] [review] > patch I can't compile with that patch. Can't seem to find the dbus/dbus.h header. There is probably a magic trick I forgot about. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/15 ------------------------------------------------------------------------ On 2011-11-07T21:26:44+00:00 Hub-g wrote: Comment on attachment 572525 patch Review of attachment 572525: ----------------------------------------------------------------- With this patch I can't compile. we need to add access to the dbus headers in accessible/src/base/Makefile.in ::: accessible/src/base/nsAccessibilityService.cpp @@ +1837,5 @@ > > //////////////////////////////////////////////////////////////////////////////// > > +namespace mozilla { > +namespace a11y { > + mozilla::a11y::FocusManager* We should remove the namespace qualifier here since we are inside it. @@ +1842,1 @@ > mozilla::a11y::FocusMgr() and here @@ +1842,5 @@ > mozilla::a11y::FocusMgr() > { > return nsAccessibilityService::gAccessibilityService; > } > + We are missing } } to close the namespaces above. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/16 ------------------------------------------------------------------------ On 2011-11-07T21:48:15+00:00 Trevor Saunders wrote: Created attachment 572602 PATCH NO IDEA WHAT WENT WRONG WITH THE LAST PATCH, i WAS SURE IT COMPILED LOCALLY, BUT THAT MAKES NO SENSE, i JUST HAVE NO IDEA. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/17 ------------------------------------------------------------------------ On 2011-11-07T21:55:03+00:00 Trevor Saunders wrote: Comment on attachment 572602 PATCH >+ >diff --git a/accessible/src/base/nsAccessibilityService.cpp >b/accessible/src/base/nsAccessibilityService.cpp >--- a/accessible/src/base/nsAccessibilityService.cpp >+++ b/accessible/src/base/nsAccessibilityService.cpp >@@ -81,6 +81,7 @@ > #include "nsTextFragment.h" > #include "mozilla/Services.h" > #include "nsEventStates.h" >+#include "prenv.h" artifact will remove before landing > #ifdef MOZ_XUL > #include "nsXULAlertAccessible.h" >@@ -1832,8 +1833,9 @@ > // Services > //////////////////////////////////////////////////////////////////////////////// > >-mozilla::a11y::FocusManager* >+ mozilla::a11y::FocusManager* > mozilla::a11y::FocusMgr() ugh, this not my day :( Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/18 ------------------------------------------------------------------------ On 2011-11-07T23:12:26+00:00 Hub-g wrote: Comment on attachment 572602 PATCH Review of attachment 572602: ----------------------------------------------------------------- ::: accessible/src/atk/nsApplicationAccessibleWrap.cpp @@ +912,5 @@ > + switch (dbus_message_iter_get_arg_type (&iter_variant)) { > + case DBUS_TYPE_STRUCT: > + // at-spi2-core 2.2.0-2.2.1 had a bug where it returned a struct > + dbus_message_iter_recurse (&iter_variant, &iter_struct); > + if (dbus_message_iter_get_arg_type (&iter_struct) != > DBUS_TYPE_BOOLEAN) { Did you mean == DBUS_TYPE_BOOLEAN instead? Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/19 ------------------------------------------------------------------------ On 2011-11-07T23:39:08+00:00 Hub-g wrote: Comment on attachment 572602 PATCH Review of attachment 572602: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessibilityService.cpp @@ +81,4 @@ > #include "nsTextFragment.h" > #include "mozilla/Services.h" > #include "nsEventStates.h" > +#include "prenv.h" is this needed? @@ +1833,4 @@ > // Services > > //////////////////////////////////////////////////////////////////////////////// > > + mozilla::a11y::FocusManager* I don't see the need for the indentation. just nitpicking here. ::: widget/src/gtk2/nsWindow.cpp @@ +1110,4 @@ > } > > #ifdef ACCESSIBILITY > + if (aState && a11y::ShouldA11yBeEnabled()) in nsAccessibilityService.h, ShouldA11yBeEnabled() is declared only we use ATK, while here it seems to be for the ACESSIBILITY. @@ +6475,4 @@ > void > nsWindow::DispatchEventToRootAccessible(PRUint32 aEventType) > { > + if (!a11y::ShouldA11yBeEnabled()) same as above. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/20 ------------------------------------------------------------------------ On 2011-11-07T23:49:12+00:00 Trevor Saunders wrote: (In reply to Hub Figuiere [:hub] from comment #18) > Comment on attachment 572602 [diff] [details] [review] > PATCH > > Review of attachment 572602 [diff] [details] [review]: > ----------------------------------------------------------------- > > ::: accessible/src/base/nsAccessibilityService.cpp > @@ +81,4 @@ > > #include "nsTextFragment.h" > > #include "mozilla/Services.h" > > #include "nsEventStates.h" > > +#include "prenv.h" > > is this needed? > > @@ +1833,4 @@ > > // Services > > > > //////////////////////////////////////////////////////////////////////////////// > > > > + mozilla::a11y::FocusManager* > > I don't see the need for the indentation. just nitpicking here. yeah, see my earlier comment, these two are fixed locally, I can upload a new patch if you like. > ::: widget/src/gtk2/nsWindow.cpp > @@ +1110,4 @@ > > } > > > > #ifdef ACCESSIBILITY > > + if (aState && a11y::ShouldA11yBeEnabled()) > > in nsAccessibilityService.h, ShouldA11yBeEnabled() is declared only we use > ATK, while here it seems to be for the ACESSIBILITY. I guess saying its only for atk is a little misleading, its really for desktop linux where we use atk / at-spi However there is a sort of implicit assumption that the gtk backend will be related to using atk accessibility (which is sort of reasonable since gtk uses atk internally). Another thing is that for atk we need general accessibility to suport atk. I'd be willing to improve the comment if you have ideas. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/21 ------------------------------------------------------------------------ On 2011-11-07T23:53:31+00:00 Roc-ocallahan wrote: Comment on attachment 572602 PATCH Review of attachment 572602: ----------------------------------------------------------------- More dbus usage ... Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/22 ------------------------------------------------------------------------ On 2011-11-08T00:02:32+00:00 Trevor Saunders wrote: (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > Comment on attachment 572602 [diff] [details] [review] > PATCH > > Review of attachment 572602 [diff] [details] [review]: > ----------------------------------------------------------------- > > More dbus usage ... true... I'm not saying I like it, but I think we need to get used to linux a11y using it. We haven't really had to deal with it since its all been hidden behind atk so far, but at-spi2 is all dbus and people have been using firefox a11y with at-spi2 for a while now, and we will need to drop support for at-spi over corba for e10s because of atk plug / socket only being available in at-spi2. A fall back of multiple accessible trees has been discused, but I'm not particularly interested personally. Finally whatever we think of it I doubt there's much we can do to change that at-spi2 is the future. All that said other ideas welcome! Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/23 ------------------------------------------------------------------------ On 2011-11-08T00:21:52+00:00 Hub-g wrote: (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > Comment on attachment 572602 [diff] [details] [review] > PATCH > > Review of attachment 572602 [diff] [details] [review]: > ----------------------------------------------------------------- > > More dbus usage ... To me it looks to be the saner way to do it without mandating Gnome3. Gsettings being worse. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/24 ------------------------------------------------------------------------ On 2011-11-08T01:57:22+00:00 Trevor Saunders wrote: Created attachment 572700 patch with nits fixed Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/25 ------------------------------------------------------------------------ On 2011-11-08T02:43:37+00:00 Karlt wrote: (In reply to Trevor Saunders (:tbsaunde) from comment #21) (In reply to Hub Figuiere [:hub] from comment #22) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > > More dbus usage ... I think roc was talking to me with this comment. DBus definitely sounds preferable to GSettings. (And DBus sounds like the right way to do GNOME 3 desktop settings stored with GSettings.) Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/26 ------------------------------------------------------------------------ On 2011-11-08T03:33:25+00:00 Trevor Saunders wrote: (In reply to Karl Tomlinson (:karlt) from comment #24) > (In reply to Trevor Saunders (:tbsaunde) from comment #21) > (In reply to Hub Figuiere [:hub] from comment #22) > > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment > > #20) > > > > More dbus usage ... > > I think roc was talking to me with this comment. > > DBus definitely sounds preferable to GSettings. > (And DBus sounds like the right way to do GNOME 3 desktop settings stored > with GSettings.) Oh, Ok, ftr I'm pretty confident in the dbus code at this point, I haven't really changed it much just try and make it more gecko styley and it looks fine to me. I beleive Hub has dealt with dbus some it seems like he thinks its reasonable too. Finally at-spi2 and the atk-bridge for dbus is basically Mike's at this point so I generally trust the dbus code he writes. any chance you can review this soon? It's a bit of a usability problem since currently as the bug says firefox appears to be inaccessible in modern linux installs. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/27 ------------------------------------------------------------------------ On 2011-11-08T06:01:41+00:00 Karlt wrote: Comment on attachment 572700 patch with nits fixed Can you provide function names and 8 lines of context with future patches, please? https://developer.mozilla.org/en/Installing_Mercurial#Configuration >+ DBusConnection* bus = dbus_bus_get(DBUS_BUS_SESSION, &error); dbus_bus_get() will sometimes call dbus_connection_set_exit_on_disconnect(TRUE), so that needs to be undone with another dbus_connection_set_exit_on_disconnect(). >+ reply = dbus_connection_send_with_reply_and_block(bus, message, 1000, &error); We are actively trying to shorten the start-up path to reduce start-up time. Blocking on DBus during creation of the first window wouldn't be helpful. Is it feasible to initialize on receipt of an async reply? Or I expect it would be possible to create a DBusPendingCall on window creation, and then only block on it and steal_reply when it is really needed? (In reply to Trevor Saunders (:tbsaunde) from comment #25) > It's a bit of a usability problem > since currently as the bug says firefox appears to be inaccessible in modern > linux installs. I thought GNOME 3 distros were at least providing a read-only GConf wrapper. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/28 ------------------------------------------------------------------------ On 2011-11-08T10:13:14+00:00 Trevor Saunders wrote: (In reply to Karl Tomlinson (:karlt) from comment #26) > Comment on attachment 572700 [diff] [details] [review] > patch with nits fixed > > Can you provide function names and 8 lines of context with future patches, > please? https://developer.mozilla.org/en/Installing_Mercurial#Configuration > > >+ DBusConnection* bus = dbus_bus_get(DBUS_BUS_SESSION, &error); > > dbus_bus_get() will sometimes call > dbus_connection_set_exit_on_disconnect(TRUE), so that needs to be undone with > another dbus_connection_set_exit_on_disconnect(). ... great > >+ reply = dbus_connection_send_with_reply_and_block(bus, message, 1000, > >&error); > > We are actively trying to shorten the start-up path to reduce start-up time. > Blocking on DBus during creation of the first window wouldn't be helpful. > > Is it feasible to initialize on receipt of an async reply? I'm not sure how bad this is in practice, but I don't forsee any huge problem with using pending calls to do this async. > (In reply to Trevor Saunders (:tbsaunde) from comment #25) > > It's a bit of a usability problem > > since currently as the bug says firefox appears to be inaccessible in modern > > linux installs. > > I thought GNOME 3 distros were at least providing a read-only GConf wrapper. tbh I'm not sure, but I know we had several people confused about the problem Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/29 ------------------------------------------------------------------------ On 2011-11-14T18:31:30+00:00 Hub-g wrote: Comment on attachment 572700 patch with nits fixed Review of attachment 572700: ----------------------------------------------------------------- This looks good to me. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/30 ------------------------------------------------------------------------ On 2011-11-16T13:53:56+00:00 Alexander Surkov wrote: Comment on attachment 572700 patch with nits fixed Review of attachment 572700: ----------------------------------------------------------------- I'm not looking into the platform specific logic since I bet you did that well, just integration part. canceling review until comments are addressed ::: accessible/src/atk/nsApplicationAccessibleWrap.cpp @@ +612,4 @@ > bool > nsApplicationAccessibleWrap::Init() > { > + if (ShouldA11yBeEnabled()) { if application accessible is initialized then accessibility must be enabled. What's a reason of this check? @@ +862,5 @@ > return NS_OK; > } > + > +namespace mozilla { > + namespace a11y { nit: no indent for a11y namespace @@ +865,5 @@ > +namespace mozilla { > + namespace a11y { > + > +bool > +ShouldA11yBeEnabled() that's weird something prototyped in nsAccessibilityService gets defined in nsApplicationAccessibleWrap. I can't think of better place but you should XXX comment in prototype I think @@ +869,5 @@ > +ShouldA11yBeEnabled() > +{ > + static bool sChecked = false, sShouldEnable = false; > + if (sChecked) > + return sShouldEnable; that's ok but curious why wouldn't use enum instead? @@ +928,5 @@ > + default: > + break; > + } > + > + dbus_done: goto is unusual style and it's not appreciated in c++ world in general. I'm fine if you're sure to keep it @@ +955,5 @@ > + do_GetService(sSysPrefService, &rv); > + if (NS_SUCCEEDED(rv) && sysPrefService) > + sysPrefService->GetBoolPref(sAccessibilityKey, &sShouldEnable); > + > + return sShouldEnable; nit: wrong indentation ::: accessible/src/base/nsAccessibilityService.h @@ +57,5 @@ > FocusManager* FocusMgr(); > > +/** > + * Is platform accessibility enabled. > + * only used on linux with atk for now. nit: o -> O @@ +61,5 @@ > + * only used on linux with atk for now. > + */ > +#ifdef MOZ_ACCESSIBILITY_ATK > +bool ShouldA11yBeEnabled(); > +#endif nit: put comment into #ifdef please ::: widget/src/gtk2/nsWindow.cpp @@ +6479,1 @@ > return; it appears nsWindow::Show() manages root accessible creation, so here you should check only if accessibility service instantiated. and then you have unique consumer of ShouldA11yBeEnabled (nsWindow::Show), maybe it's worth to keep this method somewhere in gtk2 code Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/31 ------------------------------------------------------------------------ On 2011-11-17T04:40:41+00:00 Alexander Surkov wrote: (In reply to alexander surkov from comment #29) > > bool > > nsApplicationAccessibleWrap::Init() > > { > > + if (ShouldA11yBeEnabled()) { > > if application accessible is initialized then accessibility must be enabled. > What's a reason of this check? I see you do that because of bug 390761. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/32 ------------------------------------------------------------------------ On 2011-11-20T04:26:47+00:00 Mgorse wrote: Created attachment 575724 patch #7 Add a PreInit() function to be called on window creation, that establishes a D-Bus connection and makes a pending call to determine whether accessibility is enabled. Have ShouldA11yBeEnabled block on this call if needed. Hopefully this will reduce start-up time slightly. (In theory we could probably only enable a11y on a response to the pending call, but this would be much more of an architectural change, and we would also need to keep in mind that currently we are still supporting gconf as a callback, so this approach is a lot simpler.) Also, call dbus_connection_set_exit_on_disconnect, and fix some nits. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/33 ------------------------------------------------------------------------ On 2011-11-29T01:33:47+00:00 Mgorse wrote: Created attachment 577444 Updated patch. Update to apply against current source. Add a couple of MOZ_A11Y_DBUS checks where they were missing. Remove some includes that are no longer needed. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/34 ------------------------------------------------------------------------ On 2011-11-29T03:31:00+00:00 Trevor Saunders wrote: (In reply to Mike Gorse from comment #32) > Created attachment 577444 [diff] [details] [review] > Updated patch. You didn't get the merge with bug 451161 right, you should probably check the use system pref first then dbus or gconf. I'm also thinking it might be nice to put this stuff in a new file, Alex thoughts? common nit, I tend to prefer if (x) return; foo(); to if (x) return; foo(); Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/35 ------------------------------------------------------------------------ On 2011-11-29T11:28:05+00:00 Alexander Surkov wrote: (In reply to Trevor Saunders (:tbsaunde) from comment #33) > I'm also thinking it might be nice to put this stuff in a new file, Alex > thoughts? new file might be nice but actually I'd like to figure out general concept how we should handle initialization. I filed bug 706051 for that. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/36 ------------------------------------------------------------------------ On 2011-12-14T20:25:35+00:00 Mgorse wrote: Created attachment 581750 Updated patch. Merge with 451161/705983. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/37 ------------------------------------------------------------------------ On 2011-12-15T03:33:02+00:00 Trevor Saunders wrote: Comment on attachment 581750 Updated patch. >+PreInit() >+{ >+#ifdef MOZ_ENABLE_DBUS >+ static bool sChecked = FALSE; >+ if (sChecked) >+ return; >+ sChecked = TRUE; nit, blank line before sChecked = true I'm tempted to think we should bail if GNOME_ACCESSIBILITY is set since we should bail after this if GNOME_ACCESSIBILITY is set, otherwise nobody will ever check the return message. >+ if (!bus) >+ return; >+ dbus_connection_set_exit_on_disconnect(bus, FALSE); nit, blank line. >+ if (a11yPendingCall) { nit,if ( !a11yPendingCall) goto dbus_done; also we don't usually put a11y in local variables. >+ dbusSuccess = true; >+ } >+ >+ break; nit, put break in block above. >diff --git a/widget/src/gtk2/nsWindow.cpp b/widget/src/gtk2/nsWindow.cpp > #ifdef ACCESSIBILITY >-#include "nsIAccessibilityService.h" >+#include "nsAccessibilityService.h" > #include "nsIAccessibleDocument.h" >-#include "prenv.h" >-#include "stdlib.h" > > using namespace mozilla; > using namespace mozilla::widget; btw while I suppose ti doesn't really happen since these are present later unconditionally why are these here? Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/38 ------------------------------------------------------------------------ On 2011-12-15T03:48:50+00:00 Trevor Saunders wrote: > @@ +928,5 @@ > > + default: > > + break; > > + } > > + > > + dbus_done: > > goto is unusual style and it's not appreciated in c++ world in general. I'm > fine if you're sure to keep it This code is pretty "Cish" because of the dbus api, and its a reasonably common view that gotos like this are good style for the special case of error handling in C. > ::: widget/src/gtk2/nsWindow.cpp > @@ +6479,1 @@ > > return; > > it appears nsWindow::Show() manages root accessible creation, so here you > should check only if accessibility service instantiated. you mean nsAccessibilityService::GetAccService() is non-null right? Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/39 ------------------------------------------------------------------------ On 2011-12-20T00:21:21+00:00 Karlt wrote: Comment on attachment 581750 Updated patch. Thank you for tidying this up and using the async API to do this in parallel. This approach looks good to me. I wonder whether the PreInit() in ShouldA11yBeEnabled() is really necessary or could be replaced with an assertion to check that PreInit had already been called. Perhaps it is needed for unit tests? >+static DBusPendingCall *a11yPendingCall = NULL; I'm not familiar with the naming conventions in this particular code, but usually static variables in Gecko have an "s" prefix. >+ DBusError error; >+ dbus_error_init(&error); >+ DBusConnection* bus = dbus_bus_get(DBUS_BUS_SESSION, &error); >+ if (!bus) >+ return; error is not actually used here, and so can be replaced with NULL. "By convention, all functions allow NULL instead of a DBusError*, so callers who don't care about the error can ignore it." >+ dbus_connection_send_with_reply(bus, message, &a11yPendingCall, 1000); >+ >+dbus_done: >+ if (message) >+ dbus_message_unref(message); I would move the dbus_message_unref to immediately after the send, before dbus_done, so that the "if (message)" check is not required. >+ if (bus) >+ dbus_connection_unref(bus); bus is always non-NULL here, so no need for the "if (bus)" check. >+ strcmp(dbus_message_get_signature (reply), "v")) Use DBUS_TYPE_VARIANT_AS_STRING instead of "v". >+ dbus_done: >+ if (reply) Usually labels are "out"dented to make them stand out. >- if (aState && sAccessibilityEnabled) { >+ if (aState && a11y::ShouldA11yBeEnabled()) > CreateRootAccessible(); >- } Please don't unbrace the block here. Convention in this file is heading towards always brace except for jump statements, but usually leave existing code as is. What does CreateRootAccessible() actually achieve? It looks like mRootAccessible is unused. Is it holding a reference so that DispatchEventToRootAccessible will somehow not need to create another nsAccessible? (In reply to Trevor Saunders (:tbsaunde) from comment #36) > Comment on attachment 581750 > >+ dbusSuccess = true; > >+ } > >+ > >+ break; > > nit, put break in block above. The "break" needs to be out of the block so as not to fall through to the DBUS_TYPE_BOOLEAN case when the type doesn't match, but the blank line there looks a bit odd to me. Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/40 ------------------------------------------------------------------------ On 2011-12-20T15:09:54+00:00 Trevor Saunders wrote: landed https://hg.mozilla.org/mozilla-central/rev/860fdd41cfed https://hg.mozilla.org/mozilla-central/rev/887abec76bca Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/41 ------------------------------------------------------------------------ On 2011-12-20T16:07:25+00:00 Bmo-edmorley wrote: Backed out per #developers: https://hg.mozilla.org/mozilla-central/rev/6af20fb62fb7 https://tbpl.mozilla.org/php/getParsedLog.php?id=8060865&tree=Firefox Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/42 ------------------------------------------------------------------------ On 2011-12-30T03:19:25+00:00 Trevor Saunders wrote: *** Bug 714198 has been marked as a duplicate of this bug. *** Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/48 ------------------------------------------------------------------------ On 2011-12-30T03:48:48+00:00 Karlt wrote: The tinderbox logs are no longer found at the link above. If there is no DBus daemon already running, I've seen DBus spawn a child process that doesn't properly disconnect from ssh sessions; I wonder whether something similar might be happening here. These IRC comments from http://bugzilla.glob.com.au/irc/?c=developers&a=date&s=2011-12-20&e=2011-12-21&h= may be helpful. <Ms2ger philor, fwiw, tbsaunde says he was green on try earlier <tbsaunde https://tbpl.mozilla.org/?tree=Try&rev=083fd66ad8f2 fwiw * philor loads up the try log <philor> not that I don't trust try to run the same things that m-c runs, but, well, I don't trust to try to run the same things <philor> interesting: try still does a couple of rather foolish and pointless steps, that wind up creating a profile which isn't actually used, and that's apparently enough <Ms2ger> philor, the first word I can think of to describe that is "Mozilla" <philor> so my wild guess, having been awake for 20 minutes, is that the patch introduces a shutdown hang or at least a shutdown really-slow, and try survives it by being foolish <philor> "firefox-bin -no-remote -profile /builds/slave/try-lnx64-dbg/build/obj-firefox/_leaktest/leakprofile/ http://localhost:8888/bloatcycle.html -P default" <philor> no idea how you could be saved by that bit of foolishness, either <philor> tbsaunde: in theory, build with ac_add_options --enable- debug and ac_add_options --enable-trace-malloc, cd objdir/_leaktest/, python leaktest.py, then python leaktest.py --trace-malloc malloc.log --shutdown-leaks=sdleak.log should repro <philor> tbsaunde: no idea who would give you a stack, since I don't even know what's happening - leaktest.py starts up an http server, runs the browser, according to the log the browser closed, according to the exit code leaktest.py closed and closed happy, but when it tries to run again, it's still running from before Reply at: https://bugs.launchpad.net/firefox/+bug/857153/comments/49 ** Changed in: firefox Status: Unknown => Confirmed ** Changed in: firefox Importance: Unknown => High -- You received this bug notification because you are a member of Desktop Packages, which is subscribed to firefox in Ubuntu. https://bugs.launchpad.net/bugs/857153 Title: Needs to get accessibility settings from GSettings Status in The Mozilla Firefox Browser: Confirmed Status in “firefox” package in Ubuntu: Triaged Status in “firefox” source package in Oneiric: Triaged Status in “firefox” source package in Precise: Triaged Bug description: Luke mentioned to me in Dublin that the screen reader doesn't work with Firefox when accessibility is enabled. This is because Firefox is still using GConf to check if accessibility is enabled, when it really needs to be using GSettings. I've just realized this is still the case, so I assume that the screen reader still won't work by default To manage notifications about this bug go to: https://bugs.launchpad.net/firefox/+bug/857153/+subscriptions -- Mailing list: https://launchpad.net/~desktop-packages Post to : [email protected] Unsubscribe : https://launchpad.net/~desktop-packages More help : https://help.launchpad.net/ListHelp
