748639 - add set of internal accessible relation types

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

1. add enum RelationType in namespace mozilla::a11y::relation containing each type in accessible/public/nsIAccessibleRelation.idl
2. add typedef RelationType to relation::RelationType in namespace mozilla::a11y
3. change nsAccessible::RelationByType() and over rides to take a RelationType and update constants to use the one in relation::

Comment 1

[Sukun Tarachandani]

Can I work on this?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Sukun Tarachandani from comment #1)
> Can I work on this?

sure

Comment 3

[Sukun Tarachandani]

I got what I have to do. I got the types in the nsiAccessibleRelation.idl file. But I cannot find the relation namespace. Any pointers to get started.

Comment 4

[Sukun Tarachandani]

The enum here is to provide easy access to all the types in nsIAccessibleRelation interface. So, I think the namespace should go in the same file.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Sukun Tarachandani from comment #4)
> The enum here is to provide easy access to all the types in
> nsIAccessibleRelation interface. So, I think the namespace should go in the
> same file.

no, that file is only for stuff we expose to js, while you could theoretically do that with an xpidl %c++ { } block adding it to accessible/src/base/Relation.h would be nicer.

Comment 6

[Nishant Gupta]

Hi, I am interested to work on this although i am very new to this and haven't contributed to the code before..i might need some more guidance but will be very eager to work on this to get started...

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to Nishant Gupta from comment #6)
> Hi, I am interested to work on this although i am very new to this and
> haven't contributed to the code before..i might need some more guidance but
> will be very eager to work on this to get started...

Sorry, but someone else just asked to work on it (see above comments).  Feel free to find other bugs though :0

Comment 8

[Nishant Gupta]

ok, actually someone in the irc channel told me that it was abandoned so i could work on it.. thanks anyways!

Comment 9

[alexander :surkov (:asurkov)]

That bug was inactive for months. I think we can get back this bug into the pull if Sukun doesn't have plans in foreseeable future to continue the work.

Comment 10

[Nishant Gupta]

so can i work on this ?

Comment 11

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

Yes I think so. Hopefully we can find Sukun another interesting bug if he resurfaces.

Comment 12

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 14

[alexander :surkov (:asurkov)]

Nishant, if you need a bug please let me know, I'll try to find something interesting (I'm stealing this one because of inactivity)

Comment 15

[Trevor Saunders (:tbsaunde)]

I'd really prefer you got rid of rels:: and used RelationType instead of RelType.  I think the somewhat stronger type safety of enum class from TypedEnum.h would be pretty nice and simpler than the namespace hackery.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #15)
> I'd really prefer you got rid of rels:: and used RelationType instead of
> RelType.  I think the somewhat stronger type safety of enum class from
> TypedEnum.h would be pretty nice and simpler than the namespace hackery.

right, we chatted about this on irc. I wanted to be consistent as much as we can. If we go with TypedEnum.h then it'd be 3d style of how we deal with constants (two existing ones are XPCOM and namespaces). Also I'm not sure I clearly can see how that approach benefits over namespace one. If you want then I can file a follow up on this but I'd prefer to keep the taken approach here.

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #16)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> > I'd really prefer you got rid of rels:: and used RelationType instead of
> > RelType.  I think the somewhat stronger type safety of enum class from
> > TypedEnum.h would be pretty nice and simpler than the namespace hackery.
> 
> right, we chatted about this on irc. I wanted to be consistent as much as we
> can. If we go with TypedEnum.h then it'd be 3d style of how we deal with
> constants (two existing ones are XPCOM and namespaces). Also I'm not sure I
> clearly can see how that approach benefits over namespace one. If you want

the typedef thing seems pretty hacky, and atleast with roles meant there was some places name lookup stuff had to be hacked around with a11y:: qualification and other weird things.

I'd really rather not introduce more weird stuff that needs to be fixed just because it  would be slightly inconsistant to do it right the first time.

besides I don't think there's much common use of the xpidl constants left.

> then I can file a follow up on this but I'd prefer to keep the taken
> approach here.

why not do it right and then file a bug to fix the other things?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)

> the typedef thing seems pretty hacky, and atleast with roles meant there was
> some places name lookup stuff had to be hacked around with a11y::
> qualification and other weird things.

we have role craziness because we have Role type and Role method, role type and role variable name

> besides I don't think there's much common use of the xpidl constants left.

events

> why not do it right and then file a bug to fix the other things?

because it's not evident with me it's a right thing but what we have is weird and hacky. It looks for me as one approach vs alternative but equivalent another one. Is it really weird and hacky so it makes a big deal?

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> 
> > the typedef thing seems pretty hacky, and atleast with roles meant there was
> > some places name lookup stuff had to be hacked around with a11y::
> > qualification and other weird things.
> 
> we have role craziness because we have Role type and Role method, role type
> and role variable name

I guess that's fair, but Role x = Role::FOO; feels more natural to me than Role x = roles::FOO

> > besides I don't think there's much common use of the xpidl constants left.
> 
> events

I guess

> > why not do it right and then file a bug to fix the other things?
> 
> because it's not evident with me it's a right thing but what we have is
> weird and hacky. It looks for me as one approach vs alternative but
> equivalent another one. Is it really weird and hacky so it makes a big deal?

well the goal is to have enum whose elements are "namespaced" and ideally only an integer type as a implementation detail, which is what enum class exists to do.  So I'd say what we do is kind of hacky and weird.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #19)

> I guess that's fair, but Role x = Role::FOO; feels more natural to me than
> Role x = roles::FOO

it seems that's where our disagreement starts
 
> well the goal is to have enum whose elements are "namespaced" and ideally
> only an integer type as a implementation detail

yeah if the goal is to deliver that feeling from above :) While I can live with Role::FOO (vs roles::FOO) style and actually I don't care much if you or somebody takes responsibility to change that but I'm not a big fan to make the relations part by myself. It's not simple replacing since I'd need some merging with other patches in my queue, I need to fix indentations because of renaming. So I wouldn't jump into this work until what we do is a really bad practice. Would you prefer to have rels namespace over continue to deal with XPCOM constants, at least, because rels namespace is one step closer to what you want?

Comment 21

[Trevor Saunders (:tbsaunde)]

> > well the goal is to have enum whose elements are "namespaced" and ideally
> > only an integer type as a implementation detail
> 
> yeah if the goal is to deliver that feeling from above :) While I can live
> with Role::FOO (vs roles::FOO) style and actually I don't care much if you
> or somebody takes responsibility to change that but I'm not a big fan to
> make the relations part by myself. It's not simple replacing since I'd need
> some merging with other patches in my queue, I need to fix indentations
> because of renaming. So I wouldn't jump into this work until what we do is a

why can't you just run sed on .hg/patches/*? after the ability to do that is about the only good thing about mqueue

> really bad practice. Would you prefer to have rels namespace over continue
> to deal with XPCOM constants, at least, because rels namespace is one step
> closer to what you want?

relations namespace maybe, but xpcom constants don't really bother me, there just something that should be fixed some day.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #21)

> why can't you just run sed on .hg/patches/*? after the ability to do that is
> about the only good thing about mqueue

indentations won't be magically fixed

> > really bad practice. Would you prefer to have rels namespace over continue
> > to deal with XPCOM constants, at least, because rels namespace is one step
> > closer to what you want?
> 
> relations namespace maybe, but xpcom constants don't really bother me, there
> just something that should be fixed some day.

1) I prefer to have internal set of relation constants over XPCOM constants usage
2) I don't really care whether it'd be rels::RELATION or RelType::RELATION, would it be a namespace or TypedEnum
3) I don't what to make renaming until keeping constants under namespace is considered as a bad practice or TypedEnum is required by Mozilla code style because of 2) and I bet it will require manual changes in patches I have.

so where are we?

Comment 23

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #22)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> 
> > why can't you just run sed on .hg/patches/*? after the ability to do that is
> > about the only good thing about mqueue
> 
> indentations won't be magically fixed

I don't think there are any that will need to change?

> > > really bad practice. Would you prefer to have rels namespace over continue
> > > to deal with XPCOM constants, at least, because rels namespace is one step
> > > closer to what you want?
> > 
> > relations namespace maybe, but xpcom constants don't really bother me, there
> > just something that should be fixed some day.
> 
> 1) I prefer to have internal set of relation constants over XPCOM constants
> usage

I can't say that I care a whole lot about this.

> 2) I don't really care whether it'd be rels::RELATION or RelType::RELATION,
> would it be a namespace or TypedEnum
> 3) I don't what to make renaming until keeping constants under namespace is
> considered as a bad practice or TypedEnum is required by Mozilla code style
> because of 2) and I bet it will require manual changes in patches I have.

maybe, but I doubt just doing it would take longer than we've spent talking about it... :/

> so where are we?

this patch might be ok if the namespace was something other than rels, but I wouldn't see much point in bother to write it either.  If we're going to change things why not make them actually nice or not bother change them at all?

Comment 24

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #23)

> > indentations won't be magically fixed
> 
> I don't think there are any that will need to change?

the whole point of my statement is any rename requires you to be careful, reread the patch and make necessary fixes, it's never simple as one typed command in shell.

> > 2) I don't really care whether it'd be rels::RELATION or RelType::RELATION,
> > would it be a namespace or TypedEnum
> > 3) I don't what to make renaming until keeping constants under namespace is
> > considered as a bad practice or TypedEnum is required by Mozilla code style
> > because of 2) and I bet it will require manual changes in patches I have.
> 
> maybe, but I doubt just doing it would take longer than we've spent talking
> about it... :/

exactly, a dual statement is also true.

> > so where are we?
> 
> this patch might be ok if the namespace was something other than rels, 

if you are sure then you dislike rels and we need to have something nicer then please suggest a name. Would it be RelType of TypedEnum approach?

> but I
> wouldn't see much point in bother to write it either.  If we're going to
> change things why not make them actually nice or not bother change them at
> all?

I'm all up for this, we just are not on the same page what nice is.

Comment 25

[Trevor Saunders (:tbsaunde)]

> if you are sure then you dislike rels and we need to have something nicer
> then please suggest a name. Would it be RelType of TypedEnum approach?

I'd use RelationType::LABELED_BY RelationType::DESCRIPTION_FOR etc if it were me

> > but I
> > wouldn't see much point in bother to write it either.  If we're going to
> > change things why not make them actually nice or not bother change them at
> > all?
> 
> I'm all up for this, we just are not on the same page what nice is.

so, do you have a reason why using a namespace is actually a good approach, not just less work or more consistant with other code that doesn't take a great approach?

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #25)
> > if you are sure then you dislike rels and we need to have something nicer
> > then please suggest a name. Would it be RelType of TypedEnum approach?
> 
> I'd use RelationType::LABELED_BY RelationType::DESCRIPTION_FOR etc if it
> were me

won't we have same problems as with roles since Accessible have RelationType() method?

> > > but I
> > > wouldn't see much point in bother to write it either.  If we're going to
> > > change things why not make them actually nice or not bother change them at
> > > all?
> > 
> > I'm all up for this, we just are not on the same page what nice is.
> 
> so, do you have a reason why using a namespace is actually a good approach,
> not just less work or more consistant with other code that doesn't take a
> great approach?

no, I don't really care what approach is in sue. If I will do renaming then I can switch to TypedEnum.

Comment 27

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #26)
> (In reply to Trevor Saunders (:tbsaunde) from comment #25)
> > > if you are sure then you dislike rels and we need to have something nicer
> > > then please suggest a name. Would it be RelType of TypedEnum approach?
> > 
> > I'd use RelationType::LABELED_BY RelationType::DESCRIPTION_FOR etc if it
> > were me
> 
> won't we have same problems as with roles since Accessible have
> RelationType() method?

no, its RelationByType() isn't it?

> > > > but I
> > > > wouldn't see much point in bother to write it either.  If we're going to
> > > > change things why not make them actually nice or not bother change them at
> > > > all?
> > > 
> > > I'm all up for this, we just are not on the same page what nice is.
> > 
> > so, do you have a reason why using a namespace is actually a good approach,
> > not just less work or more consistant with other code that doesn't take a
> > great approach?
> 
> no, I don't really care what approach is in sue. If I will do renaming then
> I can switch to TypedEnum.

ok, it would make me happy if nothing else ;)

Comment 28

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #27)

> > won't we have same problems as with roles since Accessible have
> > RelationType() method?
> 
> no, its RelationByType() isn't it?

ah, right

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 30

[Trevor Saunders (:tbsaunde)]

Comment on attachment 818716 [details] [diff] [review]
patch2

>   // Keep in sync with AtkRelationType enum.
>-  static const uint32_t relationTypes[] = {

any reason we can't have static asserts?

>+namespace mozilla {
>+namespace a11y {
>+
>+MOZ_BEGIN_ENUM_CLASS(RelationType, int)

why do you want to specify int? it seems like not giving a type would be better here

>+Accessible::GetRelationByType(uint32_t aType, nsIAccessibleRelation** aRelation)
> {
>   NS_ENSURE_ARG_POINTER(aRelation);
>   *aRelation = nullptr;
>   if (IsDefunct())
>     return NS_ERROR_FAILURE;
> 
>-  Relation rel = RelationByType(aType);
>+  Relation rel = RelationByType(static_cast<RelationType>(aType));

it might be good to check the bounds first in case somebody passes in garbage and the compiler optimizes assuming the value is an enum element.

>@@ -68,62 +68,62 @@ ia2AccessibleRelation::get_relationType(
>   A11Y_TRYBLOCK_BEGIN
> 
>   if (!aRelationType)
>     return E_INVALIDARG;
> 
>   *aRelationType = nullptr;
> 
>   switch (mType) {
>     default:
>       return E_FAIL;

is this still needed?

>+++ b/accessible/src/windows/ia2/ia2AccessibleRelation.h
>@@ -60,32 +60,32 @@ private:
>   nsTArray<nsRefPtr<Accessible> > mTargets;

I thought you were going to make the type of mType RelationType?

>   ULONG mReferences;

btw should we use the auto ref thingy here?

>   if (xpRelation >= 0) {
>-    Relation rel = RelationByType(xpRelation);
>+    Relation rel = RelationByType(static_cast<RelationType>(xpRelation));

I'm not exactly sure how to handle the > 0 thing, but wouldn't it be nicer to change the type of xpRelation?

Comment 31

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #30)
> Comment on attachment 818716 [details] [diff] [review]
> patch2
> 
> >   // Keep in sync with AtkRelationType enum.
> >-  static const uint32_t relationTypes[] = {
> 
> any reason we can't have static asserts?

bug 923289 will introduce relations map, so it won't be necessary

> >-  Relation rel = RelationByType(aType);
> >+  Relation rel = RelationByType(static_cast<RelationType>(aType));
> 
> it might be good to check the bounds first in case somebody passes in
> garbage and the compiler optimizes assuming the value is an enum element.

how to do that? what result of static cast will be in that case?

> 
> >+++ b/accessible/src/windows/ia2/ia2AccessibleRelation.h
> >@@ -60,32 +60,32 @@ private:
> >   nsTArray<nsRefPtr<Accessible> > mTargets;
> 
> I thought you were going to make the type of mType RelationType?

missed it in a long sequence of comments, sorry

> >   ULONG mReferences;
> 
> btw should we use the auto ref thingy here?

I can do IUnknown macros here

> >   if (xpRelation >= 0) {
> >-    Relation rel = RelationByType(xpRelation);
> >+    Relation rel = RelationByType(static_cast<RelationType>(xpRelation));
> 
> I'm not exactly sure how to handle the > 0 thing, but wouldn't it be nicer
> to change the type of xpRelation?

no ideas, probably reintroduce NULL_RELATION

Comment 32

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #31)
> (In reply to Trevor Saunders (:tbsaunde) from comment #30)
> > Comment on attachment 818716 [details] [diff] [review]
> > patch2
> > 
> > >   // Keep in sync with AtkRelationType enum.
> > >-  static const uint32_t relationTypes[] = {
> > 
> > any reason we can't have static asserts?
> 
> bug 923289 will introduce relations map, so it won't be necessary

forgot about that :)

> > >-  Relation rel = RelationByType(aType);
> > >+  Relation rel = RelationByType(static_cast<RelationType>(aType));
> > 
> > it might be good to check the bounds first in case somebody passes in
> > garbage and the compiler optimizes assuming the value is an enum element.
> 
> how to do that? what result of static cast will be in that case?

if (aType < 0 || aType > static_cast<uint32_t>(RelationType::LAST))
  return

I don't think the static cast needs to change?

> > >   ULONG mReferences;
> > 
> > btw should we use the auto ref thingy here?
> 
> I can do IUnknown macros here

no need to do it here, just figured its worth mentioning

> > >   if (xpRelation >= 0) {
> > >-    Relation rel = RelationByType(xpRelation);
> > >+    Relation rel = RelationByType(static_cast<RelationType>(xpRelation));
> > 
> > I'm not exactly sure how to handle the > 0 thing, but wouldn't it be nicer
> > to change the type of xpRelation?
> 
> no ideas, probably reintroduce NULL_RELATION

or we could reorganize the function to not need the if (take other cases first) and then not have the problem.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #32)

> if (aType < 0 || aType > static_cast<uint32_t>(RelationType::LAST))
>   return

then LAST needs to be introduced and likely it should be introduced in XPCOM interface what I don't really like. It seems like we should end up with empty relation what is also ok.

> > I can do IUnknown macros here
> 
> no need to do it here, just figured its worth mentioning

I did it already

> > no ideas, probably reintroduce NULL_RELATION
> 
> or we could reorganize the function to not need the if (take other cases
> first) and then not have the problem.

we could, new bug or?

Comment 34

[alexander :surkov (:asurkov)]

Attachment: patch3

latest patch

Comment 35

[alexander :surkov (:asurkov)]

Comment on attachment 819037 [details] [diff] [review]
patch3

Trev, anything to fix before I land it?

Comment 36

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #33)
> (In reply to Trevor Saunders (:tbsaunde) from comment #32)
> 
> > if (aType < 0 || aType > static_cast<uint32_t>(RelationType::LAST))
> >   return
> 
> then LAST needs to be introduced and likely it should be introduced in XPCOM

why? just keep it out of map.

> interface what I don't really like. It seems like we should end up with
> empty relation what is also ok.

not sure what you mean here

> > 
> > or we could reorganize the function to not need the if (take other cases
> > first) and then not have the problem.
> 
> we could, new bug or?

sure

Comment 37

[Trevor Saunders (:tbsaunde)]

Comment on attachment 819037 [details] [diff] [review]
patch3

> class ia2AccessibleRelation : public IAccessibleRelation
> {
> public:
>-  ia2AccessibleRelation(uint32_t aType, Relation* aRel);
>+  ia2AccessibleRelation(RelationType aType, Relation* aRel);
>   virtual ~ia2AccessibleRelation() { }

make it final and kill the dtor?

Comment 38

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #36)
> (In reply to alexander :surkov from comment #33)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #32)
> > 
> > > if (aType < 0 || aType > static_cast<uint32_t>(RelationType::LAST))
> > >   return
> > 
> > then LAST needs to be introduced and likely it should be introduced in XPCOM
> 
> why? just keep it out of map.

it will be out of RelationType namespace then, just RELATION_TYPE_LAST constant?

> > interface what I don't really like. It seems like we should end up with
> > empty relation what is also ok.
> 
> not sure what you mean here

I meant the method should return Relation object with no targets

Comment 39

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #38)
> (In reply to Trevor Saunders (:tbsaunde) from comment #36)
> > (In reply to alexander :surkov from comment #33)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #32)
> > > 
> > > > if (aType < 0 || aType > static_cast<uint32_t>(RelationType::LAST))
> > > >   return
> > > 
> > > then LAST needs to be introduced and likely it should be introduced in XPCOM
> > 
> > why? just keep it out of map.
> 
> it will be out of RelationType namespace then, just RELATION_TYPE_LAST
> constant?

no, I'd make it be RelationType::LAST, I just wouldn't have a macro for it probably.

> > > interface what I don't really like. It seems like we should end up with
> > > empty relation what is also ok.
> > 
> > not sure what you mean here
> 
> I meant the method should return Relation object with no targets

I don't know that's true, I think the compiler is free to optimize on the assumption that the value being switched on is one of the enums elements, so if it isn't its possible something unhappy can happen.

Comment 40

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e8f7b915ad46

Comment 41

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/e8f7b915ad46