Expandoify nsTextEquivUtils::gRoleToNameRulesMap

RESOLVED FIXED in mozilla17

Status

()

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

People

(Reporter: glandium, Assigned: drexler)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
see bug 765172 comment 6 and subsequent as to why this is important.
(Assignee)

Updated

5 years ago
Assignee: nobody → andrew.quartey
(Assignee)

Comment 1

5 years ago
Created attachment 643050 [details] [diff] [review]
patch

locally tested patch; passed all.
Attachment #643050 - Flags: review?(trev.saunders)
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).
Attachment #643050 - Flags: feedback+

Comment 3

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

5 years ago
(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.
Trevor note this bug only has f+, still waiting for review.
(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) \
Attachment #643050 - Flags: review?(trev.saunders) → review+

Comment 7

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

Comment 8

5 years ago
Created attachment 644572 [details] [diff] [review]
patch_for_checkin

changed rule to nameRule and multi-lined ROLE macros
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Comment 9

5 years ago
Why not have kept the gRoleToNameRulesMap array instead of using a big switch statement in a function?
(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.
(Reporter)

Comment 11

5 years ago
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5aa545274d
Flags: in-testsuite-
Keywords: checkin-needed
(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.
(Reporter)

Comment 14

5 years ago
The patch here does create a jump table with gcc 4.7 with either -Os or -O3.
(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.
https://hg.mozilla.org/mozilla-central/rev/2a5aa545274d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.