480317 - Provide a way for atk-based plugins to slot into our a11y tree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

After talking to Brad Taylor, I'm convinced this would be a great way to have a seamless accessibility experience when focus goes inside a plugin. So as the user navigates a web page, if focus goes to the plugin, then the plugin's root accessible node/object is treated like any other. Perhaps a common practice will be for the first node to have a name like "you are now in foo" or something.

Brad's willing to do a lot of the work here.

Comment 1

[alexander :surkov (:asurkov)]

I thought about getting of our accessibility hierarchy into plugin after our discussion but haven't chance to form my thoughts, I'll try to do this on wiki soon.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: an update from brad

Comment 3

[Brad Taylor]

Attachment: WIP patch

This is a first cut of the accessibility support for plugins.  The mechanics of it appear to work, but I haven't tested hooking up an AtkObject yet, so it still needs some cleanup and more testing before it's ready to be reviewed.

Comment 4

[Ginn Chen]

Is there any plugin can be tested with?

I guess Helix should work.

Comment 5

[Brad Taylor]

Ginn, once I get this patch cleaned up, I'll attach a patch for Moonlight.  Any plugin can take advantage of it as long as they return their native accessible when NPN_GetValue(NPNVNativeAccessible, &val) is called.

Comment 6

[Brad Taylor]

Attachment: Updated WIP patch, currently getting a segfault

Here's the cleaned-up version, and a Moonlight patch.  Ginn, I'd love your feedback.

I'm currently getting a segfault which Alexander Surkov says is due to the Linux A11y support in Firefox not being ready.

Debugging around it, nsObjectFrame seems to want a nsAccessNode when I'm using a nsAccessibilityWrap (:nsIAccessible).  I don't really understand what the difference is, and where the two are used.  Worse yet, I see no documentation anywhere about either class so I can't figure it out on my own.

Here's the segfault:

Program received signal SIGSEGV, Segmentation fault.
0xa9e21e47 in CallQueryInterface<nsIAccessible, nsAccessNode> (aSource=0xad496844, aDestination=0xbfb95db4) at ../../../dist/include/xpcom/nsISupportsUtils.h:203
203	                                   reinterpret_cast<void**>(aDestination));

which is triggered from nsAccessibilityService.cpp:1255 (InitAccessible).

Comment 7

[Brad Taylor]

Attachment: WIP Patch against Moonlight Trunk

Comment 8

[Ginn Chen]

Brad,
With your patch, I can see the "Hello World" dummy accessible in accerciser.
When I click it, firefox crashed. But the stack not like yours, it is in Orbit2.

I guess it might be a bug of atk_no_op_object or something else.

I changed your patch for Moonlight a little bit, and the crash is gone.

If you still have problems, please let me know the reproduce step and the site you're browsing.

Comment 9

[Ginn Chen]

Attachment: WIP patch against Moonlight trunk (use gtk_label_new instead)

Comment 10

[Brad Taylor]

Ginn, I think that your change is pushing the error higher up the stack, and that you're missing the more fundamental problem with my Firefox change.

If you dig into the segfault, it's trying to cast nsIAccessible (in this case, nsAccessibleWrap which I'm using in nsHTMLAtkObjectOwnerAccessible) to an nsAccessNode.  This is allowed in the MSAA code (as I based my nsHTMLAtkObject off of nsHTMLWin32Object), but it appears to not be in the Atk code.  When I pointed this out to Alexander Surkov, he said this:

> I have a talk with Aaron and he told me how plugins a11y works on
> windows. So I see now you're code look correct. I think the problem is
> our ATK implementation isn't ready. But since this idea is simple, I
> think it should be easy to fix ATK code on our side. We just need to
> figure out what's wrong :). I suggest you to put your patches to the
> bug you started to work previously (where one of versions of your
> patch is presented already iirc) and move our discussion there. I'll
> ensure to cc Ginn there - he is our linux a11y guru. And please attach
> stacktrace of your segmentation fault, it should make things clearer
> until we have testcase to play with.

Does this make more sense to you?

Comment 11

[Brad Taylor]

Please disregard my earlier comments.  On closer inspection, I'm no longer getting the segfault with your changes (awesome) and see the label in the hierarchy!  Woo!  What are the next steps to get this patch integrated?  Thanks!

Comment 12

[Ginn Chen]

In general, your patch looks good.

I need some more time to think about the details.

1) Do we need g_object_ref / g_object_unref for the AtkObject?
2) Which atk role should be used for nsHTMLAtkObjectAccessible?
3) We need to do atk_obejct_set_parent(), otherwise the hierarchy is broken from bottom to top.
4) Is everything fine on creating and destroying the plugin?
5) Do we really need to change NPAPI interface? Can we get the socketWidget and do gtk_widget_get_accessible() ?

Also it would be awesome if we can have a simple gtk based plugin to demonstrate the atk hierarchy inside the plugin is seamlessly connected to Firefox.
It would be even better if we can demo it with Orca.

Comment 13

[Brad Taylor]

Great, thanks for breaking this down for me.

> 1) Do we need g_object_ref / g_object_unref for the AtkObject?

Actually, it looks like this is happening anyway -- when the parent has its children reffed, and in nsAccessibleWrap::Shutdown().

> 2) Which atk role should be used for nsHTMLAtkObjectAccessible?

nsHTMLAtkObjectAccessible should provide its own role as returned from the plugin, but nsHTMLAtkObjectOwnerAccessible is probably fine as a ATK_OBJECT_UNKNOWN.  Personally, I'd prefer to make this an ATK_OBJECT_FILLER, but it seems that UNKNOWN is used frequently enough in Firefox that it's appropriate for an accessible that is completely ignorable.

> 3) We need to do atk_obejct_set_parent(), otherwise the hierarchy is broken
from bottom to top.

Actually, since nsHTMLAtkObjectAccessible extends from nsLeafAccessible, this is set in nsAccessibleWrap::getParentCB.  Pretty slick.  You can check this in Accerciser by poking around in the IPython console.

> 4) Is everything fine on creating and destroying the plugin?

I think I'll need someone else's help figuring out exactly what this entails.

> 5) Do we really need to change NPAPI interface? Can we get the socketWidget and do gtk_widget_get_accessible() ?

Hmm, I didn't think of this approach.  It sounds reasonable to me as long as plugins are required to use GtkSocket on Linux.  My gut tells me that this is not always the case, e.g.: Flash plugin.

> Also it would be awesome if we can have a simple gtk based plugin to
> demonstrate the atk hierarchy inside the plugin is seamlessly connected to
> Firefox.
> It would be even better if we can demo it with Orca.

Do you know if there's an existing plugin example I could modify to add this?  Alternately, I could implement a new one, but I'd much prefer a shortcut if I can get it :)

Comment 14

[Ginn Chen]

(In reply to comment #13)
> Great, thanks for breaking this down for me.
> 
> > 1) Do we need g_object_ref / g_object_unref for the AtkObject?
> 
> Actually, it looks like this is happening anyway -- when the parent has its
> children reffed, and in nsAccessibleWrap::Shutdown().

That is for mAtkObject of nsHTMLAtkObjectOwnerAccessible.
We should do it for mChildAccessible of nsHTMLAtkObjectOwnerAccessible, if we want to hold the AtkObject *.
> 
> > 2) Which atk role should be used for nsHTMLAtkObjectAccessible?
> 
> nsHTMLAtkObjectAccessible should provide its own role as returned from the
> plugin, but nsHTMLAtkObjectOwnerAccessible is probably fine as a
> ATK_OBJECT_UNKNOWN.  Personally, I'd prefer to make this an ATK_OBJECT_FILLER,
> but it seems that UNKNOWN is used frequently enough in Firefox that it's
> appropriate for an accessible that is completely ignorable.
> 

If it is supposed to be ignored, can we just make it as a wrapper of plugin accessible?
I mean we return plugin accessible for nsHTMLAtkObjectOwnerAccessible::GetNativeInterface instead of GetFirstChild.
Will there be a problem?

> > 3) We need to do atk_obejct_set_parent(), otherwise the hierarchy is broken
> from bottom to top.
> 
> Actually, since nsHTMLAtkObjectAccessible extends from nsLeafAccessible, this
> is set in nsAccessibleWrap::getParentCB.  Pretty slick.  You can check this in
> Accerciser by poking around in the IPython console.
> 

It is done in refChildCB.
I think we should do it explicitly on creating nsHTMLAtkObjectOwnerAccessible.

> > 4) Is everything fine on creating and destroying the plugin?
> 
> I think I'll need someone else's help figuring out exactly what this entails.
> 
> > 5) Do we really need to change NPAPI interface? Can we get the socketWidget and do gtk_widget_get_accessible() ?
> 
> Hmm, I didn't think of this approach.  It sounds reasonable to me as long as
> plugins are required to use GtkSocket on Linux.  My gut tells me that this is
> not always the case, e.g.: Flash plugin.

AFAIK, Flashplayer 9/10 uses GtkSocket.
For old Xt plugin, they won't use new NPAPI interface anyway.

> 
> > Also it would be awesome if we can have a simple gtk based plugin to
> > demonstrate the atk hierarchy inside the plugin is seamlessly connected to
> > Firefox.
> > It would be even better if we can demo it with Orca.
> 
> Do you know if there's an existing plugin example I could modify to add this? 
> Alternately, I could implement a new one, but I'd much prefer a shortcut if I
> can get it :)

What about http://multimedia.cx/diamondx/ ?
I think we can add some labels and textfields on the canvas.

Comment 15

[Brad Taylor]

> That is for mAtkObject of nsHTMLAtkObjectOwnerAccessible.
> We should do it for mChildAccessible of nsHTMLAtkObjectOwnerAccessible, if we
> want to hold the AtkObject *.

mAtkObject is in nsHTMLAtkObjectAccessible, not in the Owner.  Check out line 91 of accessible/src/atk/nsHTMLAtkObjectAccessible.cpp.

> If it is supposed to be ignored, can we just make it as a wrapper of plugin
> accessible?
> I mean we return plugin accessible for
> nsHTMLAtkObjectOwnerAccessible::GetNativeInterface instead of GetFirstChild.
> Will there be a problem?

The only problem I see is that it is different than what the MSAA implementation does.  There is an upside to symmetry.

> > > 3) We need to do atk_obejct_set_parent(), otherwise the hierarchy is broken
> > from bottom to top.
> > 
> > Actually, since nsHTMLAtkObjectAccessible extends from nsLeafAccessible, this
> > is set in nsAccessibleWrap::getParentCB.  Pretty slick.  You can check this in
> > Accerciser by poking around in the IPython console.
> > 
> 
> It is done in refChildCB.
> I think we should do it explicitly on creating nsHTMLAtkObjectOwnerAccessible.

It's also done in getParentCB.  A quick grep can confirm what I'm saying.

Do we necessarily have a parent when an nsHTMLAtkObjectOwnerAccessible is constructed?  Otherwise, I'm happy to make this change.

> > > 5) Do we really need to change NPAPI interface? Can we get the socketWidget and do gtk_widget_get_accessible() ?
> > 
> > Hmm, I didn't think of this approach.  It sounds reasonable to me as long as
> > plugins are required to use GtkSocket on Linux.  My gut tells me that this is
> > not always the case, e.g.: Flash plugin.
> 
> AFAIK, Flashplayer 9/10 uses GtkSocket.
> For old Xt plugin, they won't use new NPAPI interface anyway.

At least in Moonlight, it seems that in windowless mode, GtkPlug/GtkSocket aren't used.  In this respect, it seems the GetValue approach is the best.  According to folks in #accessibility, we can amortize this NPAPI break in with the break necessary for #78414.

> What about http://multimedia.cx/diamondx/ ?
> I think we can add some labels and textfields on the canvas.

Unfortunately it doesn't use a canvas, but I've hijacked the guts of the plugin and developed a sample that features a treeview for testing.  Check out http://gitorious.org/accessible-plugin-demo

Comment 16

[Ginn Chen]

(In reply to comment #15)
> > That is for mAtkObject of nsHTMLAtkObjectOwnerAccessible.
> > We should do it for mChildAccessible of nsHTMLAtkObjectOwnerAccessible, if we
> > want to hold the AtkObject *.
> 
> mAtkObject is in nsHTMLAtkObjectAccessible, not in the Owner.  Check out line
> 91 of accessible/src/atk/nsHTMLAtkObjectAccessible.cpp.
> 

OK, I misread this part.
I think we need g_object_ref() after "mAtkObject = aAtkObject;" in constructor of nsHTMLAtkObjectAccessible.

> > > > 3) We need to do atk_obejct_set_parent(), otherwise the hierarchy is broken
> > > from bottom to top.
> > > 
> > > Actually, since nsHTMLAtkObjectAccessible extends from nsLeafAccessible, this
> > > is set in nsAccessibleWrap::getParentCB.  Pretty slick.  You can check this in
> > > Accerciser by poking around in the IPython console.
> > > 
> > 
> > It is done in refChildCB.
> > I think we should do it explicitly on creating nsHTMLAtkObjectOwnerAccessible.
> 
> It's also done in getParentCB.  A quick grep can confirm what I'm saying.
> 
> Do we necessarily have a parent when an nsHTMLAtkObjectOwnerAccessible is
> constructed?  Otherwise, I'm happy to make this change.

Yes, we do.
If the AT client get an event from the AtkObject of the plugin and the AT client calls get parent for it,
it will get into the get parent callback in libgail not our getParentCB.
It might not work correctly.

> > > > 5) Do we really need to change NPAPI interface? Can we get the socketWidget and do gtk_widget_get_accessible() ?
> > > 
> > > Hmm, I didn't think of this approach.  It sounds reasonable to me as long as
> > > plugins are required to use GtkSocket on Linux.  My gut tells me that this is
> > > not always the case, e.g.: Flash plugin.
> > 
> > AFAIK, Flashplayer 9/10 uses GtkSocket.
> > For old Xt plugin, they won't use new NPAPI interface anyway.
> 
> At least in Moonlight, it seems that in windowless mode, GtkPlug/GtkSocket
> aren't used.  In this respect, it seems the GetValue approach is the best. 
> According to folks in #accessibility, we can amortize this NPAPI break in with
> the break necessary for #78414.

I agree.

Comment 17

[Brad Taylor]

Attachment: Proposed patch

Ok, I've made the changes we've discussed.  Let me know if there's anything else I can do to make this ready to go in.

Comment 18

[Brad Taylor]

Comment on attachment 378647 [details] [diff] [review]
Proposed patch

I think this is ready to be reviewed more formally now.

Thanks for your feedback!

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 378647 [details] [diff] [review]
Proposed patch

Brad please be sure to use the latest boilerplate for the new files.
http://www.mozilla.org/MPL/boilerplate-1.1/

>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.

Put Brad Taylor, or your affiliation instead of Netscape... :)


>+NS_IMETHODIMP nsHTMLAtkObjectOwnerAccessible::GetFirstChild(nsIAccessible **aFirstChild)
>+{
>+  *aFirstChild = mNativeAccessible;
>+  if (!mNativeAccessible) {
>+    if (!mChildAccessible) {

NIT: maybe add a comment about when we might not have an mChildAccessible.

Great work!

r=me

Note: if the other reviews pass, we need to get plugin module approval from Josh or peer (see http://www.mozilla.org/owners.html)

Comment 20

[Marco Zehe (:MarcoZ)]

Comment on attachment 378647 [details] [diff] [review]
Proposed patch

One nit I found:
>+NS_IMETHODIMP nsHTMLAtkObjectOwnerAccessible::GetChildCount(PRInt32 *aChildCount)

This line, along with some of the other method definition headers, is longer than 80 chars. Please wrap the line like this:

>+NS_IMETHODIMP
>nsHTMLAtkObjectOwnerAccessible::GetChildCount(PRInt32 *aChildCount)

and format other long lines with more parameters accordingly so that the parameters are aligned with the first char after the opening parenthesis.

r=me with this addressed.

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 378647 [details] [diff] [review]
Proposed patch

Concerning style only. I didn't look at logic and approach yet.

>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.

nit: I would believe you work here

>+ * Portions created by the Initial Developer are Copyright (C) 1998

but I won't believe you live in 1998 ;)

the next new file has the same thing.

>+
>+nsHTMLAtkObjectOwnerAccessible::nsHTMLAtkObjectOwnerAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell, AtkObject* aChildAccessible):

nit: follow 80 char restriction in your patch

>+nsAccessibleWrap(aNode, aShell)

nit: indent in two spaces

>+{
>+  mChildAccessible = aChildAccessible;
>+}
>+
>+nsresult
>+nsHTMLAtkObjectOwnerAccessible::Shutdown()
>+{
>+  nsAccessibleWrap::Shutdown();
>+  mChildAccessible = nsnull;
>+  return NS_OK;
>+}
>+
>+/** 
>+  * Our only child is a nsHTMLAtkObjectAccessible 
>+  */

nit: I would prefer to keep comment inside of method

>+NS_IMETHODIMP nsHTMLAtkObjectOwnerAccessible::GetFirstChild(nsIAccessible **aFirstChild)

nit: NS_IMETHODIMP should be on own line, please fix this below also.

>+{
>+  *aFirstChild = mNativeAccessible;

nit: Use NS_ENSURE_ARG_POINTER(aFirstChild)

>+  if (!mNativeAccessible) {
>+    if (!mChildAccessible) {
>+      return NS_OK;
>+    }

nit: personally I don't use braces around single "if" statement, but up to you since a11y moule keeps both styles.

>+
>+#include "nsBaseWidgetAccessible.h"
>+
>+struct IAccessible;

I think it's copy-paste from MSAA where IAccessible is presented, please check comments below on IAccessible as well.

>+
>+  virtual nsresult Shutdown();

nit: short comment where this method comes from (like you did above)

>+#elif MOZ_ACCESSIBILITY_ATK
>+    AtkObject *nativeAccessible;

initialize it please

>+
>+    pluginInstance->GetValue(nsPluginInstanceVariable_NativeAccessible, &nativeAccessible);
>+    if (nativeAccessible) {
>+      *aAccessible = new nsHTMLAtkObjectOwnerAccessible(node, weakShell, nativeAccessible);

I know this is current style of the method but you should return NS_ERROR_OUT_OF_MEMORY if *aAccessible is null.

>-  NPNVprivateModeBool = 18
>+  NPNVprivateModeBool = 18,
>+
>+  NPNVNativeAccessible = 21

any comments are appreciate and you should have additional review from plugin peer, you could request it when we will finish :)

> #ifdef XP_MACOSX
>     , nsPluginInstanceVariable_DrawingModel          = 20
> #endif
>+    , nsPluginInstanceVariable_NativeAccessible      = 21

comment here please as well.

I wait for new patch. Thank you for working on this.

Comment 22

[Brad Taylor]

Attachment: Revised Patch

Thanks to everyone for your feedback.  This should address all the points mentioned thus far.

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 378876 [details] [diff] [review]
Revised Patch

again style nits, because it's hard to follow logic and keep in mind style issues the same time :)

>+
>+nsHTMLAtkObjectOwnerAccessible::nsHTMLAtkObjectOwnerAccessible(
>+  nsIDOMNode* aNode, nsIWeakReference* aShell, AtkObject* aChildAccessible):
>+  nsAccessibleWrap(aNode, aShell)

it would be better:

nsHTMLAtkObjectOwnerAccessible::
  nsHTMLAtkObjectOwnerAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell,
                                                                 AtkObject* aChildAccessible):

btw, put space before semicolon.

>+nsresult
>+nsHTMLAtkObjectOwnerAccessible::Shutdown()

list methods in the order of header file

>+NS_IMETHODIMP
>+nsHTMLAtkObjectOwnerAccessible::GetFirstChild(nsIAccessible **aFirstChild)
>+{
>+  // Our only child is a nsHTMLAtkObjectAccessible  

put this comment right before *aFirstChild initialization and don't forget '.' in the end of sentence.

>+  NS_ENSURE_ARG_POINTER(mNativeAccessible);

new line please

you should check argument actually on nsnull, not mNativeAccessible :)

>+  *aFirstChild = mNativeAccessible;

new line, in general don't be stingy on new lines if it makes code more readable.

>+  if (!mNativeAccessible) {
>+    mNativeAccessible = new nsHTMLAtkObjectAccessible(mChildAccessible);

check on null, return NS_ERROR_OUT_MEMORY

>+    SetFirstChild(mNativeAccessible);
>+    *aFirstChild = mNativeAccessible;

I don't like two *aFirstChild assignment in this method

>+NS_IMETHODIMP
>+nsHTMLAtkObjectOwnerAccessible::GetLastChild(nsIAccessible **aLastChild)

check argument on null like above

>+NS_IMETHODIMP
>+nsHTMLAtkObjectOwnerAccessible::GetChildCount(PRInt32 *aChildCount)
>+{

check argument here as well

>+  nsCOMPtr<nsIAccessible> onlyChild;
>+  GetFirstChild(getter_AddRefs(onlyChild));

in general taken approach is to cache children inside of CacheChildren method that is called from GetChildCount and GetFirstChild() returns cached first child. Is there any object you violate this rule?

>+nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):

nit: space before semicolon

>+
>+#include "nsBaseWidgetAccessible.h"
>+
>+class nsHTMLAtkObjectOwnerAccessible : public nsAccessibleWrap
>+{
>+public:
>+  // This will own the nsHTMLAtkObjectAccessible. We create this where the
>+  // <object> or <embed> exists in the tree, so that get_accNextSibling() etc.
>+  // will still point to Gecko accessible sibling content. This is necessary
>+  // because the native plugin accessible doesn't know where it exists in the
>+  // Mozilla tree, and returns null for previous and next sibling. This would
>+  // have the effect of cutting off all content after the plugin.

I prefer to have this comment before class definition in java comments style.

As well I would like to reorganize this comment to make it more readable and please remove ALL MSAA specific things. Like (please correct my english and logic if something is wrong):

/**
  * The accessible is used for HTML object or embed elements and contains nsHTMLAtkObjectAccessible object as the unique child which is a placeholder of native plugin accessible. This accessible serves to provide correct ATK navigation between Gecko accessibles and native plugin accessible. Really native plugin accessible doesn't know where it exists in the Gecko accessible tree so it can't return correct siblings for HTML object or embed elements.
  */

something like this. Feel free to reformulate or switch David for this - he's adapted very well for things like this :)

>+  nsHTMLAtkObjectOwnerAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell, AtkObject *accessible);

doesnt' fit to 80 chars restriction, accessible -> aAccessible or aATKAccessible.

>+  NS_IMETHOD GetChildCount(PRInt32 *aChildCount);  // Zero or one child

you could put this comment inside of method implementation.

>+  AtkObject * mChildAccessible;

no space before * I think (or after), check with our code.

>+  nsCOMPtr<nsIAccessible> mNativeAccessible;

do you need mNativeAccessible because nsAccessible has mFirstChild member?

>+/**
>+  * This class is used only internally, we never! send out an IAccessible linked
>+  *   back to this object. This class is used to represent a plugin object when
>+  *   referenced as a child or sibling of another nsAccessible node. We need only
>+  *   a limited portion of the nsIAccessible interface implemented here. The
>+  *   in depth accessible information will be returned by the actual IAccessible
>+  *   object returned by us in Accessible::NewAccessible() that gets the IAccessible
>+  *   from the windows system from the window handle.
>+  */

again get rid MSAA specific things. There is no IAccessible in ATK word. I don't know what is Accessible object. Possibly it's worth to reword this comment as well.

>+class nsHTMLAtkObjectAccessible : public nsLeafAccessible
>+{
>+public:
>+

no new line I guess.

>+      *aAccessible = new nsHTMLAtkObjectOwnerAccessible(node, weakShell, nativeAccessible);
>+      if (*aAccessible) {
>+        NS_ADDREF(*aAccessible);
>+        return NS_OK;
>+      }
>+      return NS_ERROR_OUT_OF_MEMORY;

it's better

NS_ENSURE_TRUE(*aAccessible, NS_ERROR_OUT_OF_MEMORY);

NS_ADDREF(*aAccessible);
return NS_OK;

>+
>+  /* Get a native accessible implementation for the plugin on the given system.

nit: "implementation" word looks unneeded.

>+   * On Linux, this is expected to be an AtkObject.

nit: for example? or currently used on linux only?

>+   * Introduced in Mozilla 1.9.2.
>+   */
>+  NPNVNativeAccessible = 21
>+     */
>+    , nsPluginInstanceVariable_NativeAccessible      = 21

it's the question to plugin peer actually but why do you need two constants if the first constant is used only. Is the second one is supposed to be used on plugin side?

waiting for new patch, thank you for patience :)

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Alex, good to catch these comment issues, I had also assumed the msaa comments could be reused, but you are right, they don't fit perfectly.

That said, I'm not sure about this one:

(In reply to comment #23)
> (From update of attachment 378876 [details] [diff] [review])
> >+class nsHTMLAtkObjectOwnerAccessible : public nsAccessibleWrap
> >+{
> >+public:
> >+  // This will own the nsHTMLAtkObjectAccessible. We create this where the
> >+  // <object> or <embed> exists in the tree, so that get_accNextSibling() etc.
> >+  // will still point to Gecko accessible sibling content. This is necessary
> >+  // because the native plugin accessible doesn't know where it exists in the
> >+  // Mozilla tree, and returns null for previous and next sibling. This would
> >+  // have the effect of cutting off all content after the plugin.
> 
> I prefer to have this comment before class definition in java comments style.
> 
> As well I would like to reorganize this comment to make it more readable and
> please remove ALL MSAA specific things. Like (please correct my english and
> logic if something is wrong):
> 
> /**
>   * The accessible is used for HTML object or embed elements and contains
> nsHTMLAtkObjectAccessible object as the unique child which is a placeholder of
> native plugin accessible. This accessible serves to provide correct ATK
> navigation between Gecko accessibles and native plugin accessible. Really
> native plugin accessible doesn't know where it exists in the Gecko accessible
> tree so it can't return correct siblings for HTML object or embed elements.
>   */
> 
> something like this. Feel free to reformulate or switch David for this - he's
> adapted very well for things like this :)

How about:

This is a smart placeholder for the nsHTMLAtkObjectAccessible which "knows" where it lives in the accessible tree heirarchy. This is necessary because the native plugin accessible doesn't know where it exists in the Mozilla tree and would otherwise fail for calls like getParent(), which could break tree walking.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> How about:
> 
> This is a smart placeholder for the nsHTMLAtkObjectAccessible which "knows"
> where it lives in the accessible tree heirarchy. This is necessary because the
> native plugin accessible doesn't know where it exists in the Mozilla tree and
> would otherwise fail for calls like getParent(), which could break tree
> walking.

In this case it makes sense to define and comment nsHTMLAtkObjectAccessible before this class. Otherwise it's not clear what is nsHTMLAtkObjectAccessible. Is it ok?

Comment 26

[Brad Taylor]

Attachment: Revised Patch (#2)

I've made some of the suggested changes, and have added some comments on your feedback below.

> >+NS_IMETHODIMP
> >+nsHTMLAtkObjectOwnerAccessible::GetLastChild(nsIAccessible **aLastChild)
> 
> check argument on null like above

This is already checked and properly handled in GetFirstChild, so I see no need.

> >+  nsCOMPtr<nsIAccessible> onlyChild;
> >+  GetFirstChild(getter_AddRefs(onlyChild));
> 
> in general taken approach is to cache children inside of CacheChildren method
> that is called from GetChildCount and GetFirstChild() returns cached first
> child. Is there any object you violate this rule?

Something should be explained a bit more clearly -- I've mostly derived this patch from nsHTMLWin32ObjectAccessible, so my use of various Firefox API should come with the caveat that I probably don't fully understand what all of this does, but a) I know that it works, and b) it's done elsewhere, so it can't be that wrong.  That said, I welcome any suggestions that you have on how to do it better.

Comment 27

[alexander :surkov (:asurkov)]

Brad, just try please to implement CacheChildren where you should initialize mAccChildCount and mFirstChild, remove implementation of GetFirstChild and GetChildCount,

Comment 28

[Ginn Chen]

Comment on attachment 379147 [details] [diff] [review]
Revised Patch (#2)

+nsresult
+nsHTMLAtkObjectAccessible::Shutdown()
+{
+  nsLeafAccessible::Shutdown();
+  g_object_unref(mAtkObject);
+  return NS_OK;
+}

nsAccessibleWrap::Shutdown will call ShutdownAtkObject(), mAtkObject is unreffed there.

+nsHTMLAtkObjectOwnerAccessible::nsHTMLAtkObjectOwnerAccessible(
+  nsIDOMNode* aNode, nsIWeakReference* aShell, AtkObject* aChildAccessible):
+  nsAccessibleWrap(aNode, aShell)
+{
+  mChildAccessible = aChildAccessible;
+}
+

I think we don't need mChildAccessible.
We can just do mNativeAccessible = new nsHTMLAtkObjectAccessible(aChildAccessible); SetFirstChild(mNativeAccessible);

+NS_IMETHODIMP
+nsHTMLAtkObjectOwnerAccessible::GetFirstChild(nsIAccessible **aFirstChild)
+{
+  // Our only child is a nsHTMLAtkObjectAccessible  
+  mNativeAccessible = new nsHTMLAtkObjectAccessible(mChildAccessible);
+  SetFirstChild(mNativeAccessible);

I think we need "if (!mNativeAccessible)" here unless we move these 2 lines to constructor, as I just suggested.


Also comment #27 sounds good.

Comment 29

[Ginn Chen]

And the atk_obejct_set_parent() issues is not addressed yet.

Comment 30

[Brad Taylor]

Attachment: Revised Patch (#3)

Ok, I've updated the patch with various requested fixes.

> And the atk_obejct_set_parent() issues is not addressed yet.

Ginn, which issues were these?  The last thing you requested was for atk_object_set_parent to happen in the nsHTMLAtkObjectAccessible ctor, and I've made this modification a few revisions ago.

+nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):
+  nsLeafAccessible(nsnull, nsnull)
+{
<snip />
+  AtkObject *parent = nsAccessibleWrap::GetAtkObject(accParent);
+  if (parent)
+    atk_object_set_parent(mAtkObject, parent);
+}

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 379682 [details] [diff] [review]
Revised Patch (#3)


>+nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):
>+  nsLeafAccessible(nsnull, nsnull)
>+{
>+  mAtkObject = aAtkObject;
>+  g_object_ref(mAtkObject);
>+
>+  nsCOMPtr<nsIAccessible> accParent;
>+  nsresult rv = GetParent(getter_AddRefs(accParent));

if it works then it's sort of dangerous. If you haven't cached parent then this method will fail since this accessible is defunct (has no DOMNode) from point of view Gecko a11y. At least it must be commented very carefully.

And I think it should fail because generally we set parent in CacheChildren which you override.

>+nsHTMLAtkObjectOwnerAccessible::nsHTMLAtkObjectOwnerAccessible(
>+  nsIDOMNode* aNode, nsIWeakReference* aShell, AtkObject* aChildAccessible):
>+  nsAccessibleWrap(aNode, aShell)
>+{
>+  mNativeAccessible = new nsHTMLAtkObjectAccessible(aChildAccessible);

possibly it's worth to create it in CacheChildren.

>+}
>+
>+void
>+nsHTMLAtkObjectOwnerAccessible::CacheChildren()
>+{
>+  if (!mWeakShell) {
>+    // This node has been shut down
>+    mAccChildCount = eChildCountUninitialized;
>+    return;
>+  }
>+
>+  if (mAccChildCount == eChildCountUninitialized) {

nit: I would prefer return here early if mAccChildCount != eChildCountUninitialized

>+    if (mNativeAccessible)
>+      SetFirstChild(mNativeAccessible);
>+  
>+    mAccChildCount = mFirstChild ? 1 : 0;

I think you really don't need mNativeAccessible accessible, use mFirstChild instead. I think you can do this if you move | new nsHTMLAtkObjectAccessible| here.

>+public:
>+
>+  nsHTMLAtkObjectAccessible(AtkObject* aNativeAccessible);

be consistent, c++ file uses name "aATKAccessible"

>+class nsHTMLAtkObjectOwnerAccessible : public nsAccessibleWrap
>+{
>+public:
>+  nsHTMLAtkObjectOwnerAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell, AtkObject *accessible);
>+  virtual ~nsHTMLAtkObjectOwnerAccessible() {}


nit: be consistent, c++ file uses name "aChildAccessible"

>+  // nsIAccessible
>+  void CacheChildren();

it's not nsIAccessible, it's nsAccessible I think.

waiting for new patch, cancelling review.

Comment 32

[Ginn Chen]

(In reply to comment #30)
> 
> Ginn, which issues were these?  The last thing you requested was for
> atk_object_set_parent to happen in the nsHTMLAtkObjectAccessible ctor, and I've
> made this modification a few revisions ago.

Sorry, I must missed it.

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

This is great to bang out all the kinks here. My review was based on some assumptions, one of which was the windows version of this being correct. Should we file a separate bug to clean up that version? (At the very least, clean up comments etc). Ginn, Alex, Maroc, *if* you agree, please file.

Comment 34

[Brad Taylor]

Attachment: Revised Patch (#4)

Alex, on your first point in comment #31, I've already gone back and forth with Ginn on this, and he insisted that we have a parent when the accessible is created.  I've attached the conversation that I had with Ginn below.

> > > > Actually, since nsHTMLAtkObjectAccessible extends from nsLeafAccessible, this
> > > > is set in nsAccessibleWrap::getParentCB.  Pretty slick.  You can check this in
> > > > Accerciser by poking around in the IPython console.
> > > 
> > > It is done in refChildCB.
> > > I think we should do it explicitly on creating nsHTMLAtkObjectOwnerAccessible.
> > 
> > It's also done in getParentCB.  A quick grep can confirm what I'm saying.
> > 
> > Do we necessarily have a parent when an nsHTMLAtkObjectOwnerAccessible is
> > constructed?  Otherwise, I'm happy to make this change.
> 
> Yes, we do.
> If the AT client get an event from the AtkObject of the plugin and the AT
> client calls get parent for it,
> it will get into the get parent callback in libgail not our getParentCB.
> It might not work correctly.

If I don't set the parent there, where do you suggest I put it?  Since this wrapper is not aware of its children, CacheChildren doesn't make any semantic sense to me.  Maybe you and Ginn can speak to each other about this off of the bug and come to a decision.  I'll be happy to do whatever you decide, but I'd like to avoid needless patch shuffle because you can't come to consensus.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> This is great to bang out all the kinks here. My review was based on some
> assumptions, one of which was the windows version of this being correct. Should
> we file a separate bug to clean up that version? (At the very least, clean up
> comments etc). Ginn, Alex, Maroc, *if* you agree, please file.

done, bug 495063

Comment 36

[alexander :surkov (:asurkov)]

Brad, I'm not sure we talk about the same thing. I mean Gecko accessible parent, if I get right Ginn concerns ATK accessible parent. I think Ginn is right and you should set ATK parent. But problem I refer to is Gecko accessible parent might be not initialized. Do I miss something? Possibly you should set Gecko accessible parent in CacheChildren like we do in http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#770

Comment 37

[alexander :surkov (:asurkov)]

Btw, Brad or Ginn could you describe me how ATK navigation methods works? 1) walking down by children (from nsHTMLAtkObjectOwnerAccessible to native ATK plugin accessibles) 2) walking up by parents (from accessible of native plugin subtree to nsHTMLAtkObjectOwnerAccessible)?

Comment 38

[Brad Taylor]

(In reply to comment #36)
> Brad, I'm not sure we talk about the same thing. I mean Gecko accessible
> parent, if I get right Ginn concerns ATK accessible parent. I think Ginn is
> right and you should set ATK parent. But problem I refer to is Gecko accessible
> parent might be not initialized. Do I miss something? Possibly you should set
> Gecko accessible parent in CacheChildren like we do in
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#770

If we do that, we can't set the ATK parent in the ctor, and that's why I mentioned Ginn's comment.  How do we satisfy both concerns?


(In reply to comment #37)
> Btw, Brad or Ginn could you describe me how ATK navigation methods works? 1)
> walking down by children (from nsHTMLAtkObjectOwnerAccessible to native ATK
> plugin accessibles) 2) walking up by parents (from accessible of native plugin
> subtree to nsHTMLAtkObjectOwnerAccessible)?

In Atk, to get an Accessible's children, you call atk_object_n_accessible_children(acc).  From there, to get an individual child, you call atk_object_ref_accessible(acc, index).

To get the parent of an object, you call atk_object_get_parent(acc).  This is really the only sticky case with this patch, and we fix it by calling atk_object_set_parent(acc, parent) in the ctor (and also in two other places in nsAccessibleWrap: getParentCB and refChildCB).

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #38)

> If we do that, we can't set the ATK parent in the ctor, and that's why I
> mentioned Ginn's comment.  How do we satisfy both concerns?

I'm not sure I really understand ATK part but I think if you'll set Gecko accessible parent then you don't need to set up manually ATK parent accessible via atk_object_set_parent. So could you try to call nsPIAccessible::SetParent() on nsHTMLAtkObjectAccessible object inside of nsHTMLAtkObjectOwnerAccessible::CacheChildren(). I think this must work.

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #38)
> In Atk, to get an Accessible's children, you call
> atk_object_n_accessible_children(acc).  From there, to get an individual child,
> you call atk_object_ref_accessible(acc, index).
> 
> To get the parent of an object, you call atk_object_get_parent(acc).  This is
> really the only sticky case with this patch, and we fix it by calling
> atk_object_set_parent(acc, parent) in the ctor (and also in two other places in
> nsAccessibleWrap: getParentCB and refChildCB).

Brad, could you clarify please how our getParentCB/refChildCB are related with atk_object_get_parent/atk_object_ref_accessible?

Comment 41

[Ginn Chen]

Sorry for the late response. We had a long weekend in China.

Alex,
getParentCB/refChildCB is callback function of objects of MaiAtkObject classes, which inherit AtkObject.
But the atk object of plugin is not a MaiAtkObject, therefore atk_object_get_parent won't get to getParentCB. So we must do atk_object_set_parent, then the default implementation of atk_object_get_parent will return the correct parent.

Yes, we'd better also set Gecko accessible parent.
But nsHTMLWin32ObjectAccessible didn't do it, either.


Brad,
I agree with Alex.
+  nsCOMPtr<nsIAccessible> accParent;
+  nsresult rv = GetParent(getter_AddRefs(accParent));
It won't work.
I think you can either pass the parent pointer to the ctor, or do it in nsHTMLAtkObjectOwnerAccessible::CacheChildren() instead.

Comment 42

[alexander :surkov (:asurkov)]

Ginn, thank you for clarification.

Btw, I think I prefer to set Gecko parent in CacheChildren. It worth to have it I think because if we are able to go down to atk object stub from gecko tree then we should be able to go up from it to gecko tree.

Comment 43

[Brad Taylor]

You'll have to excuse my lack of knowledge of the FF Accessibility codebase, but exactly what do I have to do to "set Gecko parent in CacheChildren"?

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43)
> You'll have to excuse my lack of knowledge of the FF Accessibility codebase,
> but exactly what do I have to do to "set Gecko parent in CacheChildren"?

Brad, just adhere to the code http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#744. Call setParent for new nsHTMLAtkObjectAccessible and pass nsHTMLAtkOwnerObjectAccessible object. Also I think you can atk_object_set_parent there. But please check with Ginn where the best place for this.

Comment 45

[Ginn Chen]

I think nsHTMLAtkObjectOwnerAccessible::CacheChildren() is a good place.

Comment 46

[Brad Taylor]

Attachment: Revised Patch (#5)

Hey Alex,

> Brad, just adhere to the code
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#744.
> Call setParent for new nsHTMLAtkObjectAccessible and pass
> nsHTMLAtkOwnerObjectAccessible object. Also I think you can
> atk_object_set_parent there. But please check with Ginn where the best place
> for this.

I started looking at this code today, and the thing is that nsHTMLAtkObjectOwnerAccessible is the only one that actually wraps a DOM node.  After that, all the objects are fake, so walking the nsHTMLAtkObjectOwner's children will return null.  That said, I can still set the parent without using the nsAccessibleTreeWalker.

Attached is an attempt at this.  Let me know what you think.

Comment 47

[alexander :surkov (:asurkov)]

Comment on attachment 382722 [details] [diff] [review]
Revised Patch (#5)


>+nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):
>+  nsLeafAccessible(nsnull, nsnull)
>+{
>+  mAtkObject = aAtkObject;
>+  g_object_ref(mAtkObject);

>+void
>+nsHTMLAtkObjectOwnerAccessible::CacheChildren()
>+{
>+  nsHTMLAtkObjectAccessible* childAccessible
>+    = new nsHTMLAtkObjectAccessible(mChildAccessible);
>+
>+  childAccessible->SetParent(this);
>+  atk_object_set_parent(mChildAccessible, GetAtkObject());

sorry if we discussed this already but it sounds refChildCB called on nsHTMLAtkObjectOwnerAccessible should call g_object_ref and atk_object_set_parent? Do I miss something?

>+  nsHTMLAtkObjectAccessible* childAccessible
>+    = new nsHTMLAtkObjectAccessible(mChildAccessible);

nit '=' on new line please

nit: please use

//////////////////// 80 chars
// nsHTMLAtkObjectAccessible

to separate the code of nsHTMLAtkObjectAccessible and nsHTMLAtkObjectOwnerAccessible

Comment 48

[Brad Taylor]

(In reply to comment #47)
> (From update of attachment 382722 [details] [diff] [review])
> 
> >+nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):
> >+  nsLeafAccessible(nsnull, nsnull)
> >+{
> >+  mAtkObject = aAtkObject;
> >+  g_object_ref(mAtkObject);
> 
> >+void
> >+nsHTMLAtkObjectOwnerAccessible::CacheChildren()
> >+{
> >+  nsHTMLAtkObjectAccessible* childAccessible
> >+    = new nsHTMLAtkObjectAccessible(mChildAccessible);
> >+
> >+  childAccessible->SetParent(this);
> >+  atk_object_set_parent(mChildAccessible, GetAtkObject());
> 
> sorry if we discussed this already but it sounds refChildCB called on
> nsHTMLAtkObjectOwnerAccessible should call g_object_ref and
> atk_object_set_parent? Do I miss something?

Please read comment #12 though #16.

> 
> >+  nsHTMLAtkObjectAccessible* childAccessible
> >+    = new nsHTMLAtkObjectAccessible(mChildAccessible);
> 
> nit '=' on new line please
> 
> nit: please use
> 
> //////////////////// 80 chars
> // nsHTMLAtkObjectAccessible
> 
> to separate the code of nsHTMLAtkObjectAccessible and
> nsHTMLAtkObjectOwnerAccessible

I'll fix these when I get into the office in a few hours.

Comment 49

[alexander :surkov (:asurkov)]

(In reply to comment #48)

> > sorry if we discussed this already but it sounds refChildCB called on
> > nsHTMLAtkObjectOwnerAccessible should call g_object_ref and
> > atk_object_set_parent? Do I miss something?
> 
> Please read comment #12 though #16.

Ok, I see. If I get right the problem is we don't set up ATK parent if we walk from down to top. I hope it's not a problem to set up ATK parent twice (firstly in constructor, secondly in refChildCB).

Another thing bothers me is we don't addref mChildAccessible. If plugin accessible is destroyed somehow then we won't be notified and will crash. 

I redirect these questions to Ginn since ATK part is not my best part.

otherwise is good for me.

Comment 50

[alexander :surkov (:asurkov)]

Comment on attachment 382722 [details] [diff] [review]
Revised Patch (#5)

r=me with comment #49 addressed.

Comment 51

[Ginn Chen]

Comment on attachment 382722 [details] [diff] [review]
Revised Patch (#5)

I’m sorry I didn't point it out earlier.

You need to call CacheChildren() in constructor of nsHTMLAtkObjectOwnerAccessible.
Otherwise, it is too late for g_object_ref().

Comment 52

[alexander :surkov (:asurkov)]

(In reply to comment #51)
> (From update of attachment 382722 [details] [diff] [review])
> I’m sorry I didn't point it out earlier.
> 
> You need to call CacheChildren() in constructor of
> nsHTMLAtkObjectOwnerAccessible.
> Otherwise, it is too late for g_object_ref().

Ginn, I think this is answer on my 2nd question in the comment #49. It's unusual to call CacheChildren in constructor for us. Could you think of another way?

Comment 53

[Ginn Chen]

The other way is to move following code into constructor.

+  if (!mChildAccessible) {
+    mAccChildCount = 0;
+    return;
+  }
+  nsHTMLAtkObjectAccessible* childAccessible
+    = new nsHTMLAtkObjectAccessible(mChildAccessible);
+
+  childAccessible->SetParent(this);
+  atk_object_set_parent(mChildAccessible, GetAtkObject());
+
+  SetFirstChild(childAccessible);
+  mAccChildCount = 1;

The child of nsHTMLAtkObjectOwnerAccessible is decided and could not be changed after construction.
I think it's reasonable to set first child and child count there.

Comment 54

[alexander :surkov (:asurkov)]

We have preference "accessibility.disablecache" which disables accessible cache. When this option is used then we invalidate children on every GetFirstChild call. But we can't override InvalidateChildren on void implementation because it is used to clean cached values when accessible is destroyed. I think It's safely to make nsHTMLAtkObjectOwnerAccessible behaviour to follow our common rules. Any thoughts, Ginn?

Comment 55

[Ginn Chen]

Good catch. I forgot that option.

I think we can change it a little bit.
Use nsCOMPtr<nsIAccessible> mNativeAccessible instead of AtkObject* mChildAccessible.

In nsHTMLAtkObjectOwnerAccessible::nsHTMLAtkObjectOwnerAccessible(), we do
{
  mNativeAccessible
    = new nsHTMLAtkObjectAccessible(aChildAccessible);
  mNativeAccessible->SetParent(this);
  atk_object_set_parent(aChildAccessible, GetAtkObject());
}

In CacheChildren(), we do
  SetFirstChild(mNativeAccessible);
  mAccChildCount = 1;

It's something like patch #3 (Brad, I'm sorry to make it back and forth.)
I suggested to use mFirstChild instead of mNativeAccessible, but we'd better not, because InvalidateChildren would set mFirstChild to nsnull.

Is there any problem?

Comment 56

[alexander :surkov (:asurkov)]

(In reply to comment #55)

> Is there any problem?

SetParent() should be in CacheChildren also. Though I would guess we can bring a hack and have atk_object_set_parent in constructor.

Comment 57

[Brad Taylor]

Attachment: Revised Patch (#6)

Ginn -- I had to use QueryAccessible to get a nsAccessible so that I can call SetParent as SetParent doesn't exist on nsIAccessible.  I hope this is alright.

Comment 58

[alexander :surkov (:asurkov)]

Comment on attachment 383760 [details] [diff] [review]
Revised Patch (#6)


>+nsHTMLAtkObjectOwnerAccessible::nsHTMLAtkObjectOwnerAccessible(
>+  nsIDOMNode* aNode, nsIWeakReference* aShell, AtkObject* aChildAccessible):
>+  nsAccessibleWrap(aNode, aShell)
>+{
>+  nsHTMLAtkObjectAccessible* nativeAccessible
>+    = new nsHTMLAtkObjectAccessible(aChildAccessible);

use mNativeAccessible directly

>+  nativeAccessible->SetParent(this);

it doesn't make sense to set gecko parent here I think

>+  atk_object_set_parent(aChildAccessible, GetAtkObject());

if Ginn is happy with having this call in constructor then this line must documented very well.

>+
>+  mNativeAccessible = nativeAccessible;

I think you should use cycle collector here (see NS_IMPL_CYCLE_COLLECTION_CLASS example in nsAccessible)

>+
>+  nsRefPtr<nsAccessible> nativeAccessible
>+    = nsAccUtils::QueryAccessible(mNativeAccessible);

nit: '=' on line above

I think it's ok to use nsAccessible here especially when bug 461921 will be fixed.

Comment 59

[Ginn Chen]

(In reply to comment #58)
> >+  nativeAccessible->SetParent(this);
> 
> it doesn't make sense to set gecko parent here I think

Since we can't get parent by node, we need to make sure we set Gecko parent for nsHTMLAtkObjectAccessible when we create it. Right?

> >+  atk_object_set_parent(aChildAccessible, GetAtkObject());
> 
> if Ginn is happy with having this call in constructor then this line must
> documented very well.
> 

Yes, it should be in constructor.

Comment 60

[Brad Taylor]

(In reply to comment #58)
> (From update of attachment 383760 [details] [diff] [review])
> 
> >+nsHTMLAtkObjectOwnerAccessible::nsHTMLAtkObjectOwnerAccessible(
> >+  nsIDOMNode* aNode, nsIWeakReference* aShell, AtkObject* aChildAccessible):
> >+  nsAccessibleWrap(aNode, aShell)
> >+{
> >+  nsHTMLAtkObjectAccessible* nativeAccessible
> >+    = new nsHTMLAtkObjectAccessible(aChildAccessible);
> 
> use mNativeAccessible directly

I can't as I explained earlier -- SetParent is not available in nsIAccessible.  My choices were to either create a local nsHTMLAtkObjectAccessible or call nsAccUtils::QueryAccessible, and the later seemed like a waste.

> >+
> >+  mNativeAccessible = nativeAccessible;
> 
> I think you should use cycle collector here (see NS_IMPL_CYCLE_COLLECTION_CLASS
> example in nsAccessible)

There appears to be very little documentation about how to use these macros, so I'm going to do my best here.

I'll attach the patch in a bit.

Comment 61

[alexander :surkov (:asurkov)]

(In reply to comment #59)
> (In reply to comment #58)
> > >+  nativeAccessible->SetParent(this);
> > 
> > it doesn't make sense to set gecko parent here I think
> 
> Since we can't get parent by node, we need to make sure we set Gecko parent for
> nsHTMLAtkObjectAccessible when we create it. Right?

Possibly but I can't think of the case when we need it.

(In reply to comment #60)
> 
> I can't as I explained earlier -- SetParent is not available in nsIAccessible. 
> My choices were to either create a local nsHTMLAtkObjectAccessible or call
> nsAccUtils::QueryAccessible, and the later seemed like a waste.

nsAccUtils::QueryAccessible is a right way.

> 
> > >+
> > >+  mNativeAccessible = nativeAccessible;
> > 
> > I think you should use cycle collector here (see NS_IMPL_CYCLE_COLLECTION_CLASS
> > example in nsAccessible)
> 
> There appears to be very little documentation about how to use these macros, so
> I'm going to do my best here.

neither of us :) when I introduced them for a11y then I did my best as well. Ask review from smaug additionally to check this stuff.

> I'll attach the patch in a bit.

thank you.

Comment 62

[Brad Taylor]

Attachment: Revised Patch (#7)

Comment 63

[Ginn Chen]

Comment on attachment 383918 [details] [diff] [review]
Revised Patch (#7)

I'm not sure about the CYCLE_COLLECTION part.
Others are fine.

Comment 64

[alexander :surkov (:asurkov)]

Comment on attachment 383918 [details] [diff] [review]
Revised Patch (#7)

looks ok with me. Let's check it with Olli additionally.

Comment 65

[alexander :surkov (:asurkov)]

Comment on attachment 383918 [details] [diff] [review]
Revised Patch (#7)

requesting review from Josh for plugin part.

Comment 66

[timeless]

Comment on attachment 383918 [details] [diff] [review]
Revised Patch (#7)

>+nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):
>+  nsLeafAccessible(nsnull, nsnull)
why isn't this an initializer:
>+  mAtkObject = aAtkObject;

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
extra blank line:
>+
>+
>+nsHTMLAtkObjectOwnerAccessible::nsHTMLAtkObjectOwnerAccessible(
>+  nsIDOMNode* aNode, nsIWeakReference* aShell, AtkObject* aChildAccessible):
>+  nsAccessibleWrap(aNode, aShell)
>+{
we still require oom handling today
>+  mNativeAccessible =
>+    new nsHTMLAtkObjectAccessible(aChildAccessible);
please handle:
if (!mNativeAccessible) { /* somehow */ }
>+
>+  nsRefPtr<nsAccessible> nativeAccessible =
>+    nsAccUtils::QueryAccessible(mNativeAccessible);
before you crash:
>+  nativeAccessible->SetParent(this);

is this a good name:
>+nsHTMLAtkObjectOwnerAccessible::CacheChildren()
it seems more like "update [child] cache" given the following:
>+  if (!mWeakShell) {
>+    // This node has been shut down
>+    mAccChildCount = eChildCountUninitialized;
>+    return;
>+  }
>+
>+  if (mAccChildCount != eChildCountUninitialized)
>+    return;
>+  nsCOMPtr<nsIAccessible> mNativeAccessible;
i don't think we typically want a blank line here:
>+  
>+};

plugins support is technically optional (--disable-plugins), please handle it.
>+#include "npapi.h"


I think you should support windows plugins which implement this feature...

> #ifdef XP_WIN
...
>+#elif MOZ_ACCESSIBILITY_ATK
...
>+    pluginInstance->GetValue(nsPluginInstanceVariable_NativeAccessible, &nativeAccessible);

I'm not sure we write 'Mozilla' here, i'd expect 'Gecko'
>+   * Introduced in Mozilla 1.9.2.
>+     * Introduced in Mozilla 1.9.2.

Comment 67

[Brad Taylor]

(In reply to comment #66)
> (From update of attachment 383918 [details] [diff] [review])
> >+nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):
> >+  nsLeafAccessible(nsnull, nsnull)
> why isn't this an initializer:
> >+  mAtkObject = aAtkObject;

Hmm, I'm not sure what you mean.  What needs to be initialized?
 
> is this a good name:
> >+nsHTMLAtkObjectOwnerAccessible::CacheChildren()
> it seems more like "update [child] cache" given the following:

This is an override from an inherited class.
 
> plugins support is technically optional (--disable-plugins), please handle it.
> >+#include "npapi.h"

This code is invoked by nsAccessibilityService when a plugin is found in the DOM tree.  To my knowledge, this should be handled correctly already.

> I think you should support windows plugins which implement this feature...

Windows plugins use a different mechanism to support a11y (see nsAccessibilityService::CreateHTMLObjectFrameAccessible), and Windows isn't really my expertise.

> I'm not sure we write 'Mozilla' here, i'd expect 'Gecko'
> >+   * Introduced in Mozilla 1.9.2.
> >+     * Introduced in Mozilla 1.9.2.

If you take a look at npapi.h, you'll see Mozilla used in other lines.  I'm just copying what is already there.

As far as the other comments (mainly whitespace and OOM related), I'll post a new patch shortly.

Comment 68

[Brad Taylor]

Attachment: Revised Patch (#8)

>i don't think we typically want a blank line here:
>+  
>+};

I couldn't actually find the blank line you're talking about...

Whitespace and OOM issues fixed.

Comment 69

[alexander :surkov (:asurkov)]

(In reply to comment #67)
> (In reply to comment #66)
> > (From update of attachment 383918 [details] [diff] [review] [details])
> > >+nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):
> > >+  nsLeafAccessible(nsnull, nsnull)
> > why isn't this an initializer:
> > >+  mAtkObject = aAtkObject;
> 
> Hmm, I'm not sure what you mean.  What needs to be initialized?

I think timeless means
+  nsLeafAccessible(nsnull, nsnull), mAtkObject(aAtkObject);

> > is this a good name:
> > >+nsHTMLAtkObjectOwnerAccessible::CacheChildren()
> > it seems more like "update [child] cache" given the following:
> 
> This is an override from an inherited class.

Right, Brad doesn't choose the name.

> 
> > plugins support is technically optional (--disable-plugins), please handle it.
> > >+#include "npapi.h"
> 
> This code is invoked by nsAccessibilityService when a plugin is found in the
> DOM tree.  To my knowledge, this should be handled correctly already.

The comment is you should wrap #include npapi.h by #ifdefs to handle this option. It's not about how we create accessible for plugin.

> 
> > I think you should support windows plugins which implement this feature...
> 
> Windows plugins use a different mechanism to support a11y (see
> nsAccessibilityService::CreateHTMLObjectFrameAccessible), and Windows isn't
> really my expertise.
> 
> > I'm not sure we write 'Mozilla' here, i'd expect 'Gecko'
> > >+   * Introduced in Mozilla 1.9.2.
> > >+     * Introduced in Mozilla 1.9.2.
> 
> If you take a look at npapi.h, you'll see Mozilla used in other lines.  I'm
> just copying what is already there.
> 
> As far as the other comments (mainly whitespace and OOM related), I'll post a
> new patch shortly.

Comment 70

[alexander :surkov (:asurkov)]

(In reply to comment #67)

> > I think you should support windows plugins which implement this feature...
> 
> Windows plugins use a different mechanism to support a11y (see
> nsAccessibilityService::CreateHTMLObjectFrameAccessible), and Windows isn't
> really my expertise.
> 

It probably makes sense to have spread this way on windows as well but it's subject of some discussion and out of scope of this bug I think.

Comment 71

[Brad Taylor]

Attachment: Revised Patch (#9)

(In reply to comment #69)
> > > >+  nsLeafAccessible(nsnull, nsnull)
> > > why isn't this an initializer:
> > > >+  mAtkObject = aAtkObject;
> > 
> > Hmm, I'm not sure what you mean.  What needs to be initialized?
> 
> I think timeless means
> +  nsLeafAccessible(nsnull, nsnull), mAtkObject(aAtkObject);

mAtkObject is a GObject, not a C++ object, so it does not have a copy constructor.

The rest of the comments have been addressed.

Comment 72

[alexander :surkov (:asurkov)]

ping for review please

Comment 73

[Jason Orendorff [:jorendorff]]

(In reply to comment #71)
> > > > >+  nsLeafAccessible(nsnull, nsnull)
> > > > why isn't this an initializer:
> > > > >+  mAtkObject = aAtkObject;
> > > 
> > > Hmm, I'm not sure what you mean.  What needs to be initialized?
> > 
> > I think timeless means
> > +  nsLeafAccessible(nsnull, nsnull), mAtkObject(aAtkObject);
> 
> mAtkObject is a GObject, not a C++ object, so it does not have a copy
> constructor.

That doesn't make sense to me. mAtkObject is a pointer. You should be able to initialize it using a member-initializer as timeless suggests. It doesn't matter whether the struct has a copy constructor (but I would guess it gets a default copy constructor anyway, even if it's in extern "C").

Comment 74

[alexander :surkov (:asurkov)]

No comments more than one month from reviewers, some feedback please.

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 385379 [details] [diff] [review]
Revised Patch (#9)

Sorry for the delay. I've been waiting josh's comments.

When creating patches, please use -U 8 p

>+nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):
>+  nsLeafAccessible(nsnull, nsnull)
>+{
>+  mAtkObject = aAtkObject;
>+  g_object_ref(mAtkObject);
Who releases mAtkObject?
I think it is nsAccessibleWrap, but could you add some comment.

>diff -r 882cfa39b73d modules/plugin/base/public/npapi.h
>--- a/modules/plugin/base/public/npapi.h	Fri Jun 26 14:49:37 2009 +0200
>+++ b/modules/plugin/base/public/npapi.h	Fri Jun 26 10:31:43 2009 -0400
>@@ -324,7 +324,13 @@
>   /* Checks if the plugin is interested in receiving the http body of
>    * all http requests (including failed ones, http status != 200).
>    */
>-  NPPVpluginWantsAllNetworkStreams = 18
>+  NPPVpluginWantsAllNetworkStreams = 18,
>+
>+  /* Get a native accessible for the plugin on the given system. For example,
>+   * this is expected to be an AtkObject on Linux.
>+   * Introduced in Mozilla 1.9.2.
>+   */
>+  NPNVNativeAccessible = 21
I don't know the rules about this file. Is it ok to just add new items to the enum.
Or do you need to increase NP_VERSION_MINOR?
And please make sure this feature gets documented in MDC.
And what is the expected "native accessible" in other systems.

Comment 76

[Brad Taylor]

(In reply to comment #73)
> (In reply to comment #71)
> > > > > >+  nsLeafAccessible(nsnull, nsnull)
> > > > > why isn't this an initializer:
> > > > > >+  mAtkObject = aAtkObject;
> > > > 
> > > > Hmm, I'm not sure what you mean.  What needs to be initialized?
> > > 
> > > I think timeless means
> > > +  nsLeafAccessible(nsnull, nsnull), mAtkObject(aAtkObject);
> > 
> > mAtkObject is a GObject, not a C++ object, so it does not have a copy
> > constructor.
> 
> That doesn't make sense to me. mAtkObject is a pointer. You should be able to
> initialize it using a member-initializer as timeless suggests. It doesn't
> matter whether the struct has a copy constructor (but I would guess it gets a
> default copy constructor anyway, even if it's in extern "C").

I assume you mean something like:
diff -r 772a2c3bde93 accessible/src/atk/nsHTMLAtkObjectAccessible.cpp
--- a/accessible/src/atk/nsHTMLAtkObjectAccessible.cpp	Fri Jun 26 09:52:03 2009 -0400
+++ b/accessible/src/atk/nsHTMLAtkObjectAccessible.cpp	Mon Jul 27 11:15:23 2009 -0400
@@ -43,9 +43,8 @@
 
 
 nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):
-  nsLeafAccessible(nsnull, nsnull)
+  nsLeafAccessible(nsnull, nsnull), mAtkObject(aAtkObject)
 {
-  mAtkObject = aAtkObject;
   g_object_ref(mAtkObject);
 }

right?  I get:

/home/brad/build/firefox-trunk/accessible/src/atk/nsHTMLAtkObjectAccessible.cpp: In constructor ‘nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject*)’:
/home/brad/build/firefox-trunk/accessible/src/atk/nsHTMLAtkObjectAccessible.cpp:46: error: class ‘nsHTMLAtkObjectAccessible’ does not have any field named ‘mAtkObject’

mAtkObject is a field on nsAccessibleWrap, which this class subclasses (via nsLeafAccessible.  Ideas?

(In reply to comment #75)
> (From update of attachment 385379 [details] [diff] [review])
> Sorry for the delay. I've been waiting josh's comments.
> 
> When creating patches, please use -U 8 p
> 
> >+nsHTMLAtkObjectAccessible::nsHTMLAtkObjectAccessible(AtkObject* aAtkObject):
> >+  nsLeafAccessible(nsnull, nsnull)
> >+{
> >+  mAtkObject = aAtkObject;
> >+  g_object_ref(mAtkObject);
> Who releases mAtkObject?
> I think it is nsAccessibleWrap, but could you add some comment.

Yes, it is nsAccessibleWrap.  I'll add the comment.

> I don't know the rules about this file. Is it ok to just add new items to the
> enum.

This change is going through review on the plugin-futures mailing list, so I'm working on it.

> Or do you need to increase NP_VERSION_MINOR?

I was hoping for the plugin people to chime in on this, but I assume it will happen once it is approved on plugin-futures.

> And please make sure this feature gets documented in MDC.

https://wiki.mozilla.org/Plugins:NativeAccessibility

Comment 77

[Brad Taylor]

Attachment: Revised Patch (#10)

Updated as per Olli's comment.

Comment 78

[timeless]

so...
class nsLeafAccessible : public nsAccessibleWrap
atk/class nsAccessibleWrap: public nsAccessible
132     AtkObject *mAtkObject;

I think I'd rather have nsLeafAccessible provide a protected constructor which lets it pass along another pointer to nsAccessibleWrap.

note that you still haven't handled my comment about plugins (ifdefs...)

Comment 79

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #76)
> > I don't know the rules about this file. Is it ok to just add new items to the
> > enum.
> 
> This change is going through review on the plugin-futures mailing list, so I'm
> working on it.

Any news about this?

Comment 80

[Brad Taylor]

(In reply to comment #78)
> so...
> class nsLeafAccessible : public nsAccessibleWrap
> atk/class nsAccessibleWrap: public nsAccessible
> 132     AtkObject *mAtkObject;
> 
> I think I'd rather have nsLeafAccessible provide a protected constructor which
> lets it pass along another pointer to nsAccessibleWrap.

nsLeafAccessible comes from accessible/src/base/, so it can't depend on Atk.  I could put it in nsAccessibleWrap (because it lives in accessible/src/atk), but I don't believe C++ allows you to inherit constructors.  I may be wrong though.
 
> note that you still haven't handled my comment about plugins (ifdefs...)

I just rebuilt firefox-central with --disable-plugins, and it built without issue, so I guess it isn't a problem.

Patch coming shortly that fixes some issues that arose in today's call with bsmedberg, davidb, josh and jst.

Comment 81

[Brad Taylor]

Attachment: Revised Patch (#11)

Updated patch as per phone discussion today.

Minutes: https://wiki.mozilla.org/Content_Processes/Meetings/2009-08-14-plugins

Comment 82

[Brad Taylor]

Comment on attachment 394560 [details] [diff] [review]
Revised Patch (#11)

Adding sr for jst for the API break, and David for final a11y review.

Comment 83

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 394560 [details] [diff] [review]
Revised Patch (#11)

r=me thanks.

Comment 84

[David Bolter [:davidb] (NeedInfo me for attention)]

BTW try server for this patch ran clean on trunk (except for a couple of known oranges). (Builds are here: https://build.mozilla.org/tryserver-builds/[e-mail]-atk_plugin_a11y/)

Comment 85

[Brad Taylor]

Bump.  Any way I can get reviews either way from olli and jst?  Thanks.

Comment 86

[Olli Pettay [:smaug][[e-mail]]]

Perhaps you could answer the question in comment 79.

Or is the link to the wiki page the answer?

Comment 87

[Brad Taylor]

(In reply to comment #86)
> Perhaps you could answer the question in comment 79.
> 
> Or is the link to the wiki page the answer?

The spec is available at https://wiki.mozilla.org/Plugins:NativeAccessibility and it's been proposed on plugin-futures since Aug 17 with no objections.  I'll ping josh and see if there's anything else he'd like me to do.

Comment 88

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 394560 [details] [diff] [review]
Revised Patch (#11)

>+  // 2) for plugins
>+  nsCOMPtr<nsIPluginInstance> pluginInstance;
>+  if (NS_SUCCEEDED(aFrame->GetPluginInstance(*getter_AddRefs(pluginInstance)))
>+      && pluginInstance) {
&& should be in the previous line.

If you answer to my question about NP_VERSION_MINOR, r=me, but josh should look at this, I think.

Comment 89

[Josh Aas]

Comment on attachment 394560 [details] [diff] [review]
Revised Patch (#11)

Update: This spec has been put off for the time being over concerns about out of process feasibility. See:

https://wiki.mozilla.org/Plugins:NativeAccessibility

Comment 90

[Brad Taylor]

Attachment: Revised Patch + OOP Support

In the last 6 months, my NPAPI extension has been approved, AtkPlug/AtkSocket support has been added to at-spi2, and I have a new patch which leverages the new API.

This patch works well in my testing, but I have a feeling there's some detection still left to be done to make sure that the AtkPlug/Socket support exists on the system.  Since I had to actually update FF's copy of atk in order to build, I'm not sure what mechanisms might be involved here.  David or Surkov, do you guys have any ideas?

Comment 91

[David Bolter [:davidb] (NeedInfo me for attention)]

Thank you! Gut reaction is "I like"... added Ginn as reviewer.

Comment 92

[Ginn Chen]

I think atk_get_version() may help.

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

Comment 93

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 437916 [details] [diff] [review]
Revised Patch + OOP Support

>+#include "nsAccessibleWrap.h"
>+//#include "nsMai.h"

Nit: you can remove this of course.

>+/**
>+ * This is a thin wrapper class that brings AtkSocket into Mozilla's
>+ * namespaces.
>+ */

Not sure what you mean by namespaces in this context?

>+#ifdef XP_UNIX
>+  /* Get a native accessible for the plugin on Linux.
>+   * This is expected to be an AtkObject.
>+   * Introduced in Gecko 1.9.2.

Will likely need to rev this to 1.9.3

I haven't caught any real errors yet. Do you think Ginn's suggestion will work? Is the ATK plug functionality coinciding with an at-spi version?

Comment 94

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #93)
> >+   * Introduced in Gecko 1.9.2.
> 
> Will likely need to rev this to 1.9.3

(Or 1.9.2.4 -- we'll see)

Comment 95

[alexander :surkov (:asurkov)]

(In reply to comment #93)

> >+/**
> >+ * This is a thin wrapper class that brings AtkSocket into Mozilla's
> >+ * namespaces.
> >+ */
> 
> Not sure what you mean by namespaces in this context?

I think this should be mean to plug into Mozilla accessibility tree. The comment should fixed of course.

Comment 96

[alexander :surkov (:asurkov)]

Comment on attachment 437916 [details] [diff] [review]
Revised Patch + OOP Support


>+ * Contributor(s):
>+ *   Brad Taylor ([e-mail]) (original author)

nit: use < > for email
>+nsAtkSocketAccessible::nsAtkSocketAccessible(nsIDOMNode *aDOMNode,
>+                                             nsIWeakReference *aShell):
>+  nsAccessibleWrap(aDOMNode, aShell)
>+{
>+  // Unref'ed by nsAccessibleWrap
>+  mAtkObject = atk_socket_new();

It sounds mAtkObject creation is implemented usually in GetNativeInterface(). I'd like Ginn to check where's the best place.

>+}
>+
>+void
>+nsAtkSocketAccessible::Embed(nsCString plugId)
>+{
>+  if (mAtkObject && !plugId.IsVoid()) {
>+    atk_socket_embed(ATK_SOCKET(mAtkObject), (gchar*)plugId.get());

is there opposite operation to unregister mAtkObject?

>+ * Contributor(s):
>+ *   Brad Taylor ([e-mail]) (original author)

nit: the same

>+/* For documentation of the accessibility architecture, 
>+ * see http://lxr.mozilla.org/seamonkey/source/accessible/accessible-docs.html
>+ */

nit: it's not necessary to put link here (and btw it's obsolete), please remove it.

>+
>+  void Embed(nsCString plugId);

nit: if the method will be saved then it should have a comment

>+  // 2) for plugins
>+  nsCOMPtr<nsIPluginInstance> pluginInstance;
>+  if (NS_SUCCEEDED(aFrame->GetPluginInstance(*getter_AddRefs(pluginInstance)))
>+      && pluginInstance) {

nit: operator shouldn't start new line.

>+#elif MOZ_ACCESSIBILITY_ATK
>+    nsAtkSocketAccessible* socketAccessible;

nit: please initialize it by nsnull.

>+
>+    nsCString plugId;
>+    pluginInstance->GetValueFromPlugin(NPPVnativeAccessibleAtkPlugId, &plugId);
>+    if (!plugId.IsVoid()) {
>+      socketAccessible = new nsAtkSocketAccessible(node, weakShell);
>+      socketAccessible->Embed(plugId);

probably the best way to use Init() instead an new Embed() method. I think Init() method should be called in InitAccessible() automatically.

>+
>+      NS_ENSURE_TRUE(socketAccessible, NS_ERROR_OUT_OF_MEMORY);

move the check before Embed() call.

Comment 97

[Ginn Chen]

(In reply to comment #92)
> I think atk_get_version() may help.
> 
> See: https://bugzilla.gnome.org/show_bug.cgi?id=460851

atk_get_version() is added in atk 1.20.
I guess it is beyond the minimum requirement of Firefox.

Firefox 3.6 requires GTK+ 2.10, which requires atk 1.9.0.

atk_socket_new() is added in GNOME 2.30.
So I think if you want to make it work with old GNOME, you have to use dlsym and function pointers.

Perhaps it makes sense to put all the code in #ifdef MOZ_ACCESSIBILITY_AT_SPI2, and turn it off by default.
Linux distro may turn on the option for GNOME 2.30 or later.

BTW: How did you test it? Did you patch moonlight?

Comment 98

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #96)
> (From update of attachment 437916 [details] [diff] [review])
> >+    if (!plugId.IsVoid()) {
> >+      socketAccessible = new nsAtkSocketAccessible(node, weakShell);
> >+      socketAccessible->Embed(plugId);
> 
> probably the best way to use Init() instead an new Embed() method. I think
> Init() method should be called in InitAccessible() automatically.

Yeah we strive to avoid platform #ifdefs.

Comment 99

[Brad Taylor]

(In reply to comment #96)
> >+}
> >+
> >+void
> >+nsAtkSocketAccessible::Embed(nsCString plugId)
> >+{
> >+  if (mAtkObject && !plugId.IsVoid()) {
> >+    atk_socket_embed(ATK_SOCKET(mAtkObject), (gchar*)plugId.get());
> 
> is there opposite operation to unregister mAtkObject?

No, but the embedding stops once the plug or the socket are unreffed, so it shouldn't really be a usecase.

> >+
> >+    nsCString plugId;
> >+    pluginInstance->GetValueFromPlugin(NPPVnativeAccessibleAtkPlugId, &plugId);
> >+    if (!plugId.IsVoid()) {
> >+      socketAccessible = new nsAtkSocketAccessible(node, weakShell);
> >+      socketAccessible->Embed(plugId);
> 
> probably the best way to use Init() instead an new Embed() method. I think
> Init() method should be called in InitAccessible() automatically.

Hmm, this would require Init to take an argument of nsCString (or void*) for the plugId.  Is this what you want?

(In reply to comment #97)
> (In reply to comment #92)
> > I think atk_get_version() may help.
> > 
> > See: https://bugzilla.gnome.org/show_bug.cgi?id=460851
> 
> atk_get_version() is added in atk 1.20.
> I guess it is beyond the minimum requirement of Firefox.
> 
> Firefox 3.6 requires GTK+ 2.10, which requires atk 1.9.0.
> 
> atk_socket_new() is added in GNOME 2.30.
> So I think if you want to make it work with old GNOME, you have to use dlsym
> and function pointers.
> 
> Perhaps it makes sense to put all the code in #ifdef MOZ_ACCESSIBILITY_AT_SPI2,
> and turn it off by default.
> Linux distro may turn on the option for GNOME 2.30 or later.

A third approach might be to use dlopen/dlsym to check if atk_socket_new exists.  This avoids doing a version check and instead checks for the feature itself, which is always a good thing.  I've done exactly that in my attached patch.  Let me know what you think of the approach.

> BTW: How did you test it? Did you patch moonlight?

I'm attaching a patch to moon and uia2atk that are required to test out the feature.  I will warn you, this whole stack is bleeding-edge (read: fragile) and dependency-heavy.

First, install at-spi2 (and pyatspi/accerciser if you want to test) from source and log out, making sure that at-spi2-registryd is now running.  Then, install mono, mcs, mono-basic and moon from trunk.  It's best to do this install in a parallel environment[1].

Configure, make and install moonlight on the system as usual, and also as an XPI (make user-plugin && firefox plugin/install/novell-moonlight.xpi).  Grab uia2atk[2], cd MoonAtkBridge, configure, make, and make xpi.  Install this XPI and point Firefox at a Silverlight page. (silverlight.net should do fine.)

You should see the accessibles show up in the tree.  If not, I'll be happy to stay up and help you get everything working on Beijing time; just let me know.  Also, you could always pop into #mono-a11y on irc.gimp.net, and Matt from our Beijing office should be able to help you get everything set up.

The Moonlight and MoonAtkBridge patches are not without warts though.  You need to run Firefox with MOON_DISABLE_SECURITY_DEBUG_ONLY=1 until I can figure out a security issue with Moonlight, and MoonAtkBridge causes Moonlight to segfault on exit.  These two bugs will be fixed soon though.

--
[1] http://www.mono-project.com/Parallel_Mono_Environments
[2] http://anonsvn.mono-project.com/source/trunk/uia2atk

Comment 100

[Brad Taylor]

Attachment: Revised Patch + OOP Support (v2)

Comment 101

[Brad Taylor]

Attachment: Moonlight OOP Accessibility Support

Comment 102

[Brad Taylor]

Attachment: Uia2atk OOP Accessibility Support

Comment 103

[alexander :surkov (:asurkov)]

I'd like to get Ginn as first reviewer. Just wonder can we change

MAI_ATK_OBJECT macros on MAI_ATK_OBJECT || MAI_ATK_SOCKET to  reuse the code.

Comment 104

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #103)
> I'd like to get Ginn as first reviewer.

Agreed. Ginn do you have time?

Comment 105

[Ginn Chen]

I'd like to review it. I'll try to do that in first week of May.

Comment 106

[Brad Taylor]

Attachment: Revised Patch + OOP Support (v3)

Updated to reflect Josh Aas's input, and to add a potential detection method for AtkPlug and AtkSocket.

Instead of having to use dlopen/dlsym for all of AtkSocket code, which would be error-prone, difficult to write and difficult to debug, I had the idea to check for the existence of atk_socket_new in libatk and use that as our detection method.  This should be a bit more robust than other methods and also be superior to version checking, as we check for functionality instead.

Thoughts?

Comment 107

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 442742 [details] [diff] [review]
Revised Patch + OOP Support (v3)

Drive by feedback:

>+#elif MOZ_ACCESSIBILITY_ATK
>+    void* atk = dlopen("libatk-1.0.so", RTLD_LAZY);
>+    NS_ENSURE_TRUE(atk, NS_ERROR_OUT_OF_MEMORY);
>+
>+    // The AtkSocket API is a soft dependency, as it was introduced in ATK 1.30
>+    // and Firefox requires ATK 1.19.
>+    void* atk_socket_new = dlsym(atk, "atk_socket_new");
>+    if (!atk_socket_new) {
>+      dlclose(atk);
>+      return NS_OK;
>+    }

Neat. Brad I forget, will the presence of ATK 1.30 also be a good indicator for the presence at-spi-dbus? If so I think I'd like this detector in a util method.

Comment 108

[Brad Taylor]

> Neat. Brad I forget, will the presence of ATK 1.30 also be a good indicator for
> the presence at-spi-dbus? If so I think I'd like this detector in a util
> method.

No, unfortunately it isn't a general way to detect if at-spi-dbus is running.  Remember that ATK exists completely independent of at-spi.  

To detect that at-spi-dbus is running, the best available technique is to query GConf:

/desktop/gnome/interface/at-spi-corba should be False
/desktop/gnome/interface/at-spi-dbus should be True

This could be made better if app developers step up and ask for a better method.  To date, no one has really needed this though.

Back to the subject at hand:  In this example, we can get away with detecting for just ATK 1.30 because the transport is irrelevant.  If you're running at-spi-corba, the feature is designed to degrade gracefully: you just won't be able to see the plugin's children.  If you have at-spi2 installed, magically you'll be able to see the plugin's accessibles.

Comment 109

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #108)
> > Neat. Brad I forget, will the presence of ATK 1.30 also be a good indicator for
> > the presence at-spi-dbus? If so I think I'd like this detector in a util
> > method.
> 
> No, unfortunately it isn't a general way to detect if at-spi-dbus is running. 
> Remember that ATK exists completely independent of at-spi.

Yep - was just hoping for some schedule alignment - but even then given the independence it would be fragile.
  
> To detect that at-spi-dbus is running, the best available technique is to query
> GConf:
> 
> /desktop/gnome/interface/at-spi-corba should be False
> /desktop/gnome/interface/at-spi-dbus should be True

That's fine if it is reliable. We check gconf for the accessibility key already. I'll keep it in mind if we need it later.

> 
> This could be made better if app developers step up and ask for a better
> method.  To date, no one has really needed this though.
> 
> Back to the subject at hand:  In this example, we can get away with detecting
> for just ATK 1.30 because the transport is irrelevant.  If you're running
> at-spi-corba, the feature is designed to degrade gracefully: you just won't be
> able to see the plugin's children.  If you have at-spi2 installed, magically
> you'll be able to see the plugin's accessibles.

Yep and that's totally fair. Thanks for the details Brad.

Comment 110

[Ginn Chen]

Comment on attachment 442742 [details] [diff] [review]
Revised Patch + OOP Support (v3)

Change CreateMaiInterfaces() to protected is no use here.

mai_atk_socket_ref_accessible_at_point()
I don't think it makes sense to do accWrap->GetChildAtPoint() here, we can just return the atk object.
Also
+  if (!accWrap || nsAccUtils::MustPrune(accWrap))
+    return nsnull;
I don't think MustPrune() is useful here.

nsAtkSocketAccessible::GetNativeInterface()
+    if (!mWeakShell || !nsAccUtils::IsEmbeddedObject(this)) {
I don't think IsEmbeddedObject() is useful here.

atk_socket_embed() / atk_socket_get_type() is used in the source without dlsym().
I think it might be trouble on some platforms.
You may need to define your own ATK_TYPE_SOCKET to get around.
So, to be safe, I think you might want to keep other-license/atk-1.0 unchanged and copy some defines into accessible/src/atk.

The change for npapi.h was already committed.

You need another reviewer for dom/ changes.

And, for moonlight,
I could not apply your patch cleanly now, I've some troubles to build moonlight with the patches, AtkObject is undefined.
When I complie uia2atk/MoonAtkBridge without patch, I got
./gtk-sharp/glib/GType.cs(230,79): error CS1501: No overload for method `GetReferencedAssemblies' takes `0' arguments.
What's the problem?

Comment 111

[Brad Taylor]

(In reply to comment #110)
> (From update of attachment 442742 [details] [diff] [review])
> Change CreateMaiInterfaces() to protected is no use here.

Hmm, this seems to be a leftover from refactoring.  WIll fix.

> mai_atk_socket_ref_accessible_at_point()
> I don't think it makes sense to do accWrap->GetChildAtPoint() here, we can just
> return the atk object.

But what if it's not in bounds?

> Also
> +  if (!accWrap || nsAccUtils::MustPrune(accWrap))
> +    return nsnull;
> I don't think MustPrune() is useful here.

As the comment above this method indicates, this is C&P from MaiInterfaceComponent, so I figured there was some reason for it.

> 
> nsAtkSocketAccessible::GetNativeInterface()
> +    if (!mWeakShell || !nsAccUtils::IsEmbeddedObject(this)) {
> I don't think IsEmbeddedObject() is useful here.

Same as above.

> atk_socket_embed() / atk_socket_get_type() is used in the source without
> dlsym().
> I think it might be trouble on some platforms.
> You may need to define your own ATK_TYPE_SOCKET to get around.
> So, to be safe, I think you might want to keep other-license/atk-1.0 unchanged
> and copy some defines into accessible/src/atk.

Hmm, I'm not sure if I follow your logic.  It would seem that as long as the code is not executed on platforms with old ATK versions, that we'll be fine.

There are two big problems with your approach:  First, the class (MaiAtkSocket) will have to be created using dlsym, and this is going to be very tricky and potentially impossible.  The biggest thing is the struct definition as any workaround will be very hairy.

The second problem I see is that it's going to make upgrading to the latest version of ATK in the future a bit tricky.  In particular, whoever decides to update will have to pull out all of the header file hacks and rewrite the code to use direct calls instead of dlsym.  This is completely non-obvious from the code layout, etc.

> The change for npapi.h was already committed.

Great, will update the patch tonight.


> You need another reviewer for dom/ changes.
> 
> And, for moonlight,
> I could not apply your patch cleanly now, I've some troubles to build moonlight
> with the patches, AtkObject is undefined.
> When I complie uia2atk/MoonAtkBridge without patch, I got
> ./gtk-sharp/glib/GType.cs(230,79): error CS1501: No overload for method
> `GetReferencedAssemblies' takes `0' arguments.
> What's the problem?

Moonlight has committed some changes which broke our code.  I'll get the latest from Mario on this and update the patch.

Comment 112

[Ginn Chen]

(In reply to comment #111)
> > mai_atk_socket_ref_accessible_at_point()
> > I don't think it makes sense to do accWrap->GetChildAtPoint() here, we can just
> > return the atk object.
> 
> But what if it's not in bounds?

OK, so it makes sense to reuse the code.
But we'd better not copy it.
Can you add refAccessibleAtPointHelper() getExtentsHelper() to nsMaiInterfaceComponent.h/cpp with nsAccessibleWrap * as first parameter?


> > atk_socket_embed() / atk_socket_get_type() is used in the source without
> > dlsym().
> > I think it might be trouble on some platforms.
> > You may need to define your own ATK_TYPE_SOCKET to get around.
> > So, to be safe, I think you might want to keep other-license/atk-1.0 unchanged
> > and copy some defines into accessible/src/atk.
> 
> Hmm, I'm not sure if I follow your logic.  It would seem that as long as the
> code is not executed on platforms with old ATK versions, that we'll be fine.

Right, but we have undefined/unresolvable symbol in our shared object, is it acceptable?

> 
> There are two big problems with your approach:  First, the class (MaiAtkSocket)
> will have to be created using dlsym, and this is going to be very tricky and
> potentially impossible.  The biggest thing is the struct definition as any
> workaround will be very hairy.
> 
> The second problem I see is that it's going to make upgrading to the latest
> version of ATK in the future a bit tricky.  In particular, whoever decides to
> update will have to pull out all of the header file hacks and rewrite the code
> to use direct calls instead of dlsym.  This is completely non-obvious from the
> code layout, etc.

OK, so I think you can still change atk headers in other-licenses.
Then you need to add 3 global functions pointers for atk_socket_get_type(), atk_socket_new(), atk_socket_embed().
(You probably don't need a function pointer for atk_socket_get_type(), you can just use a static GType.)
You can initialize these pointers in nsApplicationAccessibleWrap::PreCreate().
You can find similar code there, we did the hack for AtkHyperlinkImpl.

You should avoid using ATK_TYPE_SOCKET, ATK_IS_SOCKET() in mozilla a11y code, use function pointers instead.
Then I think we would be find.
Use nm to check libaccessiblity.so, it should not contain atk_socket_get_type, atk_socket_new, atk_socket_embed.

Comment 113

[Mario Carrion]

(In reply to comment #110)
> And, for moonlight,
> I could not apply your patch cleanly now, I've some troubles to build moonlight
> with the patches, AtkObject is undefined.
> When I complie uia2atk/MoonAtkBridge without patch, I got
> ./gtk-sharp/glib/GType.cs(230,79): error CS1501: No overload for method
> `GetReferencedAssemblies' takes `0' arguments.
> What's the problem?
Bug was fixed, please use the following revisions of the following modules to build moonlight:
- mono: r158115
- mcs: r158115
- mono-basic: r158115
- moon: r158268

After checking out the repositories run "./autogen.sh" in moon, then "make install" (you will probably need to setup a parallel development environment[1][2] to do that) and then "./autogen.sh && make" in uia2atk/MoonAtkBridge.

About the AtkObject error, replacing "AtkObject" with "char" will allow you to compile it.

[1] http://www.mono-project.com/Parallel_Mono_Environments
[2] http://blog.carrion.ws/2010/01/25/parallel-development-environments-pulque/

Comment 114

[Mario Carrion]

Attachment: Revised Patch + OOP Support (v4)

Patch updated to reflect Ginn's input. Please let me know if something else is missing. Thanks in advance.

The changes in Moonlight are already committed, you don't have to patch it anymore. To test this patch you have to checkout the following modules:
- svn co -r 159672 http://anonsvn.mono-project.com/source/trunk/moon
- svn co -r 159337 http://anonsvn.mono-project.com/source/trunk/mono
- svn co -r 159337 http://anonsvn.mono-project.com/source/trunk/mcs
- svn co -r 159337 http://anonsvn.mono-project.com/source/trunk/mono-basic
- svn co http://anonsvn.mono-project.com/source/trunk/uia2atk

Then apply "uia2atk/MoonAtkBridge/patches/moon_trunk/moonlight-a11y-core-extension.diff" to moon, execute "./autogen.sh", "make", "make install" and "make user-plugin".

The detailed steps are here: http://mono-project.com/Accessibility:_Moonlight

If you have any problem building or installing moonlight and moonatkbridge let me know and I will do my best to help you.

Comment 115

[Ginn Chen]

While compiling mono, I got

.libs/mini.o: In function `mini_cleanup':
/home/ginn/work/moonlight/mono/mono/mini/mini.c:6166: undefined reference to `mono_runtime_shutdown'
/usr/bin/ld.bfd.real: .libs/libmono-2.0.so.1.0.0: hidden symbol `mono_runtime_shutdown' isn't defined

what is the problem?

Thanks.

Comment 116

[Mario Carrion]

(In reply to comment #115)
> While compiling mono, I got
> 
> .libs/mini.o: In function `mini_cleanup':
> /home/ginn/work/moonlight/mono/mono/mini/mini.c:6166: undefined reference to
> `mono_runtime_shutdown'
> /usr/bin/ld.bfd.real: .libs/libmono-2.0.so.1.0.0: hidden symbol
> `mono_runtime_shutdown' isn't defined
> 
> what is the problem?
Moonlight requires specific versions of mono/mcs/mono-basic, but before building Moonlight you have to install those versions and use that mono to build Moonlight.

I updated the wiki to reflect the new steps: (http://mono-project.com/Accessibility:_Moonlight), specifically you want to read: http://mono-project.com/Accessibility:_Moonlight#Building_for_Firefox_.28mozilla-central.29 and then http://mono-project.com/Accessibility:_Moonlight#Setting_up_the_MoonAtkBridge_Environment_.28mozilla-central.29

I hope that helps.

Comment 117

[Ginn Chen]

(In reply to comment #116)
> (In reply to comment #115)
> > While compiling mono, I got
> > 
> > .libs/mini.o: In function `mini_cleanup':
> > /home/ginn/work/moonlight/mono/mono/mini/mini.c:6166: undefined reference to
> > `mono_runtime_shutdown'
> > /usr/bin/ld.bfd.real: .libs/libmono-2.0.so.1.0.0: hidden symbol
> > `mono_runtime_shutdown' isn't defined
> > 
> > what is the problem?
> Moonlight requires specific versions of mono/mcs/mono-basic, but before
> building Moonlight you have to install those versions and use that mono to
> build Moonlight.

I was messing up with an old mono trunk installed on the system.

Now I have built moonlight, and it seems it works fine with Minefield.
However when I switch to atspi2 on Ubuntu 10.04, I could not run accerciser.
Is there any trick for it?

Comment 118

[Sandy Armstrong]

I believe this is a known bug with pyatspi2/atspi2, which has been fixed in the latest development versions.

I am not 100% sure, but I believe the packages in our PPA do not suffer from this problem:

https://edge.launchpad.net/~mono-a11y/+archive/ppa

Comment 119

[Ginn Chen]

Thanks, I just figured it out.
Rebuilding pyatspi and accerciser solved the problem.

Now I got this error in gdb when I load a silverlight page.

(firefox-bin:22806): Moonlight-WARNING **: Moonlight: probing for browser type failed (or browser bridge was disabled), user agent = `Mozilla/5.0 (X11; Linux i686; en-US; rv:2.0b2pre) Gecko/20100709 Minefield/4.0b2pre'
Moonlight: Plugin AppDomain Creation: OK

(firefox-bin:22806): atk-bridge-WARNING **: AT_SPI_REGISTRY was not started at session startup.

(firefox-bin:22806): atk-bridge-WARNING **: IOR not set.

(firefox-bin:22806): atk-bridge-WARNING **: Could not locate registry
....
Program received signal SIGPWR, Power fail/restart.

If I continue, I got this error later,

(firefox-bin:22806): Moonlight-CRITICAL **: AtkObject* AccessibilityBridge::GetRootAccessible(): assertion `get_accessible' failed

Can you give me a screenshot of accerciser?
I want to see how silverlight plugin is displayed in accerciser on your side.

Thanks!

Comment 120

[Mario Carrion]

Attachment: Silverlight Control and Accerciser

Comment 121

[Mario Carrion]

(In reply to comment #119)
> Thanks, I just figured it out.
> Rebuilding pyatspi and accerciser solved the problem.
> 
> Now I got this error in gdb when I load a silverlight page.
> 
> (firefox-bin:22806): Moonlight-WARNING **: Moonlight: probing for browser type
> failed (or browser bridge was disabled), user agent = `Mozilla/5.0 (X11; Linux
> i686; en-US; rv:2.0b2pre) Gecko/20100709 Minefield/4.0b2pre'
> Moonlight: Plugin AppDomain Creation: OK
> 
> (firefox-bin:22806): atk-bridge-WARNING **: AT_SPI_REGISTRY was not started at
> session startup.
> 
> (firefox-bin:22806): atk-bridge-WARNING **: IOR not set.
> 
> (firefox-bin:22806): atk-bridge-WARNING **: Could not locate registry
> ....
> Program received signal SIGPWR, Power fail/restart.
> 
> If I continue, I got this error later,
It seems that, for some reason, all these error are because you are using both atspi1 and atspi2 and Firefox is trying to use atspi1. I'm not 100% sure of what would be causing this, are you using our packages?

> (firefox-bin:22806): Moonlight-CRITICAL **: AtkObject*
> AccessibilityBridge::GetRootAccessible(): assertion `get_accessible' failed
It seems that Moonlight is not finding that method in MoonAtkBridge, it might be related to the previous issues.
 
> Can you give me a screenshot of accerciser?
Done: attachment (id=456574)

Thanks.

Comment 122

[Stephen Shaw]

It's possible that at-spi is still on by default if the package didn't come from the mono uia ppa or if you didn't update gconf to reflect at-spi2 as the default.

Comment 123

[Ginn Chen]

I was using http://ppa.launchpad.net/accessibility-dev/ppa/ubuntu and gconf keys.
After removing atspi packages, moonlight can go further, but it still not working correctly. I guess atspi2 is buggy, I've problems with Firefox even without moonlight. I got duplicate entries for 1 tab.

I want to switch http://ppa.launchpad.net/mono-a11y/ppa/ubuntu, but it reports missing libxevie1 package.

Comment 124

[Ginn Chen]

In general the patch is OK, thanks.

A few comments:

1) next time please use diff=-p -U 8
2) nsAccessibilityService.cpp
I think we needn't to use dlopen/dlclose here.
Just use "if (g_atk_socket_embed)" should be enough.
3) nsAtkSocketAccessible::GetNativeInterface()
Remove
+    if (!mWeakShell || !nsAccUtils::IsEmbeddedObject(this)) {
+      // We don't create ATK objects for node which has been shutdown, or
+      // nsIAccessible plain text leaf nodes
+      return NS_ERROR_FAILURE;
+    }
4) In nsAtkSocketAccessible::Embed()
Nit:
+      accSocket = G_TYPE_CHECK_INSTANCE_CAST ((acc), g_atk_socket_type, AtkSocket);
remove extra space and brackets:
+      accSocket = G_TYPE_CHECK_INSTANCE_CAST(acc, g_atk_socket_type, AtkSocket);
5) +    *aPlugId = nsCString(plugId, -1);
Just do    *aPlugId = nsCString(plugId);

Please go ahead to ask review for dom side.
I'll do more testing after my vacation.

BTW: Does it work if ipc is disabled in about:config?

Comment 125

[Mario Carrion]

Attachment: Revised Patch + OOP Support (v5)

Patch updated to reflect Ginn's comments.

CC'ing DOM owners to get a review.

Comment 126

[Mario Carrion]

(In reply to comment #124)
> BTW: Does it work if ipc is disabled in about:config?
No it doesn't work.

Comment 127

[Peter Van der Beken [:peterv]]

Comment on attachment 457142 [details] [diff] [review]
Revised Patch + OOP Support (v5)

I am not the right reviewer for this patch.

Comment 128

[Ginn Chen]

(In reply to comment #126)
> (In reply to comment #124)
> > BTW: Does it work if ipc is disabled in about:config?
> No it doesn't work.

Why?
Can we make it work?

Comment 129

[Mario Carrion]

(In reply to comment #128)
> (In reply to comment #126)
> > (In reply to comment #124)
> > > BTW: Does it work if ipc is disabled in about:config?
> > No it doesn't work.
> 
> Why?
> Can we make it work?
I'm not familiar with this option, however if you tell me a place to look for more information about it I will research and I will update the patch to allow using it with this option disabled.

Comment 130

[Mario Carrion]

Comment on attachment 457142 [details] [diff] [review]
Revised Patch + OOP Support (v5)

Ping?

Comment 131

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 457142 [details] [diff] [review]
Revised Patch + OOP Support (v5)

I'm not the right reviewer for this, and I suspect neither is any of the other requestees.

Maybe Alexander Surkov for the a11y parts, and Josh Aas for the plugin parts?

Comment 132

[alexander :surkov (:asurkov)]

Comment on attachment 457142 [details] [diff] [review]
Revised Patch + OOP Support (v5)

changing review requests per comment #131. Ginn should be perfect reviewer for a11y part here

Comment 133

[David Bolter [:davidb] (NeedInfo me for attention)]

Ginn, ping?

Comment 134

[Ginn Chen]

Comment on attachment 457142 [details] [diff] [review]
Revised Patch + OOP Support (v5)

Sorry for the delay, the patch looks good to me.

Comment 135

[alexander :surkov (:asurkov)]

Comment on attachment 457142 [details] [diff] [review]
Revised Patch + OOP Support (v5)

common nits (it's up to you to ignore 1-2 since it's common style in atk part)

Please
1) use type* not type *
2) use cast operators instead of implicit casting
3) do type* acc = nsnull; not type* acc;

>+// Soft references to AtkSocket
>+typedef void (* AtkSocketEmbedType) (AtkSocket*, gchar*);
>+AtkSocketEmbedType g_atk_socket_embed = NULL;
>+GType g_atk_socket_type = G_TYPE_INVALID;
>+static const char sATKSocketEmbedSymbol[] = "atk_socket_embed";
>+static const char sATKSocketGetTypeSymbol[] = "atk_socket_get_type";

nit: I don't quite understand this comment

>+void
>+nsAtkSocketAccessible::Embed(nsCString plugId)
>+{
>+  void* acc;
>+  nsresult rv = GetNativeInterface(&acc);
>+  if (NS_FAILED(rv))
>+    return;
>+
>+  // Using G_TYPE macros instead of ATK_SOCKET macros
>+  // to avoid undefined symbols.
>+  if (IsEmbeddable() && G_TYPE_CHECK_INSTANCE_TYPE(acc, g_atk_socket_type) &&
>+      !plugId.IsVoid()) {

don't do double check, add assertions instead

>+    AtkSocket *accSocket;
>+    accSocket = G_TYPE_CHECK_INSTANCE_CAST(acc, g_atk_socket_type, AtkSocket);
>+    g_atk_socket_embed(accSocket, (gchar*)plugId.get());
>+  }
>+}
>+
>+int
>+nsAtkSocketAccessible::IsEmbeddable()
>+{
>+  return g_atk_socket_type != G_TYPE_INVALID && g_atk_socket_embed;
>+}

should be static, it makes sense to move it to nsApplicationAccessibleWrap where they get initialized

>+  /*
>+   * Returns a positive value if the current Atk version supports AtkSocket
>+   * and it was correctly loaded.
>+   */
>+  int IsEmbeddable();

use bool instead

>     NS_IF_ADDREF(accessible);
>     return accessible;
>+#elif MOZ_ACCESSIBILITY_ATK

nit: empty line between please

>+    nsAtkSocketAccessible* socketAccessible = nsnull;
>+    nsCString plugId;
>+    nsresult rv;
>+
>+    socketAccessible = new nsAtkSocketAccessible(aContent, weakShell);
>+    if (!socketAccessible->IsEmbeddable()) {
>+      delete socketAccessible;
>+      return nsnull;
>+    }
>+
>+    rv = pluginInstance->GetValueFromPlugin(
>+      NPPVpluginNativeAccessibleAtkPlugId, &plugId);
>+    if (NS_SUCCEEDED(rv) && !plugId.IsVoid()) {
>+      socketAccessible = new nsAtkSocketAccessible(aContent, weakShell);

leak

Comment 136

[Mike Gorse]

Attachment: Updated patch.

Some changes from Mario to address the last review, and updated to apply against current central.

Comment 137

[Trevor Saunders (:tbsaunde)]

Comment on attachment 527155 [details] [diff] [review]
Updated patch.

>diff --git a/accessible/src/atk/Makefile.in b/accessible/src/atk/Makefile.in
>--- a/accessible/src/atk/Makefile.in
>+++ b/accessible/src/atk/Makefile.in
>@@ -43,16 +43,17 @@ include $(DEPTH)/config/autoconf.mk
> 
> MODULE = accessibility
> LIBRARY_NAME = accessibility_toolkit_s
> EXPORT_LIBRARY = ..
> LIBXUL_LIBRARY = 1
> 
> 
> CPPSRCS = \
>+  nsAtkSocketAccessible.cpp \

I think we'd prefer to stop starting file names with ns if we could.

> EXPORTS = \
>+  nsAtkSocketAccessible.h \

same

>diff --git a/accessible/src/atk/nsApplicationAccessibleWrap.cpp b/accessible/src/atk/nsApplicationAccessibleWrap.cpp
>--- a/accessible/src/atk/nsApplicationAccessibleWrap.cpp
>+++ b/accessible/src/atk/nsApplicationAccessibleWrap.cpp
>@@ -61,16 +61,23 @@ static const char sATKLibName[] = "libat
> static const char sATKHyperlinkImplGetTypeSymbol[] =
>     "atk_hyperlink_impl_get_type";
> static const char sAccEnv [] = "GNOME_ACCESSIBILITY";
> static const char sSysPrefService [] =
>     "@mozilla.org/system-preference-service;1";
> static const char sAccessibilityKey [] =
>     "config.use_system_prefs.accessibility";
> 
>+// Soft references to AtkSocket
>+typedef void (* AtkSocketEmbedType) (AtkSocket*, gchar*);
>+AtkSocketEmbedType g_atk_socket_embed = NULL;
>+GType g_atk_socket_type = G_TYPE_INVALID;

I'd really rather not have these be global, I think the nicest solution maye to make them static globals in the atkSocketAccessible class

>@@ -720,16 +727,21 @@ nsApplicationAccessibleWrap::PreCreate()
> {
>     if (!sATKChecked) {
>         sATKLib = PR_LoadLibrary(sATKLibName);
>         if (sATKLib) {
>             AtkGetTypeType pfn_atk_hyperlink_impl_get_type = (AtkGetTypeType) PR_FindFunctionSymbol(sATKLib, sATKHyperlinkImplGetTypeSymbol);
>             if (pfn_atk_hyperlink_impl_get_type) {
>                 g_atk_hyperlink_impl_type = pfn_atk_hyperlink_impl_get_type();
>             }

nit, get rid of the braces while here if you don't mind.

>+void
>+nsAtkSocketAccessible::Shutdown()
>+{
>+  if (mAtkObject) {
>+    if (MAI_IS_ATK_SOCKET(mAtkObject)) {
>+      MAI_ATK_SOCKET(mAtkObject)->accWrap = nsnull;
>+    }
>+    g_object_unref(mAtkObject);
>+    mAtkObject = nsnull;
>+  }
>+  nsAccessibleWrap::Shutdown();
>+}
>+

>+int
>+nsAtkSocketAccessible::IsEmbeddable()
>+{
>+  return g_atk_socket_type != G_TYPE_INVALID && g_atk_socket_embed;
>+}

I think I'd prefer this to be be a constant that we set during initialization, bool  AtkSocketAccessible::IsEmbedable? in any case I think it should return a bool not a int

>+#ifndef _nsAtkSocketAccessible_H_
>+#define _nsAtkSocketAccessible_H_
>+
>+#include "nsAccessibleWrap.h"
>+#include "nsCoreUtils.h"
>+#include "nsAccUtils.h"

how many of those actually need to be included if you forward declare the classes you need?

>+  // nsAccessNode
>+  void Shutdown();

I think that wants to  be virtual

>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
>@@ -90,16 +90,22 @@
> #include "nsXULTreeGridAccessibleWrap.h"
> #endif
> 
> // For native window support for object/embed/applet tags
> #ifdef XP_WIN
> #include "nsHTMLWin32ObjectAccessible.h"
> #endif
> 
>+// For embedding plugin accessibles
>+#ifdef MOZ_ACCESSIBILITY_ATK
>+#include <dlfcn.h>
>+#include "nsAtkSocketAccessible.h"
>+#endif
>+
>+#elif MOZ_ACCESSIBILITY_ATK
>+    nsAtkSocketAccessible* socketAccessible = nsnull;
>+    nsCString plugId;
>+    nsresult rv;
>+
>+    socketAccessible = new nsAtkSocketAccessible(aContent, weakShell);
>+    if (!socketAccessible->IsEmbeddable()) {
>+      delete socketAccessible;
>+      return nsnull;
>+    }

creating an  object to test if we can use atk socket is a pain, it'd be much nicer if we had instead in  AtkSocketAccessible sttic bool gCanEmbed which we could test before making the object.

>+
>+    rv = pluginInstance->GetValueFromPlugin(
>+      NPPVpluginNativeAccessibleAtkPlugId, &plugId);
>+    if (NS_SUCCEEDED(rv) && !plugId.IsVoid()) {
>+      socketAccessible = new nsAtkSocketAccessible(aContent, weakShell);

this  leaks the object socketAccessible used to point at that we created to call  IsEmbedable() on

>+      socketAccessible->Embed(plugId);

thought, I wonder if it would be reasonable to add a constructor that takes the plug id?

>diff --git a/dom/plugins/ipc/PluginInstanceChild.h b/dom/plugins/ipc/PluginInstanceChild.h
>--- a/dom/plugins/ipc/PluginInstanceChild.h
>+++ b/dom/plugins/ipc/PluginInstanceChild.h
>@@ -87,17 +87,19 @@ class PluginInstanceChild : public PPlug
> 
>+    virtual bool
>+    AnswerNPP_GetValue_NPPVpluginNativeAccessibleAtkPlugId(nsCString* aPlugId,
>+                                                     NPError* aResult);

The bool seems redundant to me here, I'd just use the pointer being NULL,  but using a bool seems to be  consistant with the code here, and I don't know anythng about plugins so I'm fine with this.

This generally looks fine to me, but I think I'd like to see a little cleanup before r+ing this

Comment 138

[Mike Gorse]

Attachment: Updated patch.

I think this addresses all of Trevor's comments.

One question: Is Firefox 6 going to require ATK 1.30?  I saw some IRC discussion about this but don't know if a conclusion was reached.  If so, then this patch could be simplified somewhat, since it currently needs to dynamically load anything involving AtkPlug and AtkSocket.

Comment 139

[Mike Gorse]

Attachment: Updated patch.

Replaced IsEmbeddable with a static bool.

Comment 140

[Mike Gorse]

Attachment: Updated patch.

More style fixes.

Comment 141

[alexander :surkov (:asurkov)]

Attachment: patch #20

Fixing some style nits. I don't like we deal with atk socket globals too much outside the atk socket file (nsAcessibilityService and nsApplicationAccessibleWrap). Ideally I would enclose them into static variables.

Trevor, could you finish this patch please to make things faster?

Comment 142

[alexander :surkov (:asurkov)]

(In reply to comment #138)
> Created attachment 527751 [details] [diff] [review]
> Updated patch.
> 
> I think this addresses all of Trevor's comments.
> 
> One question: Is Firefox 6 going to require ATK 1.30?  I saw some IRC
> discussion about this but don't know if a conclusion was reached.

What new will be in this version? Does it provide new text inserted/removed events? If so then I think yes.

Comment 143

[alexander :surkov (:asurkov)]

(In reply to comment #141)
> Created attachment 528265 [details] [diff] [review]
> patch #20
> 
> Fixing some style nits. I don't like we deal with atk socket globals too much
> outside the atk socket file (nsAcessibilityService and
> nsApplicationAccessibleWrap). Ideally I would enclose them into static
> variables.

read: methods

Comment 144

[Trevor Saunders (:tbsaunde)]

(In reply to comment #143)
> (In reply to comment #141)
> > Created attachment 528265 [details] [diff] [review]
> > patch #20
> > 
> > Fixing some style nits. I don't like we deal with atk socket globals too much
> > outside the atk socket file (nsAcessibilityService and
> > nsApplicationAccessibleWrap). Ideally I would enclose them into static
> > variables.
> 
> read: methods

I agree its not ideal,  but for now I think its the best option.   all of the stuff in nsAccessibleWrap can go away when we can stop supporting older atk, and I don't think its worth much time to make legacy support code nice.  we could make a static inline accessor for gCanEmbed I gues, but that seems silly to not use a global in nsAccessibilityService

Comment 145

[alexander :surkov (:asurkov)]

fine with me, let's land the patch if you're happy with it

Comment 146

[Trevor Saunders (:tbsaunde)]

(In reply to comment #142)
> (In reply to comment #138)
> > Created attachment 527751 [details] [diff] [review]
> > Updated patch.
> > 
> > I think this addresses all of Trevor's comments.
> > 
> > One question: Is Firefox 6 going to require ATK 1.30?  I saw some IRC
> > discussion about this but don't know if a conclusion was reached.
> 
> What new will be in this version? Does it provide new text inserted/removed
> events? If so then I think yes.

NO, IIRC THAT'S  1.32 OR THE NEXT RELEASE FROM THE BGINING OF THE MONTH 2.0
the  problem here is that  libxul.so links against atk which means that any linux machine firefox runs on needs atleast the minimum version of atk THAT we support. There  are a couple workarounds, but not particularly nice, we could dlopen libatk.so or we could   ship some sort of libatk.so ourselves that gets used in the absense of a acceptable system one.

Comment 147

[Trevor Saunders (:tbsaunde)]

Comment on attachment 528265 [details] [diff] [review]
patch #20

r=me some optional nits to fix before landing  but either way unless we need a plugins person to look at this again I think your good to go :)

>+void
>+
>+  AtkSocketAccessible(nsIContent* aContent, nsIWeakReference* aShell,
>+                      nsCString aPlugId);

It seems like it might be useful in the future to be able to create a socket then embed, but if we need that then we can add them later.

>+AtkObject*
>+refAccessibleAtPointHelper(nsAccessibleWrap* aAccWrap, gint aAccX, gint aAccY,
>+                           AtkCoordType aCoordType)
>+{
>+  if (!aAccWrap || nsAccUtils::MustPrune(aAccWrap))
>+      return nsnull;

since  you already dexpcomed  this one we should probably have an IsDefunct() check sooner rather than later?

>+
>+  // nsIAccessible getChildAtPoint (x,y) is in screen pixels.
>+  if (aCoordType == ATK_XY_WINDOW) {
>+    nsIntPoint winCoords =
>+      nsCoreUtils::GetScreenCoordsForWindow(aAccWrap->GetNode());
>+    aAccX += winCoords.x;
>+    aAccY += winCoords.y;
>+  }
>+
>+  nsAccessible* accAtPoint = aAccWrap->GetChildAtPoint(aAccX, aAccY,
>+                                                       nsAccessible::eDirectChild);
>+  if (!accAtPoint)
>+    return nsnull;
>+
>+  AtkObject* atkObj = nsAccessibleWrap::GetAtkObject(accAtPoint);
>+  if (atkObj)
>+    g_object_ref(atkObj);
>+  return atkObj;
>+}
>+
>+void
>+getExtentsHelper(nsAccessibleWrap* aAccWrap,
>+                 gint* aX, gint* aY, gint* aWidth, gint* aHeight,
>+                 AtkCoordType aCoordType)
>+{
>+  *aX = *aY = *aWidth = *aHeight = 0;
>+
>+  if (!aAccWrap)
>+    return;

I think it would be nice to add an IsDefunct() check here and in  ref accessible at point helper but that can wait for   when we dexpcom GetBounds()

>+
>+  PRInt32 x = 0, y = 0, width = 0, height = 0;
>+  // Returned in screen coordinates
>+  nsresult rv = accWrap->GetBounds(&x, &y, &width, &height);
>+  if (NS_FAILED(rv))
>+    return;
>+
>+  if (aCoordType == ATK_XY_WINDOW) {
>+    nsIntPoint winCoords =
>+      nsCoreUtils::GetScreenCoordsForWindow(aAccWrap->GetNode());
>+    x -= winCoords.x;
>+    y -= winCoords.y;
>+  }
>+
>+  *aX = x;
>+  *aY = y;
>+  *aWidth = width;
>+  *aHeight = height;
>+}

WHY NOT JUST PASS THE ARGUMENTS THROUGH?

>@@ -54,16 +54,22 @@ void getExtentsCB(AtkComponent *aCompone
>                   gint *aAccX, gint *aAccY,
>                   gint *aAccWidth, gint *aAccHeight,
>                   AtkCoordType aCoordType);
> /* the "contains", "get_position", "get_size" can take advantage of
>  * "get_extents", there is no need to implement them now.
>  */
> gboolean grabFocusCB(AtkComponent *aComponent);
> 
>+AtkObject* refAccessibleAtPointHelper(nsAccessibleWrap* aAccWrap,
>+                                      gint aX, gint aY, AtkCoordType aCoordType);
>+void getExtentsHelper(nsAccessibleWrap* aAccWrap,
>+                      gint* aX, gint* aY, gint* aWidth, gint* aHeight,
>+                      AtkCoordType aCoordType);
>+
> /* what are missing now for atk component */

WHICH?

Comment 148

[alexander :surkov (:asurkov)]

(In reply to comment #147)
> Comment on attachment 528265 [details] [diff] [review]
> patch #20
> 
> r=me some optional nits to fix before landing  but either way unless we need a
> plugins person to look at this again I think your good to go :)

I think it's ok to go without final approval from plugins persons since this bug can't regress us even if something got broken from previous patch revisions. We always can deal with it in followups.

> >+  if (!aAccWrap || nsAccUtils::MustPrune(aAccWrap))
> >+      return nsnull;
> 
> since  you already dexpcomed  this one we should probably have an IsDefunct()
> check sooner rather than later?

good catch

> I think it would be nice to add an IsDefunct() check here and in  ref
> accessible at point helper but that can wait for   when we dexpcom GetBounds()

fine with me

> >+  PRInt32 x = 0, y = 0, width = 0, height = 0;
> >+  // Returned in screen coordinates
> >+  nsresult rv = accWrap->GetBounds(&x, &y, &width, &height);

> >+  *aX = x;
> >+  *aY = y;
> 
> WHY NOT JUST PASS THE ARGUMENTS THROUGH?

I think because gint* and PRint32* are considered as different types.

> > /* what are missing now for atk component */
> 
> WHICH?

they are listed below

Comment 149

[alexander :surkov (:asurkov)]

compile error: no atkmisk.h file that's included into atk.h

Comment 150

[alexander :surkov (:asurkov)]

Attachment: patch #21

Comment 151

[alexander :surkov (:asurkov)]

landed - http://hg.mozilla.org/mozilla-central/rev/0c49216dd89f

Comment 152

[alexander :surkov (:asurkov)]

Guys, please verify the patch.

Comment 153

[Mike Gorse]

Can this be back-ported for FF 5?  What is the policy for backporting / deciding what can and can't be backported?  I'm asking because it has implications for the Moonlight ATK bridge / may determine which versions of Firefox it can support.

Comment 154

[Fernando Herrera]

Thank you very much for the patch. 

I'm afraid that we cannot backport to FF5, as this is a new feature. The good news are that Firefox 6 is coming very soon: we are branching next week for Aurora, so this feature will be next week on the Aurora Channel and some time later in beta and the firefox 6.