Closed
Bug 672507
Opened 13 years ago
Closed 13 years ago
merge nsIAccessNode and nsIAccessible
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: surkov, Assigned: tbsaunde)
References
(Blocks 1 open bug)
Details
(Keywords: access, addon-compat)
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
move GetRootAccessible() to nsIAccessible and since nobody needs nsAccessNode to have the nonxpcom version move it to nsAccessible
Attachment #548641 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 2•13 years ago
|
||
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.
Attachment #548641 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 3•13 years ago
|
||
>
> ::: 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
Reporter | ||
Comment 4•13 years ago
|
||
(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.
Assignee | ||
Comment 5•13 years ago
|
||
(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.
Reporter | ||
Comment 6•13 years ago
|
||
(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.
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → trev.saunders
Assignee | ||
Comment 7•13 years ago
|
||
(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.
Reporter | ||
Comment 8•13 years ago
|
||
(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.
Assignee | ||
Comment 9•13 years ago
|
||
(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?
Reporter | ||
Comment 10•13 years ago
|
||
(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.
Assignee | ||
Comment 11•13 years ago
|
||
(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.
Reporter | ||
Comment 12•13 years ago
|
||
(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.
Assignee | ||
Comment 13•13 years ago
|
||
(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.
Reporter | ||
Comment 14•13 years ago
|
||
(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.
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #587334 -
Flags: review?(bolterbugz)
Comment 16•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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
Assignee | ||
Comment 19•13 years ago
|
||
> 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•13 years ago
|
||
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.
Attachment #587334 -
Flags: review?(bolterbugz) → review+
Reporter | ||
Comment 21•13 years ago
|
||
Why do we need to have dupe code like nsAccessNodeWrap::get_innerHTML and nsAccessible::GetInnerHTML?
Assignee | ||
Comment 22•13 years ago
|
||
(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.
Assignee | ||
Comment 23•13 years ago
|
||
Attachment #592051 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 24•13 years ago
|
||
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
Attachment #592051 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 25•13 years ago
|
||
Attachment #548641 -
Attachment is obsolete: true
Attachment #587334 -
Attachment is obsolete: true
Attachment #592051 -
Attachment is obsolete: true
Assignee | ||
Comment 26•13 years ago
|
||
Assignee | ||
Comment 27•13 years ago
|
||
backed out for burning windows :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf5f720c3b1a
Comment 28•13 years ago
|
||
This was pushed to inbound again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ca407ff5564
But then backed out again, because it turned the a11y tests orange by the thousand:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0229f4c8282
Example logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=9057277&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=9056774&tree=Mozilla-Inbound
etc.
Assignee | ||
Comment 29•13 years ago
|
||
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•13 years ago
|
||
Where does that leave us?
Assignee | ||
Comment 31•13 years ago
|
||
(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.
Assignee | ||
Comment 32•13 years ago
|
||
I pushed what just went to try as http://hg.mozilla.org/integration/mozilla-inbound/rev/c2a35fcfe371
Comment 33•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Reporter | ||
Comment 34•13 years ago
|
||
It seems you introduced not-used nsAccessNode::GetStringBundle what is merging issue.
Reporter | ||
Comment 35•13 years ago
|
||
(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
Assignee | ||
Comment 36•13 years ago
|
||
Attachment #596143 -
Flags: review?(bolterbugz)
Updated•13 years ago
|
Attachment #596143 -
Flags: review?(bolterbugz) → review+
Comment 37•13 years ago
|
||
Updated•13 years ago
|
Keywords: addon-compat
Comment 38•13 years ago
|
||
Jorge are we hurting anyone?
Comment 39•13 years ago
|
||
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.
Reporter | ||
Comment 40•13 years ago
|
||
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•13 years ago
|
||
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/
Reporter | ||
Comment 42•13 years ago
|
||
(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
You need to log in
before you can comment on or make changes to this bug.
Description
•