Last Comment Bug 672507 - merge nsIAccessNode and nsIAccessible
: merge nsIAccessNode and nsIAccessible
Status: RESOLVED FIXED
: access, addon-compat
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: cleanxpcoma11y
  Show dependency treegraph
 
Reported: 2011-07-19 07:55 PDT by alexander :surkov
Modified: 2012-05-11 08:18 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part1 (9.25 KB, patch)
2011-07-26 17:45 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (52.65 KB, patch)
2012-01-10 08:47 PST, Trevor Saunders (:tbsaunde)
dbolter: review+
Details | Diff | Splinter Review
patch (51.87 KB, patch)
2012-01-26 21:08 PST, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
patch (56.99 KB, patch)
2012-02-01 23:13 PST, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
bug 672507 - fix review nit (3.82 KB, patch)
2012-02-10 12:20 PST, Trevor Saunders (:tbsaunde)
dbolter: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-07-19 07:55:41 PDT
Every object that implement nsIAccessible implement nsIAccessNode (in terms of XPCOM) since we removed accessnode idea from XPCOM. That allows to save 4 bytes per instance by merging these interfaces.
Comment 1 Trevor Saunders (:tbsaunde) 2011-07-26 17:45:27 PDT
Created attachment 548641 [details] [diff] [review]
part1

move GetRootAccessible() to nsIAccessible and since nobody needs nsAccessNode to have the nonxpcom version move it to nsAccessible
Comment 2 alexander :surkov 2011-07-27 22:04:20 PDT
Comment on attachment 548641 [details] [diff] [review]
part1

Review of attachment 548641 [details] [diff] [review]:
-----------------------------------------------------------------

please bump uids of nsIAccessNode/nsIAccessible

::: accessible/public/nsIAccessible.idl
@@ +153,5 @@
>     */
>    readonly attribute unsigned long role;
>  
>    /**
> +   * The root document accessible that this access node resides in.

nit: access node -> accessible

::: accessible/src/base/nsAccessNode.h
@@ -104,5 @@
>  
>    /**
> -   * Return the root document accessible for this accessnode.
> -   */
> -  nsRootAccessible* RootAccessible() const;

I wouldn't move RootAccessible from nsAccessNode to nsAccessible, since you can't move everything, for example, GetDocAccessible() will stay in nsAccessNode because it's used in msaa/nsAccessNodeWrap. Keeping RootAccessible and DocAccessible in different classes is confusing.

Possibly we could get rid nsAccessNode at all if our consumers can work in case when ISimpleDOMNode interface is exposed only on objects that implement IAccessible. I'll try to figure out if it works for them.
Comment 3 Trevor Saunders (:tbsaunde) 2011-07-28 13:50:01 PDT
> 
> ::: accessible/src/base/nsAccessNode.h
> @@ -104,5 @@
> >  
> >    /**
> > -   * Return the root document accessible for this accessnode.
> > -   */
> > -  nsRootAccessible* RootAccessible() const;
> 
> I wouldn't move RootAccessible from nsAccessNode to nsAccessible, since you
> can't move everything, for example, GetDocAccessible() will stay in
> nsAccessNode because it's used in msaa/nsAccessNodeWrap. Keeping

where? I can't find it used in msaa/nsAccessNodewrap.cpp or h
Comment 4 alexander :surkov 2011-07-28 20:47:01 PDT
(In reply to comment #3)
> > 
> > ::: accessible/src/base/nsAccessNode.h
> > @@ -104,5 @@
> > >  
> > >    /**
> > > -   * Return the root document accessible for this accessnode.
> > > -   */
> > > -  nsRootAccessible* RootAccessible() const;
> > 
> > I wouldn't move RootAccessible from nsAccessNode to nsAccessible, since you
> > can't move everything, for example, GetDocAccessible() will stay in
> > nsAccessNode because it's used in msaa/nsAccessNodeWrap. Keeping
> 
> where? I can't find it used in msaa/nsAccessNodewrap.cpp or h

I was confused by nsAccUtils version. Anyway I don't get a point to move it, since it's a property of access node.
Comment 5 Trevor Saunders (:tbsaunde) 2011-08-08 03:28:39 PDT
(In reply to alexander surkov from comment #4)
> (In reply to comment #3)
> > > 
> > > ::: accessible/src/base/nsAccessNode.h
> > > @@ -104,5 @@
> > > >  
> > > >    /**
> > > > -   * Return the root document accessible for this accessnode.
> > > > -   */
> > > > -  nsRootAccessible* RootAccessible() const;
> > > 
> > > I wouldn't move RootAccessible from nsAccessNode to nsAccessible, since you
> > > can't move everything, for example, GetDocAccessible() will stay in
> > > nsAccessNode because it's used in msaa/nsAccessNodeWrap. Keeping
> > 
> > where? I can't find it used in msaa/nsAccessNodewrap.cpp or h
> 
> I was confused by nsAccUtils version. Anyway I don't get a point to move it,
> since it's a property of access node.

hmm, so, there's a difference between a property of  accessible and accessNode, but not for the xpcom interfaces? that doesn't really make sense.
Comment 6 alexander :surkov 2011-08-08 03:47:53 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> hmm, so, there's a difference between a property of  accessible and
> accessNode, but not for the xpcom interfaces? that doesn't really make sense.

We don't get rid concept of accessnode since it's used on MSAA layer, but there's no similar concept on XPCOM layer. Accessnode belongs to some document and root document so these methods are applicable for accessnode but granted not used, you just move the code that doesn't make huge sense to move.
Comment 7 Trevor Saunders (:tbsaunde) 2011-08-08 04:48:25 PDT
(In reply to alexander surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > hmm, so, there's a difference between a property of  accessible and
> > accessNode, but not for the xpcom interfaces? that doesn't really make sense.
> 
> We don't get rid concept of accessnode since it's used on MSAA layer, but
> there's no similar concept on XPCOM layer. Accessnode belongs to some
> document and root document so these methods are applicable for accessnode
> but granted not used, you just move the code that doesn't make huge sense to
> move.

So, I think things like nsDocAccessible will cause problems if we want to try and seperate xpcom and ms com completely while using the internal object to implement msaa / ia2 methods.  Also note that after we kill nsIWinAccessNode the only interfaces on nsAccessNodeWrap are ISimpleDOMNode and IServiceProvider.  While I'm not quiet sure what IServiceProvider does, I haven't looked yet it looks like its just one static method.  So, I'm thinking the impl of ISimpleDOMNode almost needs to become a tear off, or atleast a  native object with no other interfaces on the highest type object in the hyerarchy.  However if  the impl of ISimpleDOM was a tear from the native object for the  native accessible object we  wouldn't have to change much, but could get rid of accessnodes.

However if you'd prefer I won't object too much more to leaving the native imple of GetRootAcc(0 on nsAccessNode.
Comment 8 alexander :surkov 2011-08-08 06:21:58 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> So, I think things like nsDocAccessible will cause problems if we want to
> try and seperate xpcom and ms com completely while using the internal object
> to implement msaa / ia2 methods.

What kind of problems you can foresee?

>  Also note that after we kill
> nsIWinAccessNode the only interfaces on nsAccessNodeWrap are ISimpleDOMNode
> and IServiceProvider.  While I'm not quiet sure what IServiceProvider does,
> I haven't looked yet it looks like its just one static method.

Don't forget about IUnknown :) Anyway, IServiceProvider provides QueryService method that is similar to QueryInterface but can be used to get objects different than this one.

>  So, I'm
> thinking the impl of ISimpleDOMNode almost needs to become a tear off, or
> atleast a  native object with no other interfaces on the highest type object
> in the hyerarchy.  However if  the impl of ISimpleDOM was a tear from the
> native object for the  native accessible object we  wouldn't have to change
> much, but could get rid of accessnodes.

What is advantages of having it as tearoff? Is the point we should take care about ATs that don't use these interfaces? This should be nice.

> we  wouldn't have to change
> much, but could get rid of accessnodes.

IA2 and ISimpleDOMNode share some methods (like ScrollTo), keeping accessnode allows us to keep methods like this on it and do not care, otherwise the only one way I can see is to move these methods into utility classes. Is that what you keep in mind?

> However if you'd prefer I won't object too much more to leaving the native
> imple of GetRootAcc(0 on nsAccessNode.

please object and argue.

I find appeal the idea to get rid accessnode in the end (though I'm still not quite sure that's the best alternative for us). On the another hand this bug is not related with this idea. Surely I'm fine to get the code that suites for future needs but I need to be convinced it really is.
Comment 9 Trevor Saunders (:tbsaunde) 2011-08-08 20:35:40 PDT
(In reply to alexander surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> 
> > So, I think things like nsDocAccessible will cause problems if we want to
> > try and seperate xpcom and ms com completely while using the internal object
> > to implement msaa / ia2 methods.
> 
> What kind of problems you can foresee?

The one I've just noticed is nsDocAccessible inheriting from nsIDocumentObserver, nsIObserver, nsIScrollPositionListener nad nsSupportsWeakReference.

ll> >  Also note that after we ki
> > nsIWinAccessNode the only interfaces on nsAccessNodeWrap are ISimpleDOMNode
> > and IServiceProvider.  While I'm not quiet sure what IServiceProvider does,
> > I haven't looked yet it looks like its just one static method.
> 
> Don't forget about IUnknown :) Anyway, IServiceProvider provides
> QueryService method that is similar to QueryInterface but can be used to get
> objects different than this one.

ok, well, IUnknown only sort of counts ;)

>> >  So, I'm
> > thinking the impl of ISimpleDOMNode almost needs to become a tear off, or
> > atleast a  native object with no other interfaces on the highest type object
> > in the hyerarchy.  However if  the impl of ISimpleDOM was a tear from the
> > native object for the  native accessible object we  wouldn't have to change
> > much, but could get rid of accessnodes.
> 
> What is advantages of having it as tearoff? Is the point we should take care
> about ATs that don't use these interfaces? This should be nice.

yes, mostly for all the cases where we don't have users for the interface, windows at not using ISimpleDOMNode, linux / mac at, and browser addon that uses our xpcom interfaces would all seem to be cases like this.

>> > we  wouldn't have to change
> > much, but could get rid of accessnodes.
> 
> IA2 and ISimpleDOMNode share some methods (like ScrollTo), keeping
> accessnode allows us to keep methods like this on it and do not care,
> otherwise the only one way I can see is to move these methods into utility
> classes. Is that what you keep in mind?

well, tbh I had caught that but it  sounds sensible to have a shared class, or maybe lave the ISimpleDOMNode tear off hold a pointer to  an accessible that it can call the method on.

> > However if you'd prefer I won't object too much more to leaving the native
> > imple of GetRootAcc(0 on nsAccessNode.
> 
> please object and argue.
> 
> I find appeal the idea to get rid accessnode in the end (though I'm still
> not quite sure that's the best alternative for us). On the another hand this
> bug is not related with this idea. Surely I'm fine to get the code that
> suites for future needs but I need to be convinced it really is.

sweets for future needs doesn't parse for e, can phrase what you mean by that last sentence differently?
Comment 10 alexander :surkov 2011-08-08 20:55:36 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #9)

> The one I've just noticed is nsDocAccessible inheriting from
> nsIDocumentObserver, nsIObserver, nsIScrollPositionListener nad
> nsSupportsWeakReference.

It think it's ok if we don't get rid nsISupports from some classes. Though I miss the point how it's related with accessnode stuffs.

> >> >  So, I'm
> > > thinking the impl of ISimpleDOMNode almost needs to become a tear off, or
> > > atleast a  native object with no other interfaces on the highest type object
> > > in the hyerarchy.  However if  the impl of ISimpleDOM was a tear from the
> > > native object for the  native accessible object we  wouldn't have to change
> > > much, but could get rid of accessnodes.
> > 
> > What is advantages of having it as tearoff? Is the point we should take care
> > about ATs that don't use these interfaces? This should be nice.
> 
> yes, mostly for all the cases where we don't have users for the interface,
> windows at not using ISimpleDOMNode, linux / mac at, and browser addon that
> uses our xpcom interfaces would all seem to be cases like this.

this interface isn't exposed for linux and mac, after MSAA objects separation, it won't hit pure xpcom interface users. There's only one valid point to keep it as tearoff is don't spend extra memory in the case of ATs that don't use ISimpleDOM interfaces.

> > IA2 and ISimpleDOMNode share some methods (like ScrollTo), keeping
> > accessnode allows us to keep methods like this on it and do not care,
> > otherwise the only one way I can see is to move these methods into utility
> > classes. Is that what you keep in mind?
> 
> well, tbh I had caught that but it  sounds sensible to have a shared class,
> or maybe lave the ISimpleDOMNode tear off hold a pointer to  an accessible
> that it can call the method on.

ISimpleDOMNode object may be have accessible object. That's the whole point of accessnode class.

> > I find appeal the idea to get rid accessnode in the end (though I'm still
> > not quite sure that's the best alternative for us). On the another hand this
> > bug is not related with this idea. Surely I'm fine to get the code that
> > suites for future needs but I need to be convinced it really is.
> 
> sweets for future needs doesn't parse for e, can phrase what you mean by
> that last sentence differently?

My point is it doesn't make big sense to do RootAccessible transition from nsAccessNode to nsAccessible as part of this bug until we are sure we will do that in future.
Comment 11 Trevor Saunders (:tbsaunde) 2011-08-08 21:10:39 PDT
(In reply to alexander surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> > The one I've just noticed is nsDocAccessible inheriting from
> > nsIDocumentObserver, nsIObserver, nsIScrollPositionListener nad
> > nsSupportsWeakReference.
> 
> It think it's ok if we don't get rid nsISupports from some classes. Though I
> miss the point how it's related with accessnode stuffs.

well, it would be related if we didn't seperate the saa / ia2 ipls onto there own object, becuase it would mean that we'd still have class implementing mscom and xpcom, but if we're  definitely seperating the objects then it would seem to not be related.

> > >> >  So, I'm
> > > > thinking the impl of ISimpleDOMNode almost needs to become a tear off, or
> > > > atleast a  native object with no other interfaces on the highest type object
> > > > in the hyerarchy.  However if  the impl of ISimpleDOM was a tear from the
> > > > native object for the  native accessible object we  wouldn't have to change
> > > > much, but could get rid of accessnodes.
> > > 
> > > What is advantages of having it as tearoff? Is the point we should take care
> > > about ATs that don't use these interfaces? This should be nice.
> > 
> > yes, mostly for all the cases where we don't have users for the interface,
> > windows at not using ISimpleDOMNode, linux / mac at, and browser addon that
> > uses our xpcom interfaces would all seem to be cases like this.
> 
> this interface isn't exposed for linux and mac, after MSAA objects
> separation, it won't hit pure xpcom interface users. There's only one valid
> point to keep it as tearoff is don't spend extra memory in the case of ATs
> that don't use ISimpleDOM interfaces.

well, killing nsAccessNode would effect linux and mac, because nsAccessible would have one less vtable right?

> > > IA2 and ISimpleDOMNode share some methods (like ScrollTo), keeping
> > > accessnode allows us to keep methods like this on it and do not care,
> > > otherwise the only one way I can see is to move these methods into utility
> > > classes. Is that what you keep in mind?
> > 
> > well, tbh I had caught that but it  sounds sensible to have a shared class,
> > or maybe lave the ISimpleDOMNode tear off hold a pointer to  an accessible
> > that it can call the method on.
> 
> ISimpleDOMNode object may be have accessible object. That's the whole point
> of accessnode class.
> 
> > > I find appeal the idea to get rid accessnode in the end (though I'm still
> > > not quite sure that's the best alternative for us). On the another hand this
> > > bug is not related with this idea. Surely I'm fine to get the code that
> > > suites for future needs but I need to be convinced it really is.
> > 
> > sweets for future needs doesn't parse for e, can phrase what you mean by
> > that last sentence differently?
> 
> My point is it doesn't make big sense to do RootAccessible transition from
> nsAccessNode to nsAccessible as part of this bug until we are sure we will
> do that in future.

I think it would be interesting /helpful to remove everything that isn't needed on nsAccessNode and see what is  left, but I don't feel a strong need to ove it now.
Comment 12 alexander :surkov 2011-08-08 21:35:25 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #11)

> > It think it's ok if we don't get rid nsISupports from some classes. Though I
> > miss the point how it's related with accessnode stuffs.
> 
> well, it would be related if we didn't seperate the saa / ia2 ipls onto
> there own object, becuase it would mean that we'd still have class
> implementing mscom and xpcom, but if we're  definitely seperating the
> objects then it would seem to not be related.

Ok, we have nsDocAccessible that implements both nsISupports and IUknonwn, how is it related with accessnode?

> > > > What is advantages of having it as tearoff? Is the point we should take care
> > > > about ATs that don't use these interfaces? This should be nice.
> > > 
> > > yes, mostly for all the cases where we don't have users for the interface,
> > > windows at not using ISimpleDOMNode, linux / mac at, and browser addon that
> > > uses our xpcom interfaces would all seem to be cases like this.
> > 
> > this interface isn't exposed for linux and mac, after MSAA objects
> > separation, it won't hit pure xpcom interface users. There's only one valid
> > point to keep it as tearoff is don't spend extra memory in the case of ATs
> > that don't use ISimpleDOM interfaces.
> 
> well, killing nsAccessNode would effect linux and mac, because nsAccessible
> would have one less vtable right?

you told here about keeping ISimpleDOMNode as tear-off, it's not directly related with keeping the nsAccessNode. You may keep or not ISimpleDOMNode as tear-off and the same time have or not have nsAccessNode.

> I think it would be interesting /helpful to remove everything that isn't
> needed on nsAccessNode and see what is  left, but I don't feel a strong need
> to ove it now.

I prefer to plan things rather than do and see how it goes. Obviously you can't just get rid nsAccessNode class at all because it's virtual and interface methods are used for ISimpleDOMNode implementation. You have few alternatives to proceed it, one of them is keep nsAccessNode (not nice but surely easiest one).

Also the easiest way is just to fix this bug and save future stuffs for future.
Comment 13 Trevor Saunders (:tbsaunde) 2011-08-09 01:03:18 PDT
(In reply to alexander surkov from comment #12)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> 
> > > It think it's ok if we don't get rid nsISupports from some classes. Though I
> > > miss the point how it's related with accessnode stuffs.
> > 
> > well, it would be related if we didn't seperate the saa / ia2 ipls onto
> > there own object, becuase it would mean that we'd still have class
> > implementing mscom and xpcom, but if we're  definitely seperating the
> > objects then it would seem to not be related.
> 
> Ok, we have nsDocAccessible that implements both nsISupports and IUknonwn,
> how is it related with accessnode?

its not really this started out with you asking about problems with seperating xpcom from mscom that would cause us to need to seperate the objects.

> > I think it would be interesting /helpful to remove everything that isn't
> > needed on nsAccessNode and see what is  left, but I don't feel a strong need
> > to ove it now.
> 
> I prefer to plan things rather than do and see how it goes. Obviously you

heh, I'll agree a plan is good and you should now what direction  you want to go, but I don't tend  to like to create a detailed plan about every detail, because I'll invariably overlook an important issue.

> can't just get rid nsAccessNode class at all because it's virtual and
> interface methods are used for ISimpleDOMNode implementation. You have few
> alternatives to proceed it, one of them is keep nsAccessNode (not nice but
> surely easiest one).
> 
> Also the easiest way is just to fix this bug and save future stuffs for
> future.

true, the easiest thing is to just leave it, but oving the stuff that there is true, but there isn't much reason not to move this particular method GetDocAccessible() and whatever else isn't used by something that needs to care about access nodes.
Comment 14 alexander :surkov 2011-08-09 02:24:25 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #13)

> true, the easiest thing is to just leave it, but oving the stuff that there
> is true, but there isn't much reason not to move this particular method
> GetDocAccessible() and whatever else isn't used by something that needs to
> care about access nodes.

GetDocAccessible() is characteristic of accessnode since it belongs to document, that's a reason to not move this method. The fact that we don't develop ISimpleDOMNode anymore is sort of guarantee that this method won't be used but still not a reason to move it. If we are going to get rid accessnode from crossplatform layer then this is a reason to do that now.
Comment 15 Trevor Saunders (:tbsaunde) 2012-01-10 08:47:29 PST
Created attachment 587334 [details] [diff] [review]
patch
Comment 16 David Bolter [:davidb] 2012-01-13 07:58:16 PST
Comment on attachment 587334 [details] [diff] [review]
patch

Review of attachment 587334 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/nsAccessNode.h
@@ +72,5 @@
>    0x2b07e3d7,                                           \
>    0x00b3,                                               \
>    0x4379,                                               \
>    { 0xaa, 0x0b, 0xea, 0x22, 0xe2, 0xc8, 0xff, 0xda }    \
>  }

You removed the NS_ACCESSNODE_IMPL_CID accessor macro, so we probably don't need this #define anymore?
Comment 17 David Bolter [:davidb] 2012-01-13 08:18:14 PST
Comment on attachment 587334 [details] [diff] [review]
patch

Review of attachment 587334 [details] [diff] [review]:
-----------------------------------------------------------------

nit: a lot of comments exists where you've simply removed the reference to nsIAccessNode, but the comment could be further reformatted to line up better.

Looking at this patch makes me wish we had about:memory implementation. Rebooting into Windows to test patch...
Comment 18 David Bolter [:davidb] 2012-01-13 08:42:02 PST
Windows test run:

mochitest-a11y failed:
15428 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/test_nsIAccessNode_utils.html | Can't query [object Object] for span
15429 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/test_nsIAccessNode_utils.html | an unexpected uncaught JS exception reported thro
ugh window.onerror - uncaught exception: [Exception... "Operation is not supported"  code: "9" nsresult: "0x80530009 (NS_ERROR_DOM_NOT_SUPPORTED_ERR)"  location
: "chrome://mochitests/content/a11y/accessible/test_nsIAccessNode_utils.html Line: 19"] at :0
Comment 19 Trevor Saunders (:tbsaunde) 2012-01-13 09:59:22 PST
> nit: a lot of comments exists where you've simply removed the reference to
> nsIAccessNode, but the comment could be further reformatted to line up
> better.

that has the slight cost of more stuff to dig through when going through history, but sure

> Looking at this patch makes me wish we had about:memory implementation.

why? to gauge memory effect I believe all you need is the number of accessibles * sizeof(void*)

but as I think I've mentioned before I sort of think the more important effect here would be the effects on caching.
Comment 20 David Bolter [:davidb] 2012-01-13 12:16:14 PST
Comment on attachment 587334 [details] [diff] [review]
patch

Review of attachment 587334 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with nits fixed, tests failures fixed, and with Alexander's review since I'm not our ISimpleDOM guy and I'm not sure if I'm missing something WRT removing nsIAccessNode.
Comment 21 alexander :surkov 2012-01-19 03:57:25 PST
Why do we need to have dupe code like nsAccessNodeWrap::get_innerHTML and nsAccessible::GetInnerHTML?
Comment 22 Trevor Saunders (:tbsaunde) 2012-01-26 18:07:00 PST
(In reply to alexander :surkov from comment #21)
> Why do we need to have dupe code like nsAccessNodeWrap::get_innerHTML and
> nsAccessible::GetInnerHTML?

you mean those silly wrappers? because of linker errors, NS_DECL_NSIACCESSIBLE declares those virtual funcs on nsAccessible, and there needs to be some impl on nsAccessnode or somewhere for ISimpleDOMNode to use.
Comment 23 Trevor Saunders (:tbsaunde) 2012-01-26 21:08:30 PST
Created attachment 592051 [details] [diff] [review]
patch
Comment 24 alexander :surkov 2012-01-26 22:45:07 PST
Comment on attachment 592051 [details] [diff] [review]
patch

Review of attachment 592051 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: accessible/public/nsIAccessible.idl
@@ +103,5 @@
>     */
>    readonly attribute long indexInParent;
>  
>    /**
> +   * The innerHTML for the DOM node

The innerHTML for the HTML element associated with this accessible if applicable.

@@ +113,5 @@
> +  /**
> +   * Retrieve the computed style value for this DOM node, if it is a DOM element.
> +   * Note: the meanings of width, height and other size measurements depend
> +   * on the version of CSS being used. Therefore, for bounds information, 
> +   * it is better to use nsIAccessible::accGetBounds.

new line please containing '*'

@@ +117,5 @@
> +   * it is better to use nsIAccessible::accGetBounds.
> +   * @param pseudoElt The pseudo element to retrieve style for, or NULL
> +   *                  for general computed style information for this node.
> +   * @param propertyName Retrieve the computed style value for this property name,
> +   *                     for example "border-bottom".

nit:

@param pseudoElm [in] the pseude elemnt to retrive style for, or NULL
                   for general computed ....

and below in this file

@@ +351,5 @@
> +   *                         constants refer to nsIAccessibleCoordinateType)
> +   * @param aX - defines the x coordinate
> +   * @param aY - defines the y coordinate
> +  */
> +  void scrollToPoint(in unsigned long aCoordinateType, in long aX, in long aY);

it appears we shouldn't use 'a'prefix for arguments in idl files, at least you should be consistent for methods you add here

::: accessible/src/base/nsAccessNode.h
@@ +177,5 @@
>    static nsIStringBundle* GetStringBundle()
>      { return gStringBundle; }
>  
> +  /**
> +   * interface methods on nsIAccessible shared with ISimpleDOM

interface -> Interface, dot in the end

@@ +180,5 @@
> +  /**
> +   * interface methods on nsIAccessible shared with ISimpleDOM
> +   */
> +  NS_IMETHOD GetLanguage(nsAString& aLocale);
> +  NS_IMETHOD ScrollTo(PRUint32 aType);

you don't need to keep them in XPCOM style, something like
void GetLanguage(nsAString) should work

::: accessible/src/base/nsAccessible.h
@@ +140,5 @@
> +    nsIDOMNode *DOMNode = nsnull;
> +    if (GetNode())
> +      CallQueryInterface(GetNode(), &DOMNode);
> +    return DOMNode;
> +  }

I'd say to remove it entirely. In few cases we have you can do
nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(accessible->GetNode()));

::: accessible/src/msaa/CAccessibleComponent.cpp
@@ +154,5 @@
>  {
>  __try {
>    *aColorValue = 0;
>  
> +  nsCOMPtr<nsIAccessible> acc(do_QueryObject(this));

Use nsAccessible instead that'll be helpful for dexpcoming

::: accessible/src/xul/nsXULFormControlAccessible.cpp
@@ +662,5 @@
>  bool
>  nsXULToolbarButtonAccessible::IsSeparator(nsAccessible *aAccessible)
>  {
> +  nsINode* node = aAccessible->GetNode();
> +  nsCOMPtr<nsIContent> content(do_QueryInterface(node));

here you safely can do
nsIContent* content = aAccessible->GetContent();
because the result of code below doesn't depend on document accessible where GetContnet() is not equivalent.

@@ +669,5 @@
>      return false;
>  
> +  return (content->Tag() == nsGkAtoms::toolbarseparator) ||
> +         (content->Tag() == nsGkAtoms::toolbarspacer) ||
> +         (content->Tag() == nsGkAtoms::toolbarspring);

you can do
return content & (content->Tag() == bla ||
if you like

::: accessible/tests/mochitest/hypertext/test_update.html
@@ +59,5 @@
>      function updateText(aContainerID)
>      {
>        this.containerNode = getNode(aContainerID);
>        this.container = getAccessible(this.containerNode, nsIAccessibleHyperText);
> +      this.text = this.container.firstChild.QueryInterface(nsIAccessible);

firstChild is already nsIAccessible. You don't need to query it

::: accessible/tests/mochitest/test_nsIAccessNode_utils.html
@@ +14,5 @@
>    <script type="application/javascript">
>      function doTest()
>      {
>        var elmObj = {};
> +      var acc = getAccessible("span", false, elmObj);

use null instead of false
Comment 25 Trevor Saunders (:tbsaunde) 2012-02-01 23:13:26 PST
Created attachment 593736 [details] [diff] [review]
patch
Comment 26 Trevor Saunders (:tbsaunde) 2012-02-02 00:27:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9813b26dacb2
Comment 27 Trevor Saunders (:tbsaunde) 2012-02-02 01:41:23 PST
backed out for burning windows :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf5f720c3b1a
Comment 29 Trevor Saunders (:tbsaunde) 2012-02-05 20:05:51 PST
bug 698823 added a QI to nsIAccessNode in js and somehow  that caused the world to completely explode.
I removed that QI and pushed https://tbpl.mozilla.org/?tree=Try&rev=edf4cbef924b  which looks good :)
Comment 30 David Bolter [:davidb] 2012-02-06 07:00:06 PST
Where does that leave us?
Comment 31 Trevor Saunders (:tbsaunde) 2012-02-06 07:50:32 PST
(In reply to David Bolter [:davidb] from comment #30)
> Where does that leave us?

I removed it locally since this bug makes it nolonger needed and I'll push it again soon when I get a chance.
Comment 32 Trevor Saunders (:tbsaunde) 2012-02-07 05:21:03 PST
I pushed what just went to try as http://hg.mozilla.org/integration/mozilla-inbound/rev/c2a35fcfe371
Comment 33 Marco Bonardo [::mak] 2012-02-07 15:05:05 PST
https://hg.mozilla.org/mozilla-central/rev/c2a35fcfe371
Comment 34 alexander :surkov 2012-02-07 15:22:46 PST
It seems you introduced not-used nsAccessNode::GetStringBundle what is merging issue.
Comment 35 alexander :surkov 2012-02-07 15:24:44 PST
(In reply to alexander :surkov from comment #24)

> ::: accessible/src/base/nsAccessible.h
> @@ +140,5 @@
> > +    nsIDOMNode *DOMNode = nsnull;
> > +    if (GetNode())
> > +      CallQueryInterface(GetNode(), &DOMNode);
> > +    return DOMNode;
> > +  }
> 
> I'd say to remove it entirely. In few cases we have you can do
> nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(accessible->GetNode()));

it seems this comment wasn't addressed
Comment 36 Trevor Saunders (:tbsaunde) 2012-02-10 12:20:31 PST
Created attachment 596143 [details] [diff] [review]
bug 672507 - fix review nit
Comment 37 Ed Morley [:emorley] 2012-02-10 19:18:45 PST
https://hg.mozilla.org/mozilla-central/rev/cb4532333feb
Comment 38 David Bolter [:davidb] 2012-05-10 11:21:00 PDT
Jorge are we hurting anyone?
Comment 39 Jorge Villalobos [:jorgev] 2012-05-10 12:07:40 PDT
I noticed a few add-ons accessing this interface, but it doesn't appear to be a big deal. However, all interface removals or modifications should be flagged for addon-compat, since most interfaces are accessed by some add-on.
Comment 40 alexander :surkov 2012-05-10 20:19:47 PDT
Ok, we will do that. We change our interfaces often enough. Jorge is there a way to list all add-ons using accessibility interfaces (located in accessible/public folder)? It must be interesting to know what they do and what for.
Comment 41 Jorge Villalobos [:jorgev] 2012-05-11 08:11:42 PDT
We have a private instance of MXR that indexes add-on code from AMO. If you have an LDAP account, you can use it.

https://mxr.mozilla.org/addons/
Comment 42 alexander :surkov 2012-05-11 08:18:47 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #41)
> We have a private instance of MXR that indexes add-on code from AMO. If you
> have an LDAP account, you can use it.
> 
> https://mxr.mozilla.org/addons/

cool, thank you

Note You need to log in before you can comment on or make changes to this bug.