add set of internal accessible relation types

RESOLVED FIXED in mozilla27

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: tbsaunde, Assigned: surkov)

Tracking

unspecified
mozilla27
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
Can I work on this?
(Assignee)

Comment 2

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

sure
Assignee: nobody → sukun007

Comment 3

6 years ago
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

6 years ago
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.
(Reporter)

Comment 5

6 years ago
(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

5 years ago
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...
(Reporter)

Comment 7

5 years ago
(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

5 years ago
ok, actually someone in the irc channel told me that it was abandoned so i could work on it.. thanks anyways!
(Assignee)

Comment 9

5 years ago
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.
Assignee: sukun007 → nobody

Comment 10

5 years ago
so can i work on this ?
Yes I think so. Hopefully we can find Sukun another interesting bug if he resurfaces.
Assignee: nobody → nishantg2108

Comment 12

5 years ago
Can you confirm that you're still working on this bug?
Flags: needinfo?(nishantg2108)
(Assignee)

Comment 13

5 years ago
Created attachment 812378 [details] [diff] [review]
patch
Assignee: nishantg2108 → surkov.alexander
Attachment #812378 - Flags: review?(trev.saunders)
(Assignee)

Comment 14

5 years ago
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)
Flags: needinfo?(nishantg2108)
(Reporter)

Comment 15

4 years ago
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.
(Assignee)

Comment 16

4 years ago
(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.
(Reporter)

Comment 17

4 years ago
(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?
(Assignee)

Comment 18

4 years ago
(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?
(Reporter)

Comment 19

4 years ago
(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.
(Assignee)

Comment 20

4 years ago
(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?
(Reporter)

Comment 21

4 years ago
> > 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.
(Assignee)

Comment 22

4 years ago
(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?
(Reporter)

Comment 23

4 years ago
(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?
(Assignee)

Comment 24

4 years ago
(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.
(Reporter)

Comment 25

4 years ago
> 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?
(Assignee)

Comment 26

4 years ago
(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.
(Reporter)

Comment 27

4 years ago
(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 ;)
(Assignee)

Comment 28

4 years ago
(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
(Assignee)

Comment 29

4 years ago
Created attachment 818716 [details] [diff] [review]
patch2
Attachment #812378 - Attachment is obsolete: true
Attachment #812378 - Flags: review?(trev.saunders)
Attachment #818716 - Flags: review?(trev.saunders)
(Reporter)

Comment 30

4 years ago
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?
Attachment #818716 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 31

4 years ago
(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
(Reporter)

Comment 32

4 years ago
(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.
(Assignee)

Comment 33

4 years ago
(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?
(Assignee)

Comment 34

4 years ago
Created attachment 819037 [details] [diff] [review]
patch3

latest patch
Attachment #818716 - Attachment is obsolete: true
(Assignee)

Comment 35

4 years ago
Comment on attachment 819037 [details] [diff] [review]
patch3

Trev, anything to fix before I land it?
(Reporter)

Comment 36

4 years ago
(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
(Reporter)

Comment 37

4 years ago
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?
(Assignee)

Comment 38

4 years ago
(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
(Reporter)

Comment 39

4 years ago
(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.
Depends on: 928674
https://hg.mozilla.org/mozilla-central/rev/e8f7b915ad46
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.