1137714 - Make roleDescription nicer/correct/faster

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

the MozAccessiblemm roleDescription method is quite a mess. Make it nicer, more correct, and faster.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 8570652 [details] [diff] [review]
patch

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

r=me with nits fixed/questions answered.

::: accessible/mac/mozAccessible.mm
@@ +450,1 @@
>        return @"AXLandmarkSearch";

I briefly wondered if we should make these into a map, too, but then figured that the list of landmarks doesn't change that much, so it's probably not worth it, and also not less code, or?

@@ +462,2 @@
>      case roles::SWITCH:
>        return @"AXSwitch";

Same here. Would a map ake sense here, too, esp when extending with new roles?

@@ +470,5 @@
>  }
>  
> +struct RoleDescrMap
> +{
> +  NSString* role;

Nit: subRole, since we're comparing against sub roles and not the actual roles.

@@ +483,5 @@
> +  { @"AXLandmarkMain", NS_LITERAL_STRING("main") },
> +  { @"AXLandmarkNavigation", NS_LITERAL_STRING("navigation") },
> +  { @"AXLandmarkSearch", NS_LITERAL_STRING("search") },
> +  { @"AXTerm", NS_LITERAL_STRING("term") }
> +};

I'm afraid you'll have to rebase and augment the patch since I just landed bug 1121518 which adds stuff to the old code blocks in this file. Sorry! :)

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #2)

> ::: accessible/mac/mozAccessible.mm
> @@ +450,1 @@
> >        return @"AXLandmarkSearch";
> 
> I briefly wondered if we should make these into a map, too, but then figured
> that the list of landmarks doesn't change that much, so it's probably not
> worth it, and also not less code, or?

it's small, we shouldn't benefit much from map

> @@ +462,2 @@
> >      case roles::SWITCH:
> >        return @"AXSwitch";
> 
> Same here. Would a map ake sense here, too, esp when extending with new
> roles?

I'd say this one should go into RoleMap.h, that be useful for bug 1137748 I think

> > +struct RoleDescrMap
> > +{
> > +  NSString* role;
> 
> Nit: subRole, since we're comparing against sub roles and not the actual
> roles.

I would keep generic 'role', since it's used from roleDescription which works both for roles and subroles and (maybe quite unlikely) can be used for roles too. On the another hand subrole is also role, it's just an approach of role taxonomy implementation.

> > +  { @"AXLandmarkMain", NS_LITERAL_STRING("main") },
> > +  { @"AXLandmarkNavigation", NS_LITERAL_STRING("navigation") },
> > +  { @"AXLandmarkSearch", NS_LITERAL_STRING("search") },
> > +  { @"AXTerm", NS_LITERAL_STRING("term") }
> > +};
> 
> I'm afraid you'll have to rebase and augment the patch since I just landed
> bug 1121518 which adds stuff to the old code blocks in this file. Sorry! :)

I'm aware of that :)

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #3)
> (In reply to Marco Zehe (:MarcoZ) from comment #2)
> > @@ +462,2 @@
> > >      case roles::SWITCH:
> > >        return @"AXSwitch";
> > 
> > Same here. Would a map ake sense here, too, esp when extending with new
> > roles?
> 
> I'd say this one should go into RoleMap.h, that be useful for bug 1137748 I
> think

That's what I was thinking, too. Should that be part of bug 1137748, or a separate bug, or are you going to weave that into this one?

> > > +struct RoleDescrMap
> > > +{
> > > +  NSString* role;
> > 
> > Nit: subRole, since we're comparing against sub roles and not the actual
> > roles.
> 
> I would keep generic 'role', since it's used from roleDescription which
> works both for roles and subroles and (maybe quite unlikely) can be used for
> roles too. On the another hand subrole is also role, it's just an approach
> of role taxonomy implementation.

OK fine. :)

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #4)

> > I'd say this one should go into RoleMap.h, that be useful for bug 1137748 I
> > think
> 
> That's what I was thinking, too. Should that be part of bug 1137748, or a
> separate bug, or are you going to weave that into this one?

it's relatively small change and can be part of bug 1137748 I think. It can be dealt separately of course. I wouldn't grow this one :)

Comment 6

[Marco Zehe (:MarcoZ)]

Fair enough. Happy landing! :)

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c47eaffd147