665672 - move nsIAccessibleHyperLink off nsAccessible

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

at some point someone provided noxpcom versions of the methods for this interface and started using them in msaa/ and atk/ so lets finish moving the interface off nsAccessible

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: wip

- create xpcom/nsHyperLinkAccessible.{cpp,h} containing an xpcom class nsHyperLinkAccessible that implements nsIAccesssibleHyperLink by calling the nonxpcom methods on nsAccessible.

I think we'll need to change the way some classes that always implement nsIAccessibleHyperLink implement QueryInterface().

Comment 2

[alexander :surkov (:asurkov)]

I'm not sure I like the idea to create new object every time when it was requested.

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to comment #2)
> I'm not sure I like the idea to create new object every time when it was
> requested.

that's fine, how about we keep a pointer to one we create on the first request in nsAccessible?

also, does it seem reasonable to write some temporary qi impls where we currntly use macro ones so that we return the object we want not ourselves?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (In reply to comment #2)
> > I'm not sure I like the idea to create new object every time when it was
> > requested.
> 
> that's fine, how about we keep a pointer to one we create on the first
> request in nsAccessible?

don't like either. I want to keep xpcom implementation separately from internal objects.

> also, does it seem reasonable to write some temporary qi impls where we
> currntly use macro ones so that we return the object we want not ourselves?

I don't see real advantages in our case to not use the same object for querying.

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: add a cache for xpcom accessibles

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: bug 665672 - move nsIAccessibleHyperLink off nsAccessible

Comment 7

[Trevor Saunders (:tbsaunde)]

Attachment: build fixes and start using cached xpcom accessibles

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 541617 [details] [diff] [review]
build fixes and  start using cached xpcom accessibles

this patch is based on something and doesn't provide whole picture

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to comment #8)
> Comment on attachment 541617 [details] [diff] [review] [review]
> build fixes and  start using cached xpcom accessibles
> 
> this patch is based on something and doesn't provide whole picture

yes, the other two patches I just attached, together hopefully there's more of a picture if its easier I can give you one patch containing all three.

Comment 10

[alexander :surkov (:asurkov)]

maybe not neccessary. So you suggest to have one big object that has implementation of all XPCOM interfaces?

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to comment #10)
> maybe not neccessary. So you suggest to have one big object that has
> implementation of all XPCOM interfaces?

well, atleast those implemented  on nsAccessNode and children.  That should be more efficient since we'll only ever have one  xpcom object for each accessible, and it may also cut down the header boilerplate we'd need if each interface  got its own class / object.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> (In reply to comment #10)
> > maybe not neccessary. So you suggest to have one big object that has
> > implementation of all XPCOM interfaces?
> 
> well, atleast those implemented  on nsAccessNode and children.

I meant any kind of XPCOM interface, like table and etc. Can you show me class hierarchy for xpcom objects you keep in mind?

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > maybe not neccessary. So you suggest to have one big object that has
> > > implementation of all XPCOM interfaces?
> > 
> > well, atleast those implemented  on nsAccessNode and children.
> 
> I meant any kind of XPCOM interface, like table and etc. Can you show me
> class hierarchy for xpcom objects you keep in mind?

I mean XPCOMAccessible would implement nsIAccessNode nsIAccessibleTable nsIAccessibleTableCell nsIAccessibleApplication etc, just not nsIAccessible relation nsIAccessibilityService nsIAccessibleProvider  does that make more sense?  basically all the ones it would make sense to exist on an accessible but not the other ones we have.

Comment 14

[alexander :surkov (:asurkov)]

It should be ok until xpcom interfaces impl requires to keep members. Do you think to keep all interface implementation in one file or keeping them in different classes and then use multiple inheritance?

Comment 15

[Trevor Saunders (:tbsaunde)]

Comment on attachment 541614 [details] [diff] [review]
add a cache for xpcom accessibles

Alex, tell me if you'd like me to change how these are broken up, but I'd like to see what you think of this implementation of the cache and what you want done differently :-)

Comment 16

[alexander :surkov (:asurkov)]

the approach should be fine, except I don't like many thousand lines files (isn't multiple inheritance approach nicer?)

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to comment #16)
> the approach should be fine, except I don't like many thousand lines files
> (isn't multiple inheritance approach nicer?)

Could you explain the multiple inheritance idea? I roughly see that we could have one xpcom class that's just nsIAccessible implementor and something that inherits from that for nsIAccessible table as a second xpcom class, but wouldn't this lead to need for a lot of classes? supose you have an accessible that should implement nsIAccessibleHyperText nsIAccessibleTableCell and nsIAccessible?  

I think we should only have one xpcom object per each accessible for memory reasons, but other than that I'm happy to consider other ways.    That's why for example I didn't take the route of having a class for each interface, and each accessible would have as many objects as implemented interfaces.

I'm not aware of any rule that says all the implementation of a class must be in one file, so in this case it might be reasonable to have the implementation of each interfaces functions in a different cpp file, but all would belong to one class.

I think this should be nice, but I think in general we'd prefer to just work with the code outside of xpcom/ and leave this code alone right?

Comment 18

[alexander :surkov (:asurkov)]

for multiple inheritance we could have one class for one interface, then we have several classes, each is for set of interfaces, similar to IA2 implementation.

In this case objects should take less memory than having one big objects. Correct?

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to comment #18)
> for multiple inheritance we could have one class for one interface, then we
> have several classes, each is for set of interfaces, similar to IA2
> implementation.

you mean  the tearoffish things for CAccessibleHyperalsam
you mean like the way 
> 
> In this case objects should take less memory than having one big objects.
> Correct?

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to comment #18)
> for multiple inheritance we could have one class for one interface, then we
> have several classes, each is for set of interfaces, similar to IA2
> implementation.
> 
> In this case objects should take less memory than having one big objects.
> Correct?

no, I think it would be the same as 1 big class module the number of vtables.  either way you have one class that  ends up implementing all the interfaces either through inheritance or not, and has one member the pointer to the native accessible right?

currently it seems like this approach would be worse, because you have to downcast this to the class that iplements  all the interfaces, and has the pointer to the accessible.  We wouldn't neccessarily need to use do_QueryObject() like we do in msaa/ now but we'd need something which would add to the amount of code we'd need, and maybe hurt perf a little.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> (In reply to comment #18)
> > for multiple inheritance we could have one class for one interface, then we
> > have several classes, each is for set of interfaces, similar to IA2
> > implementation.
> > 
> > In this case objects should take less memory than having one big objects.
> > Correct?
> 
> no, I think it would be the same as 1 big class module the number of
> vtables.  either way you have one class that  ends up implementing all the
> interfaces either through inheritance or not, and has one member the pointer
> to the native accessible right?

It's not evident why we should end up with one class implementing all interfaces. 

Following this way all XPCOM objects will have the same size, i.e. 11 (amount of interfaces) + 1 (pointer to native object). Taking into account that most objects need to implement 1 interface (combined version of nsIAccessible + nsIAccessNode), we have 48 vs 8 bytes per each object on 32 bit systems. That's not what we can do.

> currently it seems like this approach would be worse, because you have to
> downcast this to the class that iplements  all the interfaces, and has the
> pointer to the accessible.  We wouldn't neccessarily need to use
> do_QueryObject() like we do in msaa/ now but we'd need something which would
> add to the amount of code we'd need, and maybe hurt perf a little.

we always deal with memory savings vs perf. In this case I don't think downcasting hits a perf really (trivial if and static cast).

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)

> we have 48 vs 8 bytes per each object on 32
> bit systems.

actually I speculated by numbers, most of objects are hypertext object but anyway they don't implement half of interfaces (like table, tablecell, image, value, application, document, selectable interfaces).

Comment 23

[Trevor Saunders (:tbsaunde)]

Attachment: wip

approach multiple classes for accessibles that implement different interfaces.
so, one class XPCOMAccessible implements nsIAccessible
multiple classes inherit from XPCOMAccessible one for objects that should implment nsIAccessibleTable a different class for nsIAccessibleFooText
to do this  we'll probably need to only have a few of this types because nsIAccessibleHyperText and nsIAccessibleEditableText need to be implemented on the same object, but nsIAccessibleTable shouldn't need to be on that object same with TableCell or Value.

Comment 24

[alexander :surkov (:asurkov)]

based on our irc chat and latest patch to make sure we talk about same things:

1) introduce xpClass that implements one of set of xpcom interfaces (can be inherited from each other), for example, xpAccessible (nsIAccessible, nsIAccessibleHyperLink), xpHyperTextAccessible (nsIAccessibleText, nsIAccessibleEditableText and etc), xpTableAccessible (nsIAccessibleTable). These are bricks to build final classes.

2) introduce nsClass that are inherited from xpClass, for example, nsARIAGridAccessible inherited from xpTableAccessible and xpHyperTextAccessible. Classes that conincide with xp classes can be defined as typedefs.

3) xpClass and nsClass are template that take native class, that allows to avoid downcasting. Native class knows final type of nsClass and responsible to create nsClass instance (is there better option)?

4) root accessible manages the cache of xpcom objects

Comment 25

[Trevor Saunders (:tbsaunde)]

> 
> 2) introduce nsClass that are inherited from xpClass, for example,
> nsARIAGridAccessible inherited from xpTableAccessible and
> xpHyperTextAccessible. Classes that conincide with xp classes can be defined

does it actually make sense for the grid itself to be both a table and have text? (just a nit pick for the example not general question)

> as typedefs.

ok, I think the generally makes sense, but lets implement this as we have  interfaces we can move off native accessibles and see how it goes.

> 3) xpClass and nsClass are template that take native class, that allows to
> avoid downcasting. Native class knows final type of nsClass and responsible

i'M NOT SURE THE TEMPLATING IS NEEDED.  i  WAS THINKING WE'D  have all classes inherit from one class that holds the pointer to the non xpcom accessible then we could just use accessible downcasting on it to get the interface we need  which should be pretty fast since in many cases that downcast will just be a check of mFlags inlined.

> to create nsClass instance (is there better option)?

when everything is done, I think the only place we need to be able to create xpcom accessibles is  in the impl for nsIAccessibilityService for java script and whoever else outside of our code wants to use the service.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> > 
> > 2) introduce nsClass that are inherited from xpClass, for example,
> > nsARIAGridAccessible inherited from xpTableAccessible and
> > xpHyperTextAccessible. Classes that conincide with xp classes can be defined
> 
> does it actually make sense for the grid itself to be both a table and have
> text? (just a nit pick for the example not general question)

ideally probably not, because of IA2 restrictions all tables should implement it (though we don't always follow it).

> > as typedefs.
> 
> ok, I think the generally makes sense, but lets implement this as we have 
> interfaces we can move off native accessibles and see how it goes.

not sure I share this point, we'd need to understand how we are going to implement.

> > 3) xpClass and nsClass are template that take native class, that allows to
> > avoid downcasting. Native class knows final type of nsClass and responsible
> 
> i'M NOT SURE THE TEMPLATING IS NEEDED.  i  WAS THINKING WE'D  have all
> classes inherit from one class that holds the pointer to the non xpcom
> accessible then we could just use accessible downcasting on it to get the
> interface we need  which should be pretty fast since in many cases that
> downcast will just be a check of mFlags inlined.

sure you can do downcasting but that sound strange since downcasting is just workaround the xpcom class doesn't know everything about native class. Templating is going to fix that. The approach assumes we have few or several template specializations so extra memory issue isn't a problem here.

> > to create nsClass instance (is there better option)?
> 
> when everything is done, I think the only place we need to be able to create
> xpcom accessibles is  in the impl for nsIAccessibilityService for java
> script and whoever else outside of our code wants to use the service.

that requires accessibility service to know which type of xpcom class the native class is going to use. Making native class to create xpcom object or provide a type of xpcom class (the second might be overkill) allows to avoid many ifs in accessibility service. Note, in your latest patch xpcom class type information is provided by native class. But you can avoid templates usage if native class creates it itself.

Comment 27

[alexander :surkov (:asurkov)]

Comment on attachment 541614 [details] [diff] [review]
add a cache for xpcom accessibles

canceling review until we get an agreement