Last Comment Bug 766240 - Expandoify nsTextEquivUtils::gRoleToNameRulesMap
: Expandoify nsTextEquivUtils::gRoleToNameRulesMap
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Andrew Quartey [:drexler]
:
Mentors:
Depends on: 716644 765172
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-19 12:01 PDT by Mike Hommey [:glandium]
Modified: 2012-07-21 14:42 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (40.37 KB, patch)
2012-07-17 11:17 PDT, Andrew Quartey [:drexler]
tbsaunde+mozbugs: review+
dbolter: feedback+
Details | Diff | Review
patch_for_checkin (40.48 KB, patch)
2012-07-20 19:37 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review

Description Mike Hommey [:glandium] 2012-06-19 12:01:43 PDT
see bug 765172 comment 6 and subsequent as to why this is important.
Comment 1 Andrew Quartey [:drexler] 2012-07-17 11:17:36 PDT
Created attachment 643050 [details] [diff] [review]
patch

locally tested patch; passed all.
Comment 2 David Bolter [:davidb] 2012-07-17 12:52:01 PDT
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 2012-07-17 22:38:13 PDT
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 2012-07-17 22:40:47 PDT
(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] 2012-07-18 08:42:31 PDT
Trevor note this bug only has f+, still waiting for review.
Comment 6 Trevor Saunders (:tbsaunde) 2012-07-19 20:17:55 PDT
(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 2012-07-20 19:35:50 PDT
(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] 2012-07-20 19:37:46 PDT
Created attachment 644572 [details] [diff] [review]
patch_for_checkin

changed rule to nameRule and multi-lined ROLE macros
Comment 9 Mike Hommey [:glandium] 2012-07-21 00:39:59 PDT
Why not have kept the gRoleToNameRulesMap array instead of using a big switch statement in a function?
Comment 10 Trevor Saunders (:tbsaunde) 2012-07-21 04:55:37 PDT
(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] 2012-07-21 11:12:27 PDT
(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] 2012-07-21 11:34:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5aa545274d
Comment 13 Trevor Saunders (:tbsaunde) 2012-07-21 11:46:11 PDT
(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] 2012-07-21 11:49:20 PDT
The patch here does create a jump table with gcc 4.7 with either -Os or -O3.
Comment 15 Trevor Saunders (:tbsaunde) 2012-07-21 12:00:17 PDT
(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] 2012-07-21 14:42:36 PDT
https://hg.mozilla.org/mozilla-central/rev/2a5aa545274d

Note You need to log in before you can comment on or make changes to this bug.