766240 - Expandoify nsTextEquivUtils::gRoleToNameRulesMap

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mike Hommey [:glandium]]

see bug 765172 comment 6 and subsequent as to why this is important.

Comment 1

[Andrew Quartey [:drexler]]

Attachment: patch

locally tested patch; passed all.

Comment 2

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

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

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

The rules seem to be copied over correctly (I cross checked).

Comment 3

[alexander :surkov (:asurkov)]

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

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

what rules were taken for missed roles?

::: accessible/src/atk/AccessibleWrap.cpp
@@ +685,5 @@
>  
>    if (aAtkObj->role != ATK_ROLE_INVALID)
>      return aAtkObj->role;
>  
> +#define ROLE(geckoRole, stringRole, atkRole, macRole, msaaRole, ia2Role, rule) \

rule -> nameRule and everywhere

Comment 4

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #3)
> what rules were taken for missed roles?

ignore the question, i see they were introduced in bug 765172.

Comment 5

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

Trevor note this bug only has f+, still waiting for review.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> Comment on attachment 643050 [details] [diff] [review]
> patch
> 
> Review of attachment 643050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> what rules were taken for missed roles?
> 
> ::: accessible/src/atk/AccessibleWrap.cpp
> @@ +685,5 @@
> >  
> >    if (aAtkObj->role != ATK_ROLE_INVALID)
> >      return aAtkObj->role;
> >  
> > +#define ROLE(geckoRole, stringRole, atkRole, macRole, msaaRole, ia2Role, rule) \
> 
> rule -> nameRule and everywhere

that makes sense, on other hand its already a long line.  it might make sense to name the unused ones just as x, y, z... so you'd have 
#define ROLE(x, AtkRole, y, z, x1, y2, z2) \

Comment 7

[alexander :surkov (:asurkov)]

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

> > > +#define ROLE(geckoRole, stringRole, atkRole, macRole, msaaRole, ia2Role, rule) \
> > 
> > rule -> nameRule and everywhere
> 
> that makes sense, on other hand its already a long line.  it might make
> sense to name the unused ones just as x, y, z... so you'd have 
> #define ROLE(x, AtkRole, y, z, x1, y2, z2) \

it works

Comment 8

[Andrew Quartey [:drexler]]

Attachment: patch_for_checkin

changed rule to nameRule and multi-lined ROLE macros

Comment 9

[Mike Hommey [:glandium]]

Why not have kept the gRoleToNameRulesMap array instead of using a big switch statement in a function?

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #9)
> Why not have kept the gRoleToNameRulesMap array instead of using a big
> switch statement in a function?

does it have an advantage?

I think switch makes a little more sense, and it seems like the compiler gets a little more freedom, but I don't care to strongly.  In practice gcc atleast seems to just  implement the switch with an array.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> (In reply to Mike Hommey [:glandium] from comment #9)
> > Why not have kept the gRoleToNameRulesMap array instead of using a big
> > switch statement in a function?
> 
> does it have an advantage?
> 
> I think switch makes a little more sense, and it seems like the compiler
> gets a little more freedom, but I don't care to strongly.  In practice gcc
> atleast seems to just  implement the switch with an array.

That's not entirely true. It creates a jump table. So instead of just reading values out of an array (which, btw, since all values are under 256, could be a uint8_t[]), it reads a value in a pointer table, then jumps to the corresponding address, at which location there basically is a return value, where value is 4, 3, or 1.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5aa545274d

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #11)
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > (In reply to Mike Hommey [:glandium] from comment #9)
> > > Why not have kept the gRoleToNameRulesMap array instead of using a big
> > > switch statement in a function?
> > 
> > does it have an advantage?
> > 
> > I think switch makes a little more sense, and it seems like the compiler
> > gets a little more freedom, but I don't care to strongly.  In practice gcc
> > atleast seems to just  implement the switch with an array.
> 
> That's not entirely true. It creates a jump table. So instead of just
> reading values out of an array (which, btw, since all values are under 256,
> could be a uint8_t[]), it reads a value in a pointer table, then jumps to
> the corresponding address, at which location there basically is a return
> value, where value is 4, 3, or 1.

no, actually I meant meant what I said.   admittly it was a while ago that I really played with it, but a quick test of switch(rand()) { case 1: return 1; case 2: return 2; ... case 200: return 200; } with gcc 4.7 and -O3 gave me the same result.

As I remember it sometimes did emit a jump table, but it also sometimes definitely does not.  The disassembly for the ultra simple test case above was basically

call rand
mov edx, eax
cmp edx, 200
ja <address of return 0>
mov eax, byte ptr [0x400680+ex]
ret

it also fiddled with the stack pointer even though it didn't use any stack slots that I noticed, which seems funny.

Comment 14

[Mike Hommey [:glandium]]

The patch here does create a jump table with gcc 4.7 with either -Os or -O3.

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #14)
> The patch here does create a jump table with gcc 4.7 with either -Os or -O3.

ok, so you checked this particular one.  That's kind of interesting, however I  don't believe this code to be particularly hot or bottlenecky so I'm not terribly concerned.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a5aa545274d