move nsARIAMap::UniversalStatesFor to aria namespace

RESOLVED FIXED in mozilla16

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
1) introduce mozilla::a11y::aria namespace in nsARIAMap.h (if it's still missed)
2) move nsARIAMap::UniversalStatesFor there
3) remove nsARIAMap::gWAIUnivStateMap and keep it as static in nsARIAMap.cpp
(Reporter)

Updated

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

Comment 1

5 years ago
Trevor, that makes us to not keep UniversalStatesFor inline anymore. Also following this we should replace gEmptyRoleMap on something like aria::EmptyRole() which is not inline too. I wonder how we can workaround that.
(In reply to alexander :surkov from comment #1)
> Trevor, that makes us to not keep UniversalStatesFor inline anymore. Also

I have no guess what that'll do to perf, but I suspect its not critical.  Otherwise why is this interesting?

> following this we should replace gEmptyRoleMap on something like
> aria::EmptyRole() which is not inline too. I wonder how we can workaround
> that.

why can't we have 

namespace aria {
nsRoleMap* gEmptyRole;
}

then in nsARIAMap.cpp

nsRoleMap sEmptyMappingRule {
...
};

gEmptyRole = &sEmptyRule;
> > following this we should replace gEmptyRoleMap on something like
> > aria::EmptyRole() which is not inline too. I wonder how we can workaround
> > that.
> 
> why can't we have 
> 
> namespace aria {
> nsRoleMap* gEmptyRole;
> }
> 
> then in nsARIAMap.cpp
> 
> nsRoleMap sEmptyMappingRule {
> ...
> };
> 
> gEmptyRole = &sEmptyRule;

on the other hand it would be nice if we could store an index in accessibles instead of a pointer to use less space for the same amount of information.
(Reporter)

Comment 4

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> why can't we have 
> 
> namespace aria {
> nsRoleMap* gEmptyRole;
> }

to keep it unified like
namespace aria {
  RoleMap* RoleMapFor(nsINode* aNode);
  RoleMap* EmptyRoleMap();
}

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> on the other hand it would be nice if we could store an index in accessibles
> instead of a pointer to use less space for the same amount of information.

well, we could do that but also that means we need to get RoleMap every time what sound not so nice as having a pointer.
(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > why can't we have 
> > 
> > namespace aria {
> > nsRoleMap* gEmptyRole;
> > }
> 
> to keep it unified like
> namespace aria {
>   RoleMap* RoleMapFor(nsINode* aNode);
>   RoleMap* EmptyRoleMap();

I guess that's a fair point though my feeling is sort of that's a bit far to the everything must be uniform side.

> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > on the other hand it would be nice if we could store an index in accessibles
> > instead of a pointer to use less space for the same amount of information.
> 
> well, we could do that but also that means we need to get RoleMap every time
> what sound not so nice as having a pointer.

Well, I'm not sure I understand this we either have null or we have a pointer, similarly we could have a special none number or an actual index.  After all in a sense all a pointer is an index :)
Alex, it occurs to me it might be more reasonable to move this to ARIAStateMap.cpp while we're here to keep the aria state stuff all in one place.
(Reporter)

Comment 7

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> Alex, it occurs to me it might be more reasonable to move this to
> ARIAStateMap.cpp while we're here to keep the aria state stuff all in one
> place.

I'm not sure. It seems ARIAStateMap defines set of simple rules how to map ARIA states and it doesn't maintain any connection to markup used to turn on that ARIA state. nsARIAMap is more high level logic that describes connection between roles and ARIA states so perhaps it's reasonable to make nsARIAMap to describe universal roles as well.
(Reporter)

Comment 8

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> Well, I'm not sure I understand this we either have null or we have a
> pointer, similarly we could have a special none number or an actual index. 
> After all in a sense all a pointer is an index :)

anyway, having pointer or pointer + idx doesn't make big difference. If we provide set of inlines to operate with RoleMap by RoleMap index then it's fine with me I think. Can you think of how we can split this work into small bugs?
(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > Alex, it occurs to me it might be more reasonable to move this to
> > ARIAStateMap.cpp while we're here to keep the aria state stuff all in one
> > place.
> 
> I'm not sure. It seems ARIAStateMap defines set of simple rules how to map
> ARIA states and it doesn't maintain any connection to markup used to turn on
> that ARIA state. nsARIAMap is more high level logic that describes
> connection between roles and ARIA states so perhaps it's reasonable to make
> nsARIAMap to describe universal roles as well.

ok, that sort of makes sense, on the other hand I sort of tempted to try and break what's left in nsARIAMap.cpp up into related chunks.


(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > Well, I'm not sure I understand this we either have null or we have a
> > pointer, similarly we could have a special none number or an actual index. 
> > After all in a sense all a pointer is an index :)
> 
> anyway, having pointer or pointer + idx doesn't make big difference. If we
> provide set of inlines to operate with RoleMap by RoleMap index then it's
> fine with me I think. Can you think of how we can split this work into small
> bugs?

I don't have a clear picture of what I want to do in my mind yet.  I think I want to move the ARIA stuffs in nsAccUtils somewhere else, I'd sort of like to break up or find a better name for nsARIAMap, and maybe something else I'll think of as we go along, but I'm not sure I see how either of those should be done.  What do you want?
(Reporter)

Comment 10

5 years ago
I think I would replace nsARIAMap on aria namespace and then probably add helper functions to nsRoleMapEntry to move out the ARIA logic from nsAccessible (or if we keep integer instead of pointer to nsRoleMapEntry provide set of functions to deal with ARIA role). But first of all I would like to make nsARIAMap code consistent with code style we use. I think I'm ok if we follow comment #0 for this bug.

Comment 11

5 years ago
Created attachment 617180 [details] [diff] [review]
Bug 742659

Please review.
(Reporter)

Comment 12

5 years ago
Comment on attachment 617180 [details] [diff] [review]
Bug 742659

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

asking Trevor for review since he had conerns

::: accessible/src/base/nsARIAMap.h
@@ +242,5 @@
> +/**
> + * Return accessible state from ARIA universal states applied to the given
> + * element.
> + */
> +static PRUint64 UniversalStatesFor(mozilla::dom::Element* aElement);

don't make it static please, if you have linker error then put aria:: to function definition in cpp
Attachment #617180 - Flags: review?(trev.saunders)
Attachment #617180 - Flags: feedback+
Comment on attachment 617180 [details] [diff] [review]
Bug 742659

>-EStateRule nsARIAMap::gWAIUnivStateMap[] = {
>+static EStateRule gWAIUnivStateMap[] = {

rename it to sWAIUnivStateMap, and make it const please

>+static PRUint64 UniversalStatesFor(mozilla::dom::Element* aElement)
>+{
>+  PRUint64 state = 0;
>+  PRUint32 index = 0;
>+  while (MapToState(gWAIUnivStateMap[index], aElement, &state))
>+    index++;

so, one thing we could do is move the array into the function as a static local var, and do
for (int idx = 0; idx < ArrayLength(sWAIUnivStateMap); idx++)
    MapToStates(elem, sWAIUnivStateMap[idx], &states);

  return states;
}

but that seem complicated when it's really just
MapToStates(elem, eARIAFoo1, &states);
MapToStates(elem, eARIAFoo2, &states);
...
for 6 values of eARIAFoo, and I'll bet the compiler just generates a list of calls.
Attachment #617180 - Flags: review?(trev.saunders) → review+
(Reporter)

Comment 14

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #13)

> so, one thing we could do is move the array into the function as a static
> local var, and do
> for (int idx = 0; idx < ArrayLength(sWAIUnivStateMap); idx++)
>     MapToStates(elem, sWAIUnivStateMap[idx], &states);
> 
>   return states;
> }
> 
> but that seem complicated when it's really just
> MapToStates(elem, eARIAFoo1, &states);
> MapToStates(elem, eARIAFoo2, &states);
> ...
> for 6 values of eARIAFoo, and I'll bet the compiler just generates a list of
> calls.

I would leave it as is, perhaps a follow up would be nice (I'm not sure which way I like more).
(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> Comment on attachment 617180 [details] [diff] [review]
> Bug 742659
> 
> >-EStateRule nsARIAMap::gWAIUnivStateMap[] = {
> >+static EStateRule gWAIUnivStateMap[] = {
> 
> rename it to sWAIUnivStateMap, and make it const please

pcheng, mind doing this?
Created attachment 640144 [details] [diff] [review]
Patch - Rebased

Trev, this has been sitting for a few months ... I rebased it and addressed the last nit plus a build error ... if the review+ is still valid I can land it for the contributor...
(In reply to Mark Capella [:capella] from comment #16)
> Created attachment 640144 [details] [diff] [review]
> Patch - Rebased
> 
> Trev, this has been sitting for a few months ... I rebased it and addressed
> the last nit plus a build error ... if the review+ is still valid I can land
> it for the contributor...

sure, but fix the commit message / author :)
Start with TRY:
https://tbpl.mozilla.org/?tree=Try&rev=d2ef36979527
And on to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a86157211faa
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/a86157211faa
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.