Last Comment Bug 716644 - expandoify accessible roles
: expandoify accessible roles
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Andrzej Skalski
:
:
Mentors:
Depends on: 765172
Blocks: cleana11y 750853 766240
  Show dependency treegraph
 
Reported: 2012-01-09 12:58 PST by Trevor Saunders (:tbsaunde)
Modified: 2012-06-19 12:01 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed RoleMap.h file (31.43 KB, text/x-chdr)
2012-04-12 11:05 PDT, Andrzej Skalski
no flags Details
proposed RoleMap.h file (31.34 KB, text/plain)
2012-04-12 11:16 PDT, Andrzej Skalski
no flags Details
patch proposition (67.41 KB, patch)
2012-04-23 11:07 PDT, Andrzej Skalski
no flags Details | Diff | Splinter Review
Second patch proposition (75.53 KB, patch)
2012-04-24 05:08 PDT, Andrzej Skalski
no flags Details | Diff | Splinter Review
Third patch proposition (77.86 KB, patch)
2012-04-25 06:45 PDT, Andrzej Skalski
no flags Details | Diff | Splinter Review
Fourth patch proposition (86.34 KB, patch)
2012-04-25 10:26 PDT, Andrzej Skalski
no flags Details | Diff | Splinter Review
5th patch. (74.68 KB, patch)
2012-04-26 11:17 PDT, Andrzej Skalski
no flags Details | Diff | Splinter Review
6th patch (74.16 KB, patch)
2012-04-26 11:26 PDT, Andrzej Skalski
hub: review-
Details | Diff | Splinter Review
7th patch (74.81 KB, patch)
2012-04-27 03:19 PDT, Andrzej Skalski
no flags Details | Diff | Splinter Review
patch 7 (82.42 KB, patch)
2012-04-27 03:19 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
patch8 (82.64 KB, patch)
2012-04-29 19:09 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (903 bytes, patch)
2012-04-30 13:34 PDT, Olli Pettay [:smaug]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-01-09 12:58:31 PST
its really anoying that you need to remember to change a bunch of places to change or add a role.  additionally, its really silly that we assert at *run time* that the table lengths match up.

proposal: use expandos to generate the tables at build time like we do with atoms and various other things.  So we'd have a file RoleList.h that looks like

ROLE(BUTTON, <ia2_button_role>, <atk button role>, "button") or so

then in Roles.h we'd have

enum Role {
#define ROLE(geckoRole, IA2Role, AtkRole, StringRole) geckoRole,
#include RoleList.h
#undef ROLE
};

then in the platform RoleMap.h we'd have

AtkRole roleMap[] = {
#define ROLE(geckoRole, IA2Role, ATKRole, stringRole) ATKRole,
#include RoleList.h
#undef ROLE
};

and finally because we assume that the gecko role and the xpcom role are the same, but saddly can't gnerate the xpcom role list from RoleList.h because of the way xpidl works we can static assert that Role::BUTTON and nsIAccessibleRole::ROLE_BUTTON are the same by having a file like RoleAsserts.cpp like this

#define ROLE(geckoRole, ia2Role, atkRole, stringRole, xpcomRole) static_assert(roles::geckoRole == nsIAccessibleRole::ROLE_ ## xpcomRole);

Alex, david objections?
Comment 1 David Bolter [:davidb] 2012-01-09 13:02:09 PST
I'd review a patch.
Comment 2 Trevor Saunders (:tbsaunde) 2012-01-11 19:35:43 PST
Andrzej I don't think anyone has claimed this yet if you want it.
Comment 3 alexander :surkov 2012-01-19 02:37:23 PST
I see what are you trying achieve but I don't think I'm fan of implementation approach since it puts MSAA/IA2/ATK/WhateverElse together. Especially it's sort of ugly in bug 717507 which makes think we move ATK stuffs into base folder.
Comment 4 Andrzej Skalski 2012-01-19 05:26:18 PST
Alexander, can you explain further, what do you mean by "moving ATK stuffs into base folder"? I am working on this patch, as well as 717507, and I wanted to solve both the way I Trevor proposed.
Comment 5 Trevor Saunders (:tbsaunde) 2012-01-19 06:17:35 PST
(In reply to alexander :surkov from comment #3)
> I see what are you trying achieve but I don't think I'm fan of
> implementation approach since it puts MSAA/IA2/ATK/WhateverElse together.

I think I like that they're in the same place

> Especially it's sort of ugly in bug 717507 which makes think we move ATK
> stuffs into base folder.

I'll give you putting them in base/ is a little weird.

Do you have a better suggestion?
Comment 6 David Bolter [:davidb] 2012-01-19 06:29:41 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to alexander :surkov from comment #3)
> > I see what are you trying achieve but I don't think I'm fan of
> > implementation approach since it puts MSAA/IA2/ATK/WhateverElse together.
> 
> I think I like that they're in the same place

I agree on this point, since they serve almost as a document.
Comment 7 alexander :surkov 2012-01-19 11:01:56 PST
(In reply to David Bolter [:davidb] from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > (In reply to alexander :surkov from comment #3)
> > > I see what are you trying achieve but I don't think I'm fan of
> > > implementation approach since it puts MSAA/IA2/ATK/WhateverElse together.
> > 
> > I think I like that they're in the same place
> 
> I agree on this point, since they serve almost as a document.

I wouldn't say that because in case of states for example it documents atk and core stuffs only and include some atk internal mapping logic (constants kMapOpposite). So it sounds like we put all we can into one big heap what doesn't give us a good picture.

for roles we have LAST_ENTRY assertions so we should be ok if somebody forgot to update the array, we don't have it for states though. Maybe the current approach is not so scary and we could live with it until we have something nice.
Comment 8 Trevor Saunders (:tbsaunde) 2012-01-20 07:07:23 PST
(In reply to alexander :surkov from comment #7)
> (In reply to David Bolter [:davidb] from comment #6)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > > (In reply to alexander :surkov from comment #3)
> > > > I see what are you trying achieve but I don't think I'm fan of
> > > > implementation approach since it puts MSAA/IA2/ATK/WhateverElse together.
> > > 
> > > I think I like that they're in the same place
> > 
> > I agree on this point, since they serve almost as a document.
> 
> I wouldn't say that because in case of states for example it documents atk
> and core stuffs only and include some atk internal mapping logic (constants
> kMapOpposite). So it sounds like we put all we can into one big heap what
> doesn't give us a good picture.

Well, for one thing when you change part of  a role or add a new one this makes it very clear what else you  need to update right?  It also makes it easier to see what each thing gets mapped to on each platform.  If you want to do that now you iether need to maintain a bunch of comments in several places or count into the array to the index of the  internal thing you want to know about.

btw ideally we could put the xpcom string state and the ia2 one in with everything else, but I wasn't sure of that until I looked more deeply.

> for roles we have LAST_ENTRY assertions so we should be ok if somebody
> forgot to update the array, we don't have it for states though. Maybe the

that only"saves you" if you make the array a different length, but not if you say reorder it.  Also it only saves you if you either run the code on all platforms or look at assertion logs for all platforms.  I'd claim based on the frequency of asserts I see when I run debug builds we don't do this terribly well.  This is also a win here because you know sooner that there is a problem in what you've written.

> current approach is not so scary and we could live with it until we have
> something nice.

Unless it has a cost I tend to think incrementally improving to something somewhat better is worth doing especially when we don't have an idea for the perfect solution, and in this case I don't have any idea what your perfect solution even looks like.
Comment 9 alexander :surkov 2012-01-20 08:27:46 PST
Now we have plain and nice Role.h file like:

/**
 * Represents a title or caption bar for a window. It is used by MSAA only,
 * supported automatically by MS Windows.
 */
TITLEBAR = 1,

and have a bunch of nsRoleMap.h, one for each platform, if you change Role.h then you change nsRoleMap (and yes granted, plus array of strings).

Your suggestion to make Role.h look like

/**
 * Represents a title or caption bar for a window. It is used by MSAA only,
 * supported automatically by MS Windows.
 */
ROLE(TITLEBAR, // internal
     "titlebar", // string rep
     ROLE_TITLEBAR, // XPCOM,
     ROLE_SYSTEM_TITLEBAR, // MSAA
     ROLE_SYSTEM_TITLEBAR, // IA2
     NSAccessibilityUnknownRole, // OS X NSAccessibility
     ATK_ROLE_UNKNOWN // ATK

I see that's quite useful to see how we map role on each platform and it's easier to change role mappings but that looks too cumbersome. So I can't say I'm convinced.

in case of states you suggest to add here the logic description how we map particular internal state into platform state.

Opinions? Neil, Hub, Eitan?
Comment 10 Trevor Saunders (:tbsaunde) 2012-01-21 10:08:00 PST
(In reply to alexander :surkov from comment #9)
> Now we have plain and nice Role.h file like:
> 
> /**
>  * Represents a title or caption bar for a window. It is used by MSAA only,
>  * supported automatically by MS Windows.
>  */
> TITLEBAR = 1,
> 
> and have a bunch of nsRoleMap.h, one for each platform, if you change Role.h
> then you change nsRoleMap (and yes granted, plus array of strings).

and nsIAccessibleRole.idl :)

> Your suggestion to make Role.h look like
> 
> /**
>  * Represents a title or caption bar for a window. It is used by MSAA only,
>  * supported automatically by MS Windows.
>  */
> ROLE(TITLEBAR, // internal
>      "titlebar", // string rep
>      ROLE_TITLEBAR, // XPCOM,

I'd be a little suprised if we can't generate the third from the first, and maybe the second too, but that needs investigation.

>      ROLE_SYSTEM_TITLEBAR, // MSAA
>      ROLE_SYSTEM_TITLEBAR, // IA2
>      NSAccessibilityUnknownRole, // OS X NSAccessibility
>      ATK_ROLE_UNKNOWN // ATK

I wonder if we can skip the prefixs here, and have the macros automatically prepend them, its more complicated, but makes it shorter.

btw I think repeating those comments for each macro invokation is kind of silly, so I imagine something like

ROLE(TITLEBAR, "titlebar", ROLE_SYSTEM_TITLEBAR,
     ROLE_TITLEBAR, Unknown, UNKNOWN)

> I see that's quite useful to see how we map role on each platform and it's
> easier to change role mappings but that looks too cumbersome. So I can't say
> I'm convinced.

I'll agree with the cumbersome part a little but isn't things like
 ATK_ROLE_FOO, // roels::FOO 11
 also a little heavy?

> in case of states you suggest to add here the logic description how we map
> particular internal state into platform state.

I'll grant that its a little unfortunate.
Comment 11 alexander :surkov 2012-01-22 04:52:17 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #10)

> > ROLE(TITLEBAR, // internal
> >      "titlebar", // string rep
> >      ROLE_TITLEBAR, // XPCOM,
> 
> I'd be a little suprised if we can't generate the third from the first, and
> maybe the second too, but that needs investigation.

automation hides details, that's probably what we wouldn't want since details serves here for transparency how we do a mapping. though it might be ok to get xpcom role from internal role since we are allowed keep them the same.

> >      ROLE_SYSTEM_TITLEBAR, // MSAA
> >      ROLE_SYSTEM_TITLEBAR, // IA2
> >      NSAccessibilityUnknownRole, // OS X NSAccessibility
> >      ATK_ROLE_UNKNOWN // ATK
> 
> I wonder if we can skip the prefixs here, and have the macros automatically
> prepend them, its more complicated, but makes it shorter.

I wouldn't do that. Usually you copy/paste/search and then your search get confused because of absent prefixes.

> btw I think repeating those comments for each macro invokation is kind of
> silly, so I imagine something like
> 
> ROLE(TITLEBAR, "titlebar", ROLE_SYSTEM_TITLEBAR,
>      ROLE_TITLEBAR, Unknown, UNKNOWN)

that's harder to read I think
Comment 12 Trevor Saunders (:tbsaunde) 2012-04-05 18:51:51 PDT
> > >      ROLE_SYSTEM_TITLEBAR, // MSAA
> > >      ROLE_SYSTEM_TITLEBAR, // IA2
> > >      NSAccessibilityUnknownRole, // OS X NSAccessibility
> > >      ATK_ROLE_UNKNOWN // ATK
> > 
> > I wonder if we can skip the prefixs here, and have the macros automatically
> > prepend them, its more complicated, but makes it shorter.
> 
> I wouldn't do that. Usually you copy/paste/search and then your search get
> confused because of absent prefixes.

I'm not sure I follow, but ok.

> > btw I think repeating those comments for each macro invokation is kind of
> > silly, so I imagine something like
> > 
> > ROLE(TITLEBAR, "titlebar", ROLE_SYSTEM_TITLEBAR,
> >      ROLE_TITLEBAR, Unknown, UNKNOWN)
> 
> that's harder to read I think

so, break the lines nicely? what's hard to read about it?

At any rate could we figure something here that's better than for each platform
MYPLATFORM_ROLE_FOO // roles::FOO 100
and then when you have to review a change to one of these you can either
count what role this in the list and what number it is in Roles.h and make sure they match up or hope the comment is correct?
Comment 13 alexander :surkov 2012-04-05 23:35:36 PDT
Ok, I can't think of anything better. XPCOM idl generation is not friendly but should be ok (like generation in general). But did you have a chance to figure how we can do that?

I'm still thinking about the following format (granted we can exclude XPCOM constants for now since it's 1 to 1 to internal roles).

/**
 * Represents a title or caption bar for a window. It is used by MSAA only,
 * supported automatically by MS Windows.
 */
ROLE(TITLEBAR,
     ATK_ROLE_UNKNOWN, // ATK
     ROLE_SYSTEM_TITLEBAR, // MSAA
     ROLE_SYSTEM_TITLEBAR, // IA2
     NSAccessibilityUnknownRole, // OS X NSAccessibility
     "titlebar") // string rep

keeping each constants on own line and having a comment is easier to read.

I would name the file as RoleMap.h instead of RoleList.h I think.

Also it'd be nice to hide role mapping arrays by nice methods, for example:

class nsAccessibleWrap : public nsAccesisble, public IAccessible
{
public:
  long MSAARole() { return sMSAARoles[Role()]; }
  long IARole() { return sIARoles[Role()]; }

private:
  // MSAA role map.
  static long sMSAARoles[
#define ROLE(geckoRole, AtkRole, MSAARole, IA2Role, StringRole) MSAARole,
#include RoleMap.h
#undef ROLE
  ];

  // IA2 role map.
  static long sIA2Roles[
#define ROLE(geckoRole, AtkRole, MSAARole, IA2Role, StringRole) IA2Role,
#include RoleMap.h
#undef ROLE
  ];
};
Comment 14 Andrzej Skalski 2012-04-06 06:11:58 PDT
good. I stopped working on it 2 months ago, I will propose a patch in accordance to Alexander's suggestions soon.
Comment 15 alexander :surkov 2012-04-06 06:51:24 PDT
(In reply to Andrzej Skalski from comment #14)
> good. I stopped working on it 2 months ago, I will propose a patch in
> accordance to Alexander's suggestions soon.

Thank you, Andrzej. Note, for MSAARole/IA2Role stuff you should initialize them in cpp to avoid RoleMap.h including into header files.
Comment 16 Andrzej Skalski 2012-04-11 08:11:28 PDT
Alexander, two questions:
1) should I merge the base/Role.h as well? That would require me to add integer identifier to state (it is done in my script, just tell me if that's desired)
2) does the order of these constants in macro make any significant difference?
Comment 17 alexander :surkov 2012-04-11 08:21:56 PDT
(In reply to Andrzej Skalski from comment #16)
> Alexander, two questions:
> 1) should I merge the base/Role.h as well? That would require me to add
> integer identifier to state (it is done in my script, just tell me if that's
> desired)

for base/Role.h I think you should follow comment #0 I think. I didn't catch you about state.

> 2) does the order of these constants in macro make any significant
> difference?

no, it just should be nice :) if you keep in mind a different order then put it here please so that everybody gets agree on it
Comment 18 Andrzej Skalski 2012-04-12 11:05:06 PDT
Created attachment 614455 [details]
proposed RoleMap.h file

Alexander, please take a look on the macro file. If it's good, I will build a patch around it.
Comment 19 Andrzej Skalski 2012-04-12 11:11:15 PDT
btw, in February I tried to generate some of these constants from other, and I was unsuccessful. I think this is a minimal number of fields in macro.
Comment 20 Andrzej Skalski 2012-04-12 11:16:01 PDT
Created attachment 614463 [details]
proposed RoleMap.h file

some details fixed
Comment 21 alexander :surkov 2012-04-13 04:38:50 PDT
Comment on attachment 614463 [details]
proposed RoleMap.h file

>/* This Source Code Form is subject to the terms of the Mozilla Public
> * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> * You can obtain one at http://mozilla.org/MPL/2.0/. */
>
>/*
> * macro:
> * Role base name
> * Role number
> * Atk role name
> * OS X NSAccessibility
> * MSAA role
> * I2A role
> */
>
>/**
> * Used when accessible hans't strong defined role.
> */
>ROLE(NOTHING,
>	0,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_UNKNOWN) 
>
>/**
> * Represents a title or caption bar for a window. It is used by MSAA only,
> * supported automatically by MS Windows.
> */
>ROLE(TITLEBAR,
>	1,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,	//(irrelevant on OS X; windows are always native.)
>	ROLE_SYSTEM_TITLEBAR,
>	ROLE_SYSTEM_TITLEBAR) 
>
>/**
> * Represents the menu bar (positioned beneath the title bar of a window)
> * from which menus are selected by the user. The role is used by
> * xul:menubar or role="menubar".
> */
>ROLE(MENUBAR,
>	2,
>	ATK_ROLE_MENU_BAR,
>	NSAccessibilityMenuBarRole,	//(irrelevant on OS X; the menubar will always be native and on the top of the screen.)
>	ROLE_SYSTEM_MENUBAR,
>	ROLE_SYSTEM_MENUBAR) 
>
>/**
> * Represents a vertical or horizontal scroll bar, which is part of the client
> * area or used in a control.
> */
>ROLE(SCROLLBAR,
>	3,
>	ATK_ROLE_SCROLL_BAR,
>	NSAccessibilityScrollBarRole,	//we might need to make this its own mozAccessible, to support the children objects (valueindicator, down/up buttons).
>	ROLE_SYSTEM_SCROLLBAR,
>	ROLE_SYSTEM_SCROLLBAR) 
>
>/**
> * Represents a special mouse pointer, which allows a user to manipulate user
> * interface elements such as windows. For example, a user clicks and drags
> * a sizing grip in the lower-right corner of a window to resize it.
> */
>ROLE(GRIP,
>	4,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilitySplitterRole,
>	ROLE_SYSTEM_GRIP,
>	ROLE_SYSTEM_GRIP) 
>
>/**
> * Represents a system sound, which is associated with various system events.
> */
>ROLE(SOUND,
>	5,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,	//unused on OS X
>	ROLE_SYSTEM_SOUND,
>	ROLE_SYSTEM_SOUND) 
>
>/**
> * Represents the system mouse pointer.
> */
>ROLE(CURSOR,
>	6,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,	//unused on OS X
>	ROLE_SYSTEM_CURSOR,
>	ROLE_SYSTEM_CURSOR) 
>
>/**
> * Represents the system caret. The role is supported for caret.
> */
>ROLE(CARET,
>	7,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,	//unused on OS X
>	ROLE_SYSTEM_CARET,
>	ROLE_SYSTEM_CARET) 
>
>/**
> * Represents an alert or a condition that a user should be notified about.
> * Assistive Technologies typically respond to the role by reading the entire
> * onscreen contents of containers advertising this role. Should be used for
> * warning dialogs, etc. The role is used by xul:browsermessage,
> * role="alert", xforms:message.
> */
>ROLE(ALERT,
>	8,
>	ATK_ROLE_ALERT,
>	NSAccessibilityWindowRole,
>	ROLE_SYSTEM_ALERT,
>	ROLE_SYSTEM_ALERT) 
>
>/**
> * Represents the window frame, which contains child objects such as
> * a title bar, client, and other objects contained in a window. The role
> * is supported automatically by MS Windows.
> */
>ROLE(WINDOW,
>	9,
>	ATK_ROLE_WINDOW,
>	NSAccessibilityWindowRole,	//irrelevant on OS X; all window a11y is handled by the system.
>	ROLE_SYSTEM_WINDOW,
>	ROLE_SYSTEM_WINDOW) 
>
>/**
> * A sub-document (<frame> or <iframe>)
> */
>ROLE(INTERNAL_FRAME,
>	10,
>	ATK_ROLE_INTERNAL_FRAME,
>	NSAccessibilityScrollAreaRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_INTERNAL_FRAME) 
>
>/**
> * Represents a menu, which presents a list of options from which the user can
> * make a selection to perform an action. It is used for role="menu".
> */
>ROLE(MENUPOPUP,
>	11,
>	ATK_ROLE_MENU,
>	NSAccessibilityMenuRole,	//the parent of menuitems
>	ROLE_SYSTEM_MENUPOPUP,
>	ROLE_SYSTEM_MENUPOPUP) 
>
>/**
> * Represents a menu item, which is an entry in a menu that a user can choose
> * to carry out a command, select an option. It is used for xul:menuitem,
> * role="menuitem".
> */
>ROLE(MENUITEM,
>	12,
>	ATK_ROLE_MENU_ITEM,
>	NSAccessibilityMenuItemRole,	//
>	ROLE_SYSTEM_MENUITEM,
>	ROLE_SYSTEM_MENUITEM) 
>
>/**
> * Represents a ToolTip that provides helpful hints.
> */
>ROLE(TOOLTIP,
>	13,
>	ATK_ROLE_TOOL_TIP,
>	@"AXHelpTag",	//10.4+ only, so we re-define the constant.
>	ROLE_SYSTEM_TOOLTIP,
>	ROLE_SYSTEM_TOOLTIP) 
>
>/**
> * Represents a main window for an application. It is used for
> * role="application". Also refer to APP_ROOT
> */
>ROLE(APPLICATION,
>	14,
>	ATK_ROLE_EMBEDDED,
>	NSAccessibilityGroupRole,	//unused on OS X. the system will take care of this.
>	ROLE_SYSTEM_APPLICATION,
>	ROLE_SYSTEM_APPLICATION) 
>
>/**
> * Represents a document window. A document window is always contained within
> * an application window. It is used for role="document".
> */
>ROLE(DOCUMENT,
>	15,
>	ATK_ROLE_DOCUMENT_FRAME,
>	@"AXWebArea",
>	ROLE_SYSTEM_DOCUMENT,
>	ROLE_SYSTEM_DOCUMENT) 
>
>/**
> * Represents a pane within a frame or document window. Users can navigate
> * between panes and within the contents of the current pane, but cannot
> * navigate between items in different panes. Thus, panes represent a level
> * of grouping lower than frame windows or documents, but above individual
> * controls. It is used for the first child of a <frame> or <iframe>.
> */
>ROLE(PANE,
>	16,
>	ATK_ROLE_PANEL,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_GROUPING,
>	ROLE_SYSTEM_GROUPING)
>/**
> * msaa comment:
> * We used to map to ROLE_SYSTEM_PANE, but JAWS would
> * not read the accessible name for the contaning pane.
> * However, JAWS will read the accessible name for a groupbox.
> * By mapping a PANE to a GROUPING, we get no undesirable effects,
> * but fortunately JAWS will then read the group's label,
> * when an inner control gets focused.
> */
>
>
>/**
> * Represents a graphical image used to represent data.
> */
>ROLE(CHART,
>	17,
>	ATK_ROLE_CHART,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_CHART,
>	ROLE_SYSTEM_CHART) 
>
>/**
> * Represents a dialog box or message box. It is used for xul:dialog,
> * role="dialog".
> */
>ROLE(DIALOG,
>	18,
>	ATK_ROLE_DIALOG,
>	NSAccessibilityWindowRole,	//there's a dialog subrole.
>	ROLE_SYSTEM_DIALOG,
>	ROLE_SYSTEM_DIALOG) 
>
>/**
> * Represents a window border.
> */
>ROLE(BORDER,
>	19,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,	//unused on OS X
>	ROLE_SYSTEM_BORDER,
>	ROLE_SYSTEM_BORDER) 
>
>/**
> * Logically groups other objects. There is not always a parent-child
> * relationship between the grouping object and the objects it contains. It
> * is used for html:textfield, xul:groupbox, role="group".
> */
>ROLE(GROUPING,
>	20,
>	ATK_ROLE_PANEL,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_GROUPING,
>	ROLE_SYSTEM_GROUPING) 
>
>/**
> * Used to visually divide a space into two regions, such as a separator menu
> * item or a bar that divides split panes within a window. It is used for
> * xul:separator, html:hr, role="separator".
> */
>ROLE(SEPARATOR,
>	21,
>	ATK_ROLE_SEPARATOR,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_SEPARATOR,
>	ROLE_SYSTEM_SEPARATOR) 
>
>/**
> * Represents a toolbar, which is a grouping of controls (push buttons or
> * toggle buttons) that provides easy access to frequently used features. It
> * is used for xul:toolbar, role="toolbar".
> */
>ROLE(TOOLBAR,
>	22,
>	ATK_ROLE_TOOL_BAR,
>	NSAccessibilityToolbarRole,
>	ROLE_SYSTEM_TOOLBAR,
>	ROLE_SYSTEM_TOOLBAR) 
>
>/**
> * Represents a status bar, which is an area at the bottom of a window that
> * displays information about the current operation, state of the application,
> * or selected object. The status bar has multiple fields, which display
> * different kinds of information. It is used for xul:statusbar.
> */
>ROLE(STATUSBAR,
>	23,
>	ATK_ROLE_STATUSBAR,
>	NSAccessibilityUnknownRole,	//doesn't exist on OS X (a status bar is its parts; a progressbar, a label, etc.)
>	ROLE_SYSTEM_STATUSBAR,
>	ROLE_SYSTEM_STATUSBAR) 
>
>/**
> * Represents a table that contains rows and columns of cells, and optionally,
> * row headers and column headers. It is used for html:table,
> * role="grid". Also refer to the following role: COLUMNHEADER,
> * ROWHEADER, COLUMN, ROW, CELL.
> */
>ROLE(TABLE,
>	24,
>	ATK_ROLE_TABLE,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_TABLE,
>	ROLE_SYSTEM_TABLE) 
>
>/**
> * Represents a column header, providing a visual label for a column in
> * a table. It is used for XUL tree column headers, html:th,
> * role="colheader". Also refer to TABLE.
> */
>ROLE(COLUMNHEADER,
>	25,
>	ATK_ROLE_COLUMN_HEADER,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_COLUMNHEADER,
>	ROLE_SYSTEM_COLUMNHEADER) 
>
>/**
> * Represents a row header, which provides a visual label for a table row.
> * It is used for role="rowheader". Also, see TABLE.
> */
>ROLE(ROWHEADER,
>	26,
>	ATK_ROLE_ROW_HEADER,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_ROWHEADER,
>	ROLE_SYSTEM_ROWHEADER) 
>
>/**
> * Represents a column of cells within a table. Also, see TABLE.
> */
>ROLE(COLUMN,
>	27,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityColumnRole,
>	ROLE_SYSTEM_COLUMN,
>	ROLE_SYSTEM_COLUMN) 
>
>/**
> * Represents a row of cells within a table. Also, see TABLE.
> */
>ROLE(ROW,
>	28,
>	ATK_ROLE_LIST_ITEM,
>	NSAccessibilityRowRole,
>	ROLE_SYSTEM_ROW,
>	ROLE_SYSTEM_ROW) 
>
>/**
> * Represents a cell within a table. It is used for html:td,
> * xul:tree cell and xul:listcell. Also, see TABLE.
> */
>ROLE(CELL,
>	29,
>	ATK_ROLE_TABLE_CELL,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_CELL,
>	ROLE_SYSTEM_CELL) 
>
>/**
> * Represents a link to something else. This object might look like text or
> * a graphic, but it acts like a button. It is used for
> * xul:label@class="text-link", html:a, html:area,
> * xforms:trigger@appearance="minimal".
> */
>ROLE(LINK,
>	30,
>	ATK_ROLE_LINK,
>	@"AXLink",	//10.4+ the attr first define in SDK 10.4, so we define it here too. ROLE_LINK
>	ROLE_SYSTEM_LINK,
>	ROLE_SYSTEM_LINK) 
>
>/**
> * Displays a Help topic in the form of a ToolTip or Help balloon.
> */
>ROLE(HELPBALLOON,
>	31,
>	ATK_ROLE_UNKNOWN,
>	@"AXHelpTag",
>	ROLE_SYSTEM_HELPBALLOON,
>	ROLE_SYSTEM_HELPBALLOON) 
>
>/**
> * Represents a cartoon-like graphic object, such as Microsoft Office
> * Assistant, which is displayed to provide help to users of an application.
> */
>ROLE(CHARACTER,
>	32,
>	ATK_ROLE_IMAGE,
>	NSAccessibilityUnknownRole,	//unused on OS X
>	ROLE_SYSTEM_CHARACTER,
>	ROLE_SYSTEM_CHARACTER) 
>
>/**
> * Represents a list box, allowing the user to select one or more items. It
> * is used for xul:listbox, html:select@size, role="list". See also
> * LIST_ITEM.
> */
>ROLE(LIST,
>	33,
>	ATK_ROLE_LIST,
>	NSAccessibilityListRole,
>	ROLE_SYSTEM_LIST,
>	ROLE_SYSTEM_LIST) 
>
>/**
> * Represents an item in a list. See also LIST.
> */
>ROLE(LISTITEM,
>	34,
>	ATK_ROLE_LIST_ITEM,
>	NSAccessibilityRowRole,
>	ROLE_SYSTEM_LISTITEM,
>	ROLE_SYSTEM_LISTITEM) 
>
>/**
> * Represents an outline or tree structure, such as a tree view control,
> * that displays a hierarchical list and allows the user to expand and
> * collapse branches. Is is used for role="tree".
> */
>ROLE(OUTLINE,
>	35,
>	ATK_ROLE_TREE,
>	NSAccessibilityOutlineRole,
>	ROLE_SYSTEM_OUTLINE,
>	ROLE_SYSTEM_OUTLINE) 
>
>/**
> * Represents an item in an outline or tree structure. It is used for
> * role="treeitem".
> */
>ROLE(OUTLINEITEM,
>	36,
>	ATK_ROLE_LIST_ITEM,
>	NSAccessibilityRowRole,	//XXX: use OutlineRow as subrole.
>	ROLE_SYSTEM_OUTLINEITEM,
>	ROLE_SYSTEM_OUTLINEITEM) 
>
>/**
> * Represents a page tab, it is a child of a page tab list. It is used for
> * xul:tab, role="treeitem". Also refer to PAGETABLIST.
> */
>ROLE(PAGETAB,
>	37,
>	ATK_ROLE_PAGE_TAB,
>	NSAccessibilityRadioButtonRole,
>	ROLE_SYSTEM_PAGETAB,
>	ROLE_SYSTEM_PAGETAB) 
>
>/**
> * Represents a property sheet. It is used for xul:tabpanel,
> * role="tabpanel".
> */
>ROLE(PROPERTYPAGE,
>	38,
>	ATK_ROLE_SCROLL_PANE,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_PROPERTYPAGE,
>	ROLE_SYSTEM_PROPERTYPAGE) 
>
>/**
> * Represents an indicator, such as a pointer graphic, that points to the
> * current item.
> */
>ROLE(INDICATOR,
>	39,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_INDICATOR,
>	ROLE_SYSTEM_INDICATOR) 
>
>/**
> * Represents a picture. Is is used for xul:image, html:img.
> */
>ROLE(GRAPHIC,
>	40,
>	ATK_ROLE_IMAGE,
>	NSAccessibilityImageRole,
>	ROLE_SYSTEM_GRAPHIC,
>	ROLE_SYSTEM_GRAPHIC) 
>
>/**
> * Represents read-only text, such as labels for other controls or
> * instructions in a dialog box. Static text cannot be modified or selected.
> * Is is used for xul:label, xul:description, html:label, role="label",
> * or xforms:output.
> */
>ROLE(STATICTEXT,
>	41,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityStaticTextRole,
>	ROLE_SYSTEM_STATICTEXT,
>	ROLE_SYSTEM_STATICTEXT) 
>
>/**
> * Represents selectable text that allows edits or is designated read-only.
> */
>ROLE(TEXT_LEAF,
>	42,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityStaticTextRole,
>	ROLE_SYSTEM_TEXT,
>	ROLE_SYSTEM_TEXT) 
>
>/**
> * Represents a push button control. It is used for xul:button, html:button,
> * role="button", xforms:trigger, xforms:submit.
> */
>ROLE(PUSHBUTTON,
>	43,
>	ATK_ROLE_PUSH_BUTTON,
>	NSAccessibilityButtonRole,
>	ROLE_SYSTEM_PUSHBUTTON,
>	ROLE_SYSTEM_PUSHBUTTON) 
>
>/**
> * Represents a check box control. It is used for xul:checkbox,
> * html:input@type="checkbox", role="checkbox", boolean xforms:input.
> */
>ROLE(CHECKBUTTON,
>	44,
>	ATK_ROLE_CHECK_BOX,
>	NSAccessibilityCheckBoxRole,
>	ROLE_SYSTEM_CHECKBUTTON,
>	ROLE_SYSTEM_CHECKBUTTON) 
>
>/**
> * Represents an option button, also called a radio button. It is one of a
> * group of mutually exclusive options. All objects sharing a single parent
> * that have this attribute are assumed to be part of single mutually
> * exclusive group. It is used for xul:radio, html:input@type="radio",
> * role="radio".
> */
>ROLE(RADIOBUTTON,
>	45,
>	ATK_ROLE_RADIO_BUTTON,
>	NSAccessibilityRadioButtonRole,
>	ROLE_SYSTEM_RADIOBUTTON,
>	ROLE_SYSTEM_RADIOBUTTON) 
>
>/**
> * Represents a combo box; an edit control with an associated list box that
> * provides a set of predefined choices. It is used for html:select,
> * xul:menulist, role="combobox".
> */
>ROLE(COMBOBOX,
>	46,
>	ATK_ROLE_COMBO_BOX,
>	NSAccessibilityPopUpButtonRole,
>	ROLE_SYSTEM_COMBOBOX,
>	ROLE_SYSTEM_COMBOBOX) 
>
>/**
> * Represents the calendar control. It is used for date xforms:input.
> */
>ROLE(DROPLIST,
>	47,
>	ATK_ROLE_COMBO_BOX,
>	NSAccessibilityPopUpButtonRole,	//
>	ROLE_SYSTEM_DROPLIST,
>	ROLE_SYSTEM_DROPLIST) 
>
>/**
> * Represents a progress bar, dynamically showing the user the percent
> * complete of an operation in progress. It is used for xul:progressmeter,
> * role="progressbar".
> */
>ROLE(PROGRESSBAR,
>	48,
>	ATK_ROLE_PROGRESS_BAR,
>	NSAccessibilityProgressIndicatorRole,
>	ROLE_SYSTEM_PROGRESSBAR,
>	ROLE_SYSTEM_PROGRESSBAR) 
>
>/**
> * Represents a dial or knob whose purpose is to allow a user to set a value.
> */
>ROLE(DIAL,
>	49,
>	ATK_ROLE_DIAL,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_DIAL,
>	ROLE_SYSTEM_DIAL) 
>
>/**
> * Represents a hot-key field that allows the user to enter a combination or
> * sequence of keystrokes.
> */
>ROLE(HOTKEYFIELD,
>	50,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_HOTKEYFIELD,
>	ROLE_SYSTEM_HOTKEYFIELD) 
>
>/**
> * Represents a slider, which allows the user to adjust a setting in given
> * increments between minimum and maximum values. It is used by xul:scale,
> * role="slider", xforms:range.
> */
>ROLE(SLIDER,
>	51,
>	ATK_ROLE_SLIDER,
>	NSAccessibilitySliderRole,
>	ROLE_SYSTEM_SLIDER,
>	ROLE_SYSTEM_SLIDER) 
>
>/**
> * Represents a spin box, which is a control that allows the user to increment
> * or decrement the value displayed in a separate "buddy" control associated
> * with the spin box. It is used for xul:spinbuttons.
> */
>ROLE(SPINBUTTON,
>	52,
>	ATK_ROLE_SPIN_BUTTON,
>	NSAccessibilityIncrementorRole,	//subroles: Increment/Decrement.
>	ROLE_SYSTEM_SPINBUTTON,
>	ROLE_SYSTEM_SPINBUTTON) 
>
>/**
> * Represents a graphical image used to diagram data. It is used for svg:svg.
> */
>ROLE(DIAGRAM,
>	53,
>	ATK_ROLE_IMAGE,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_DIAGRAM,
>	ROLE_SYSTEM_DIAGRAM) 
>
>/**
> * Represents an animation control, which contains content that changes over
> * time, such as a control that displays a series of bitmap frames.
> */
>ROLE(ANIMATION,
>	54,
>	ATK_ROLE_ANIMATION,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_ANIMATION,
>	ROLE_SYSTEM_ANIMATION) 
>
>/**
> * Represents a mathematical equation. It is used by MATHML, where there is a
> * rich DOM subtree for an equation. Use FLAT_EQUATION for <img role="math" alt="[TeX]"/>
> */
>ROLE(EQUATION,
>	55,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_EQUATION,
>	ROLE_SYSTEM_EQUATION) 
>
>/**
> * Represents a button that drops down a list of items.
> */
>ROLE(BUTTONDROPDOWN,
>	56,
>	ATK_ROLE_PUSH_BUTTON,
>	NSAccessibilityPopUpButtonRole,	//
>	ROLE_SYSTEM_BUTTONDROPDOWN,
>	ROLE_SYSTEM_BUTTONDROPDOWN) 
>
>/**
> * Represents a button that drops down a menu.
> */
>ROLE(BUTTONMENU,
>	57,
>	ATK_ROLE_PUSH_BUTTON,
>	NSAccessibilityMenuButtonRole,
>	ROLE_SYSTEM_BUTTONMENU,
>	ROLE_SYSTEM_BUTTONMENU) 
>
>/**
> * Represents a button that drops down a grid. It is used for xul:colorpicker.
> */
>ROLE(BUTTONDROPDOWNGRID,
>	58,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_BUTTONDROPDOWNGRID,
>	ROLE_SYSTEM_BUTTONDROPDOWNGRID) 
>
>/**
> * Represents blank space between other objects.
> */
>ROLE(WHITESPACE,
>	59,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_WHITESPACE,
>	ROLE_SYSTEM_WHITESPACE) 
>
>/**
> * Represents a container of page tab controls. Is it used for xul:tabs,
> * DHTML: role="tabs". Also refer to PAGETAB.
> */
>ROLE(PAGETABLIST,
>	60,
>	ATK_ROLE_PAGE_TAB_LIST,
>	NSAccessibilityTabGroupRole,
>	ROLE_SYSTEM_PAGETABLIST,
>	ROLE_SYSTEM_PAGETABLIST) 
>
>/**
> * Represents a control that displays time.
> */
>ROLE(CLOCK,
>	61,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,	//unused on OS X
>	ROLE_SYSTEM_CLOCK,
>	ROLE_SYSTEM_CLOCK) 
>
>/**
> * Represents a button on a toolbar that has a drop-down list icon directly
> * adjacent to the button.
> */
>ROLE(SPLITBUTTON,
>	62,
>	ATK_ROLE_PUSH_BUTTON,
>	NSAccessibilityButtonRole,
>	ROLE_SYSTEM_SPLITBUTTON,
>	ROLE_SYSTEM_SPLITBUTTON) 
>
>/**
> * Represents an edit control designed for an Internet Protocol (IP) address.
> * The edit control is divided into sections for the different parts of the
> * IP address.
> */
>ROLE(IPADDRESS,
>	63,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_IPADDRESS,
>	ROLE_SYSTEM_IPADDRESS) 
>
>/**
> * Represents a label control that has an accelerator.
> */
>ROLE(ACCEL_LABEL,
>	64,
>	ATK_ROLE_ACCEL_LABEL,
>	NSAccessibilityStaticTextRole,
>	ROLE_SYSTEM_STATICTEXT,
>	ROLE_SYSTEM_STATICTEXT) 
>
>/**
> * Represents an arrow in one of the four cardinal directions.
> */
>ROLE(ARROW,
>	65,
>	ATK_ROLE_ARROW,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_INDICATOR,
>	ROLE_SYSTEM_INDICATOR) 
>
>/**
> * Represents a control that can be drawn into and is used to trap events.
> * It is used for html:canvas.
> */
>ROLE(CANVAS,
>	66,
>	ATK_ROLE_CANVAS,
>	NSAccessibilityImageRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_CANVAS) 
>
>/**
> * Represents a menu item with a check box.
> */
>ROLE(CHECK_MENU_ITEM,
>	67,
>	ATK_ROLE_CHECK_MENU_ITEM,
>	NSAccessibilityMenuItemRole,
>	ROLE_SYSTEM_MENUITEM,
>	IA2_ROLE_CHECK_MENU_ITEM) 
>
>/**
> * Represents a specialized dialog that lets the user choose a color.
> */
>ROLE(COLOR_CHOOSER,
>	68,
>	ATK_ROLE_COLOR_CHOOSER,
>	NSAccessibilityColorWellRole,
>	ROLE_SYSTEM_DIALOG,
>	IA2_ROLE_COLOR_CHOOSER) 
>
>/**
> * Represents control whose purpose is to allow a user to edit a date.
> */
>ROLE(DATE_EDITOR,
>	69,
>	ATK_ROLE_DATE_EDITOR,
>	NSAccessibilityUnknownRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_DATE_EDITOR) 
>
>/**
> * An iconified internal frame in an DESKTOP_PANE. Also refer to
> * INTERNAL_FRAME.
> */
>ROLE(DESKTOP_ICON,
>	70,
>	ATK_ROLE_DESKTOP_ICON,
>	NSAccessibilityImageRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_DESKTOP_ICON) 
>
>/**
> * A desktop pane. A pane that supports internal frames and iconified
> * versions of those internal frames.
> */
>ROLE(DESKTOP_FRAME,
>	71,
>	ATK_ROLE_DESKTOP_FRAME,
>	NSAccessibilityUnknownRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_DESKTOP_PANE) 
>
>/**
> * A directory pane. A pane that allows the user to navigate through
> * and select the contents of a directory. May be used by a file chooser.
> * Also refer to FILE_CHOOSER.
> */
>ROLE(DIRECTORY_PANE,
>	72,
>	ATK_ROLE_DIRECTORY_PANE,
>	NSAccessibilityBrowserRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_DIRECTORY_PANE) 
>
>/**
> * A file chooser. A specialized dialog that displays the files in the
> * directory and lets the user select a file, browse a different directory,
> * or specify a filename. May use the directory pane to show the contents of
> * a directory. Also refer to DIRECTORY_PANE.
> */
>ROLE(FILE_CHOOSER,
>	73,
>	ATK_ROLE_FILE_CHOOSER,
>	NSAccessibilityUnknownRole,	//unused on OS X
>	USE_ROLE_STRING,
>	IA2_ROLE_FILE_CHOOSER) 
>
>/**
> * A font chooser. A font chooser is a component that lets the user pick
> * various attributes for fonts.
> */
>ROLE(FONT_CHOOSER,
>	74,
>	ATK_ROLE_FONT_CHOOSER,
>	NSAccessibilityUnknownRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_FONT_CHOOSER) 
>
>/**
> * Frame role. A top level window with a title bar, border, menu bar, etc.
> * It is often used as the primary window for an application.
> */
>ROLE(CHROME_WINDOW,
>	75,
>	ATK_ROLE_FRAME,
>	NSAccessibilityUnknownRole,	//unused on OS X
>	ROLE_SYSTEM_APPLICATION,
>	IA2_ROLE_FRAME) 
>
>/**
> *  A glass pane. A pane that is guaranteed to be painted on top of all
> * panes beneath it. Also refer to ROOT_PANE.
> */
>ROLE(GLASS_PANE,
>	76,
>	ATK_ROLE_GLASS_PANE,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_GLASS_PANE) 
>
>/**
> * A document container for HTML, whose children represent the document
> * content.
> */
>ROLE(HTML_CONTAINER,
>	77,
>	ATK_ROLE_HTML_CONTAINER,
>	NSAccessibilityUnknownRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_UNKNOWN) 
>
>/**
> * A small fixed size picture, typically used to decorate components.
> */
>ROLE(ICON,
>	78,
>	ATK_ROLE_ICON,
>	NSAccessibilityImageRole,
>	ROLE_SYSTEM_PUSHBUTTON,
>	IA2_ROLE_ICON) 
>
>/**
> * Presents an icon or short string in an interface.
> */
>ROLE(LABEL,
>	79,
>	ATK_ROLE_LABEL,
>	NSAccessibilityStaticTextRole,
>	ROLE_SYSTEM_STATICTEXT,
>	IA2_ROLE_LABEL) 
>
>/**
> * A layered pane. A specialized pane that allows its children to be drawn
> * in layers, providing a form of stacking order. This is usually the pane
> * that holds the menu bar as  well as the pane that contains most of the
> * visual components in a window. Also refer to GLASS_PANE and
> * ROOT_PANE.
> */
>ROLE(LAYERED_PANE,
>	80,
>	ATK_ROLE_LAYERED_PANE,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_LAYERED_PANE) 
>
>/**
> * A specialized pane whose primary use is inside a dialog.
> */
>ROLE(OPTION_PANE,
>	81,
>	ATK_ROLE_OPTION_PANE,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_OPTION_PANE) 
>
>/**
> * A text object uses for passwords, or other places where the text content
> * is not shown visibly to the user.
> */
>ROLE(PASSWORD_TEXT,
>	82,
>	ATK_ROLE_PASSWORD_TEXT,
>	NSAccessibilityTextFieldRole,
>	ROLE_SYSTEM_TEXT,
>	ROLE_SYSTEM_TEXT) 
>
>/**
> * A temporary window that is usually used to offer the user a list of
> * choices, and then hides when the user selects one of those choices.
> */
>ROLE(POPUP_MENU,
>	83,
>	ATK_ROLE_POPUP_MENU,
>	NSAccessibilityUnknownRole,	//unused
>	ROLE_SYSTEM_MENUPOPUP,
>	ROLE_SYSTEM_MENUPOPUP) 
>
>/**
> * A radio button that is a menu item.
> */
>ROLE(RADIO_MENU_ITEM,
>	84,
>	ATK_ROLE_RADIO_MENU_ITEM,
>	NSAccessibilityMenuItemRole,
>	ROLE_SYSTEM_MENUITEM,
>	IA2_ROLE_RADIO_MENU_ITEM) 
>
>/**
> * A root pane. A specialized pane that has a glass pane and a layered pane
> * as its children. Also refer to GLASS_PANE and LAYERED_PANE.
> */
>ROLE(ROOT_PANE,
>	85,
>	ATK_ROLE_ROOT_PANE,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_ROOT_PANE) 
>
>/**
> * A scroll pane. An object that allows a user to incrementally view a large
> * amount of information.  Its children can include scroll bars and a
> * viewport. Also refer to VIEW_PORT.
> */
>ROLE(SCROLL_PANE,
>	86,
>	ATK_ROLE_SCROLL_PANE,
>	NSAccessibilityScrollAreaRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_SCROLL_PANE) 
>
>/**
> * A split pane. A specialized panel that presents two other panels at the
> * same time. Between the two panels is a divider the user can manipulate to
> * make one panel larger and the other panel smaller.
> */
>ROLE(SPLIT_PANE,
>	87,
>	ATK_ROLE_SPLIT_PANE,
>	NSAccessibilitySplitGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_SPLIT_PANE) 
>
>/**
> * The header for a column of a table.
> * XXX: it looks this role is dupe of COLUMNHEADER.
> */
>ROLE(TABLE_COLUMN_HEADER,
>	88,
>	ATK_ROLE_TABLE_COLUMN_HEADER,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_COLUMNHEADER,
>	ROLE_SYSTEM_COLUMNHEADER) 
>
>/**
> * The header for a row of a table.
> * XXX: it looks this role is dupe of ROWHEADER
> */
>ROLE(TABLE_ROW_HEADER,
>	89,
>	ATK_ROLE_TABLE_ROW_HEADER,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_ROWHEADER,
>	ROLE_SYSTEM_ROWHEADER) 
>
>/**
> * A menu item used to tear off and reattach its menu.
> */
>ROLE(TEAR_OFF_MENU_ITEM,
>	90,
>	ATK_ROLE_TEAR_OFF_MENU_ITEM,
>	NSAccessibilityMenuItemRole,
>	ROLE_SYSTEM_MENUITEM,
>	IA2_ROLE_TEAR_OFF_MENU) 
>
>/**
> * Represents an accessible terminal.
> */
>ROLE(TERMINAL,
>	91,
>	ATK_ROLE_TERMINAL,
>	NSAccessibilityUnknownRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_TERMINAL) 
>
>/**
> * Collection of objects that constitute a logical text entity.
> */
>ROLE(TEXT_CONTAINER,
>	92,
>	ATK_ROLE_TEXT,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_TEXT_FRAME) 
>
>/**
> * A toggle button. A specialized push button that can be checked or
> * unchecked, but does not provide a separate indicator for the current state.
> */
>ROLE(TOGGLE_BUTTON,
>	93,
>	ATK_ROLE_TOGGLE_BUTTON,
>	NSAccessibilityButtonRole,
>	ROLE_SYSTEM_PUSHBUTTON,
>	IA2_ROLE_TOGGLE_BUTTON) 
>
>/**
> * Representas a control that is capable of expanding and collapsing rows as
> * well as showing multiple columns of data.
> * XXX: it looks like this role is dupe of OUTLINE.
> */
>ROLE(TREE_TABLE,
>	94,
>	ATK_ROLE_TREE_TABLE,
>	NSAccessibilityTableRole,
>	ROLE_SYSTEM_OUTLINE,
>	ROLE_SYSTEM_OUTLINE) 
>
>/**
> * A viewport. An object usually used in a scroll pane. It represents the
> * portion of the entire data that the user can see. As the user manipulates
> * the scroll bars, the contents of the viewport can change. Also refer to
> * SCROLL_PANE.
> */
>ROLE(VIEWPORT,
>	95,
>	ATK_ROLE_VIEWPORT,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_PANE,
>	IA2_ROLE_VIEW_PORT) 
>
>/**
> * Header of a document page. Also refer to FOOTER.
> */
>ROLE(HEADER,
>	96,
>	ATK_ROLE_HEADER,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_HEADER) 
>
>/**
> * Footer of a document page. Also refer to HEADER.
> */
>ROLE(FOOTER,
>	97,
>	ATK_ROLE_FOOTER,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_FOOTER) 
>
>/**
> * A paragraph of text.
> */
>ROLE(PARAGRAPH,
>	98,
>	ATK_ROLE_PARAGRAPH,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_PARAGRAPH) 
>
>/**
> * A ruler such as those used in word processors.
> */
>ROLE(RULER,
>	99,
>	ATK_ROLE_RULER,
>	@"AXRuler",	//10.4+ only, so we re-define the constant.
>	USE_ROLE_STRING,
>	IA2_ROLE_RULER) 
>
>/**
> * A text entry having dialog or list containing items for insertion into
> * an entry widget, for instance a list of words for completion of a
> * text entry. It is used for xul:textbox@autocomplete
> */
>ROLE(AUTOCOMPLETE,
>	100,
>	ATK_ROLE_AUTOCOMPLETE,
>	NSAccessibilityComboBoxRole,
>	ROLE_SYSTEM_COMBOBOX,
>	ROLE_SYSTEM_COMBOBOX) 
>
>/**
> *  An editable text object in a toolbar.
> */
>ROLE(EDITBAR,
>	101,
>	ATK_ROLE_EDITBAR,
>	NSAccessibilityTextFieldRole,
>	ROLE_SYSTEM_TEXT,
>	IA2_ROLE_EDITBAR) 
>
>/**
> * An control whose textual content may be entered or modified by the user.
> */
>ROLE(ENTRY,
>	102,
>	ATK_ROLE_ENTRY,
>	NSAccessibilityTextFieldRole,
>	ROLE_SYSTEM_TEXT,
>	ROLE_SYSTEM_TEXT) 
>
>/**
> * A caption describing another object.
> */
>ROLE(CAPTION,
>	103,
>	ATK_ROLE_CAPTION,
>	NSAccessibilityStaticTextRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_CAPTION) 
>
>/**
> * A visual frame or container which contains a view of document content.
> * Document frames may occur within another Document instance, in which case
> * the second document may be said to be embedded in the containing instance.
> * HTML frames are often DOCUMENT_FRAME. Either this object, or a
> * singleton descendant, should implement the Document interface.
> */
>ROLE(DOCUMENT_FRAME,
>	104,
>	ATK_ROLE_DOCUMENT_FRAME,
>	NSAccessibilityScrollAreaRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_UNKNOWN) 
>
>/**
> * Heading.
> */
>ROLE(HEADING,
>	105,
>	ATK_ROLE_HEADING,
>	@"AXHeading",
>	USE_ROLE_STRING,
>	IA2_ROLE_HEADING) 
>
>/**
> * An object representing a page of document content.  It is used in documents
> * which are accessed by the user on a page by page basis.
> */
>ROLE(PAGE,
>	106,
>	ATK_ROLE_PAGE,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_PAGE) 
>
>/**
> * A container of document content.  An example of the use of this role is to
> * represent an html:div.
> */
>ROLE(SECTION,
>	107,
>	ATK_ROLE_SECTION,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_SECTION) 
>
>/**
> * An object which is redundant with another object in the accessible
> * hierarchy. ATs typically ignore objects with this role.
> */
>ROLE(REDUNDANT_OBJECT,
>	108,
>	ATK_ROLE_REDUNDANT_OBJECT,
>	NSAccessibilityUnknownRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_REDUNDANT_OBJECT) 
>
>/**
> * A container of form controls. An example of the use of this role is to
> * represent an html:form.
> */
>ROLE(FORM,
>	109,
>	ATK_ROLE_FORM,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_FORM) 
>
>/**
> * An object which is used to allow input of characters not found on a
> * keyboard, such as the input of Chinese characters on a Western keyboard.
> */
>ROLE(IME,
>	110,
>	ATK_ROLE_INPUT_METHOD_WINDOW,
>	NSAccessibilityUnknownRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_INPUT_METHOD_WINDOW) 
>
>/**
> * XXX: document this.
> */
>ROLE(APP_ROOT,
>	111,
>	ATK_ROLE_APPLICATION,
>	NSAccessibilityUnknownRole,	//unused on OS X
>	ROLE_SYSTEM_APPLICATION,
>	ROLE_SYSTEM_APPLICATION) 
>
>/**
> * Represents a menu item, which is an entry in a menu that a user can choose
> * to display another menu.
> */
>ROLE(PARENT_MENUITEM,
>	112,
>	ATK_ROLE_MENU,
>	NSAccessibilityMenuItemRole,
>	ROLE_SYSTEM_MENUITEM,
>	ROLE_SYSTEM_MENUITEM) 
>
>/**
> * A calendar that allows the user to select a date.
> */
>ROLE(CALENDAR,
>	113,
>	ATK_ROLE_CALENDAR,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_CLIENT,
>	ROLE_SYSTEM_CLIENT) 
>
>/**
> * A list of items that is shown by combobox.
> */
>ROLE(COMBOBOX_LIST,
>	114,
>	ATK_ROLE_MENU,
>	NSAccessibilityMenuRole,
>	ROLE_SYSTEM_LIST,
>	ROLE_SYSTEM_LIST) 
>
>/**
> * A item of list that is shown by combobox.
> */
>ROLE(COMBOBOX_OPTION,
>	115,
>	ATK_ROLE_MENU_ITEM,
>	NSAccessibilityMenuItemRole,
>	ROLE_SYSTEM_LISTITEM,
>	ROLE_SYSTEM_LISTITEM) 
>
>/**
> * An image map -- has child links representing the areas
> */
>ROLE(IMAGE_MAP,
>	116,
>	ATK_ROLE_IMAGE,
>	NSAccessibilityImageRole,
>	ROLE_SYSTEM_GRAPHIC,
>	ROLE_SYSTEM_GRAPHIC) 
>
>/**
> * An option in a listbox
> */
>ROLE(OPTION,
>	117,
>	ATK_ROLE_LIST_ITEM,
>	NSAccessibilityRowRole,
>	ROLE_SYSTEM_LISTITEM,
>	ROLE_SYSTEM_LISTITEM) 
>
>/**
> * A rich option in a listbox, it can have other widgets as children
> */
>ROLE(RICH_OPTION,
>	118,
>	ATK_ROLE_LIST_ITEM,
>	NSAccessibilityRowRole,
>	ROLE_SYSTEM_LISTITEM,
>	ROLE_SYSTEM_LISTITEM) 
>
>/**
> * A list of options
> */
>ROLE(LISTBOX,
>	119,
>	ATK_ROLE_LIST,
>	NSAccessibilityListRole,
>	ROLE_SYSTEM_LIST,
>	ROLE_SYSTEM_LIST) 
>
>/**
> * Represents a mathematical equation in the accessible name
> */
>ROLE(FLAT_EQUATION,
>	120,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,
>	ROLE_SYSTEM_EQUATION,
>	ROLE_SYSTEM_EQUATION) 
>
>/**
> * Represents a cell within a grid. It is used for role="gridcell". Unlike
> * CELL, it allows the calculation of the accessible name from subtree.
> * Also, see TABLE.
> */
>ROLE(GRID_CELL,
>	121,
>	ATK_ROLE_TABLE_CELL,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_CELL,
>	ROLE_SYSTEM_CELL) 
>
>/**
> * Represents an embedded object. It is used for html:object or html:embed.
> */
>ROLE(EMBEDDED_OBJECT,
>	122,
>	ATK_ROLE_PANEL,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_EMBEDDED_OBJECT) 
>
>/**
> * A note. Originally intended to be hidden until activated, but now also used
> * for things like html 'aside'.
> */
>ROLE(NOTE,
>	123,
>	ATK_ROLE_SECTION,
>	NSAccessibilityGroupRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_NOTE) 
>
>/**
> * A figure. Used for things like HTML5 figure element.
> */
>ROLE(FIGURE,
>	124,
>	ATK_ROLE_PANEL,
>	NSAccessibilityGroupRole,
>	ROLE_SYSTEM_GROUPING,
>	ROLE_SYSTEM_GROUPING) 
>
>/**
> * It's not role actually. This constant is important to help ensure
> * nsRoleMap's are synchronized.
> */
>ROLE(LAST_ENTRY,
>	125,
>	kROLE_ATK_LAST_ENTRY          // roles::LAST_ENTRY			 12,
>	@"ROLE_LAST_ENTRY"            // ROLE_LAST_ENTRY. bogus role that will never be shown (just marks the end of this array)
>	ROLE_WINDOWS_LAST_ENTRY,
>	ROLE_WINDOWS_LAST_ENTRY)
Comment 22 alexander :surkov 2012-04-13 04:40:23 PDT
how about to not general Role.h from that map? i.e. Role.h defines role constants and RoleMap.h takes care to map them into platform roles and string representation?
Comment 23 alexander :surkov 2012-04-13 04:41:09 PDT
Comment on attachment 614463 [details]
proposed RoleMap.h file

>ROLE(NOTHING,
>	0,
>	ATK_ROLE_UNKNOWN,
>	NSAccessibilityUnknownRole,
>	USE_ROLE_STRING,
>	IA2_ROLE_UNKNOWN) 

btw, wrong indentation
Comment 24 alexander :surkov 2012-04-13 04:41:48 PDT
btw, please file a patch instead of file
Comment 25 Andrzej Skalski 2012-04-13 06:27:25 PDT
(In reply to alexander :surkov from comment #22)
> how about to not general Role.h from that map? i.e. Role.h defines role
> constants and RoleMap.h takes care to map them into platform roles and
> string representation?

Doable. So two files, right?
Comment 26 alexander :surkov 2012-04-13 06:55:34 PDT
Let's get feedback from Trevor since this bug was his idea orignally.
Comment 27 Andrzej Skalski 2012-04-23 11:07:38 PDT
Created attachment 617548 [details] [diff] [review]
patch proposition
Comment 28 Trevor Saunders (:tbsaunde) 2012-04-23 17:53:48 PDT
Comment on attachment 617548 [details] [diff] [review]
patch proposition

>-// Map array from cross platform roles to  ATK roles
>+// Map array from cross platform roles to  ATK roles, expanded from macro defined in ../base/RoleMap.h

nits, base/RoleMap.h seems fine, we also usually do

/**
 * blah blah
 */

for comments in headers.

> static const PRUint32 atkRoleMap[] = {

Why not convert this to a function something like this

ATKRole
AtkRoleFor(role aRole)
{
  switch(aRole) {
  #define ROLE(geckoRole, atkRole, ...) case geckoRole: return atkRole;
  #include RoleMap.h
  undef ROLE
  }
}

Ideally we could have AtkRole() on nsAccessibleWrap, but we need to clean up nsMaiInterfaceText.cpp first.

>--- a/accessible/src/base/Makefile.in
>+++ b/accessible/src/base/Makefile.in
>@@ -92,6 +92,7 @@
>   FocusManager.h \
>   States.h \
>   Role.h \
>+  RoleMap.h \

why do you need to export it?

>+ * macro:
>+ * Role base name
>+ * Atk role name
>+ * OS X NSAccessibility
>+ * MSAA role
>+ * IA2 role
>+ *
>+ * ROLE(GeckoRole, AtkRole, NSARole, MSAARole, IA2Role)
>+ */

this comment is kind of funny, I'd suggest something like

/**
 * users are expected to declare the macro ROLE() with the following arguments.
 * ROLE(geckoRole, atkRole, ...)
 */

it'd be nice if you could include the string role from kRoleNames here too.

>+/**
>+ * It's not role actually. This constant is important to help ensure
>+ * nsRoleMap's are synchronized.
>+ */
>+ROLE(LAST_ENTRY,
>+  kROLE_ATK_LAST_ENTRY,
>+  @"ROLE_LAST_ENTRY",  // ROLE_LAST_ENTRY. bogus role that will never be shown (just marks the end of this array),  //bogus role that will never be shown (just marks the end of this array)!
>+  ROLE_WINDOWS_LAST_ENTRY,
>+  ROLE_WINDOWS_LAST_ENTRY)

I don't think there's any need for this anymore, so you should be able to remove it.

>+#define ROLE(GeckoRole, AtkRole, NSARole, MSAARole, IA2Role) NSARole,

nit, MACRole or similar, NSARole is funny and I don't know what it means

>+
>+long nsAccessibleWrap::sMSAARoles[] = {

I'd probably do the same as for atk and make this a function that is just a big autogenerated switch.

putting this in nsAccessibleWrap feels kind of funny, but ok.  Alex thoughts on putting this in a utils place of some sort?  on the other hand maybe we should just leave it on nsAccessibleWrap and it'll make more sense when we have seperate platform objects.

>+  PRUint32 msaaRole = sMSAARoles[role];

wrong type

>+  NS_ASSERTION(sMSAARoles[roles::LAST_ENTRY] == ROLE_WINDOWS_LAST_ENTRY,
>                "MSAA role map skewed");

pointless now so get rid of it and similar asserts.

>+private:

we usually only have one public protected / private section per class.

>+  // MSAA role map. Initialization in nsAccessibleWrap.cpp

kind of silly to say where initialized, if its in the obvious place.

>+  long MSAARole() { return sMSAARoles[Role()]; }
>+  long IARole() { return sIA2Roles[Role()]; }

never used.

> #include "OLEACC.H"
> #include "AccessibleRole.h"
> 
>+/*
>+ * The real role map have been moved to nsAccessibleWrap.h, as suggested by Alexander Surkov in
>+ * https://bugzilla.mozilla.org/show_bug.cgi?id=716644#c13
>+ */

probably makes more sense to just rm this file

>+
> const PRUint32 USE_ROLE_STRING = 0;

define it near other role stuffs.

> const PRUint32 ROLE_WINDOWS_LAST_ENTRY = 0xffffffff;

unused now right? so rm it

>@@ -57,402 +66,5 @@
> const PRUint32 ROLE_SYSTEM_OUTLINEBUTTON = 0x40; // Not defined in all oleacc.h versions

I wonder if that's still true for versions we care about, could you try removing these or checking on msdn?
Comment 29 Trevor Saunders (:tbsaunde) 2012-04-23 17:56:30 PDT
it'd also be nice if you added static asserts asserting roles::foo == nsIAccessibleRole::ROLE_FOO
Comment 30 alexander :surkov 2012-04-24 02:40:36 PDT
Comment on attachment 617548 [details] [diff] [review]
patch proposition

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

::: accessible/src/atk/nsRoleMap.h
@@ +46,4 @@
>  static const PRUint32 atkRoleMap[] = {
> +#define ROLE(GeckoRole, AtkRole, NSARole, MSAARole, IA2Role) AtkRole,
> +#include "RoleMap.h"
> +#undef ROLE

if you don't want to do similar things like you did for MSAA then please file follow ups

::: accessible/src/base/RoleMap.h
@@ +19,5 @@
> +ROLE(NOTHING,
> +  ATK_ROLE_UNKNOWN,
> +  NSAccessibilityUnknownRole,
> +  USE_ROLE_STRING,
> +  IA2_ROLE_UNKNOWN) 

I think you don't need to comment them because we don't touch Role.h, it's just a map after all.

please indent them like

ROLE(NOTHING,
     ATK_ROLE_UNKNOWN)

::: accessible/src/msaa/nsAccessibleWrap.h
@@ +341,5 @@
>    void UnattachIEnumVariant();
>  
> +private:
> +  // MSAA role map. Initialization in nsAccessibleWrap.cpp
> +  static long sMSAARoles [];

you don't need space before []

@@ +348,5 @@
> +  static long sIA2Roles [];
> +
> +public:
> +  long MSAARole() { return sMSAARoles[Role()]; }
> +  long IARole() { return sIA2Roles[Role()]; }

in general I prefer to not switch between public and private sections, public on top, private on bottom
Comment 31 alexander :surkov 2012-04-24 04:51:19 PDT
> >+
> >+long nsAccessibleWrap::sMSAARoles[] = {
> 
> I'd probably do the same as for atk and make this a function that is just a
> big autogenerated switch.

big switch probably won't get ever inlined.

> putting this in nsAccessibleWrap feels kind of funny, but ok.  Alex thoughts
> on putting this in a utils place of some sort?  on the other hand maybe we
> should just leave it on nsAccessibleWrap and it'll make more sense when we
> have seperate platform objects.

we could keep it on nsAccessibleWrap, role is property of accessible object so keeping the mapping there should be ok. shrugs
Comment 32 Andrzej Skalski 2012-04-24 05:08:23 PDT
Created attachment 617841 [details] [diff] [review]
Second patch proposition

Some of Trevor's suggestions applied, namely all but:
- haven't changed arrays to switches, as we need to discuss it yet
- not removed assertions (yet)
- some code cleaning
Comment 33 Trevor Saunders (:tbsaunde) 2012-04-24 06:54:09 PDT
08:34 <@tbsaunde|afk> surkov: well, inlining will depend on what code the compiler decides to generate, a really quick test seemed to show gcc created a test / jump for some easy cases, and then used an array if that failed
08:35 <@surkov> tbsaunde|afk: ok but why do you prefer inline over array?
08:35 -!- You're now known as tbsaunde
08:35 -!- shorlande [shorlander@F2D29657.F60B0462.67AC9B1.IP] has joined #accessibility
08:35 <@surkov> and do we want to depend on compiler decision in this case
08:35 <@tbsaunde> surkov: mostly since it gives the compiler more freedom in how to do the mapping
08:36 <@surkov> what's benefits of freedom vs having an array?
08:36 <@tbsaunde> is there a reason we think we know better than it?
08:37 -!- jhk [jiggy@8E6C34C1.A3F9767A.1C37C358.IP] has joined #accessibility
08:37 <@tbsaunde> suppose there is a mathematica function that's faster than always going to memory?
08:38 <@tbsaunde> what's the benefit of an array?
08:38 <@tbsaunde> also, it looked like the internal array used one byte blocks rather than 4, so slightly less static data
08:38 <@surkov> I thought it's fast as possible
08:39 <@surkov> and it doesn't eat memory
08:39 <@tbsaunde> that's kind of unclear, I wouldn't necessarily expect that array to be in cache
08:40 <@surkov> cache of what?
08:40 <@tbsaunde> memory
08:40 <@surkov> I thought once it was created it's kept in memory
08:41 <@surkov> can it create/destroy it while program is running?
08:41 <@tbsaunde> sure, but memory is slow, so the cpu has a cache of it
08:41 <@surkov> ok
08:41 <@surkov> so, if we have a switch then compiler generates more code?
08:42 <@tbsaunde> maybe maybe not
08:42 <@surkov> ok, is there any best practices when to use static array and when to use switch?
08:42 <@tbsaunde> and generating more code may be good or may be bad depending on what that code is
08:42 -!- shorlande [shorlander@F2D29657.F60B0462.67AC9B1.IP] has quit [Connection reset by peer]
08:43 -!- shorlande [shorlander@F2D29657.F60B0462.67AC9B1.IP] has joined #accessibility
08:43 <@surkov> it's the area I don't know much about
08:43 <@tbsaunde> I can't think of a reason to use a static array in this case other than
08:43 <@tbsaunde> 1 you've profiled the code is hot and the compiler is being dumb
08:44 <@tbsaunde> or you want to do something more complicated with the array than just a one way mapping
08:45 <@surkov> tbsaunde: did you said that compiler can end up with array generation for switch case?
08:45 <@tbsaunde> but I don't think this code is terribly important perf wise, and I haven't profiled / ooked at that much compiler output
08:45 <@tbsaunde> it seems atleast gcc can on a small test case
08:45 <@surkov> if it's not perf important then we could stay on array approach :)
08:46 <@surkov> so you think let's a compiler to decide ?
08:46 <@tbsaunde> true, but why?
08:46 <@tbsaunde> yeah, why not
08:47 <@surkov> I'm just conservative, to change something I need a good reason :)
08:47 <@surkov> ok, if you like I'm fine with that
08:47 <@surkov> tbsaunde: please put summary of this discussion to the bug for history
08:47 <@tbsaunde> I also sort of like it since its a mapping / function from role to platform role so function  seems more sense than array
08:47 <@tbsaunde> sure
08:54 -!- peteb-away [ptbrunet@moz-E9B02845.austin.res.rr.com] has joined #accessibility
Comment 34 Andrzej Skalski 2012-04-24 06:57:53 PDT
Ok, so I understand Trevor wants it and Alex is OK with that. I will send the implementation soon.
Comment 35 alexander :surkov 2012-04-24 07:00:34 PDT
(In reply to Andrzej Skalski from comment #34)
> Ok, so I understand Trevor wants it and Alex is OK with that. I will send
> the implementation soon.

thank you for reading this :) Trevor treated my words about putting a summary of our chat on irc a little wider than I expected ;)
Comment 36 Trevor Saunders (:tbsaunde) 2012-04-24 16:24:34 PDT
(In reply to alexander :surkov from comment #35)
> (In reply to Andrzej Skalski from comment #34)
> > Ok, so I understand Trevor wants it and Alex is OK with that. I will send
> > the implementation soon.
> 
> thank you for reading this :) Trevor treated my words about putting a
> summary of our chat on irc a little wider than I expected ;)

it was just easier

(In reply to alexander :surkov from comment #31)
> > >+
> > >+long nsAccessibleWrap::sMSAARoles[] = {
> > 
> > I'd probably do the same as for atk and make this a function that is just a
> > big autogenerated switch.
> 
> big switch probably won't get ever inlined.

it occurs to me that we only use each table in one place once nsMaiINterfaceText has sense beaten into it, so we could just put the switch directly in the one place that uses it.

> > putting this in nsAccessibleWrap feels kind of funny, but ok.  Alex thoughts
> > on putting this in a utils place of some sort?  on the other hand maybe we
> > should just leave it on nsAccessibleWrap and it'll make more sense when we
> > have seperate platform objects.
> 
> we could keep it on nsAccessibleWrap, role is property of accessible object
> so keeping the mapping there should be ok. shrugs

ok
Comment 37 Trevor Saunders (:tbsaunde) 2012-04-24 18:32:51 PDT
Comment on attachment 617841 [details] [diff] [review]
Second patch proposition

please address comments, and really do get rid of those asserts, I can't think of a reason we'd want to keep them.
Comment 38 Andrzej Skalski 2012-04-25 06:45:51 PDT
Created attachment 618234 [details] [diff] [review]
Third patch proposition
Comment 39 Andrzej Skalski 2012-04-25 10:26:29 PDT
Created attachment 618346 [details] [diff] [review]
Fourth patch proposition

Adressed all the comments, removed unnecessary LAST_ENTRY role and assertions, got correct indentation, removed empty files etc.

Compiles on Windows and Linux. Please test on Mac.
Comment 40 Trevor Saunders (:tbsaunde) 2012-04-25 18:40:49 PDT
Comment on attachment 618346 [details] [diff] [review]
Fourth patch proposition

>@@ -734,9 +733,7 @@

in future please use -p -u 8 as diff arguments.

>+        PRUint32 atkRole = nsAccessibleWrap::AtkRoleFor(accWrap->Role());
>         aAtkObj->role = static_cast<AtkRole>(atkRole);

why not just make the return type of the function  AtkRole

>     }
>     return aAtkObj->role;
>@@ -1405,3 +1402,23 @@
>     return NS_OK;
> }
> 
>+/**
>+ * Function mapping from cross platform roles to ATK roles, expanded from macro
>+ * defined in base/RoleMap.h. The final return statement is required for

actually its defined a few lines below this...

and nit, in cpp files we use // comments

>+ * -Werror=return-type flag enabled.

this return stuff is kind of silly I think, if you really want to explain it I'd put it right before the return and say something like "saddly compilers are silly and don't catch this is unreachable".

>+ */
>+
>+PRUint32
>+nsAccessibleWrap::AtkRoleFor(mozilla::a11y::role aRole)
>+{
>+  switch(aRole) {
>+#define ROLE(geckoRole, stringRole, atkRole, macRole, msaaRole, ia2Role) \
>+  case mozilla::a11y::role::geckoRole: return atkRole;

you should be able to say roles::geckoRole

>+#include "RoleMap.h"
>+#undef ROLE
>+  }
>+  /**
>+   * This line should never be reached. Should I put assert here?
>+   */

you could use MOZ_NOTREACHED or MOZ_MARK_UNREACHABLE here I think, and then I'd assume smart compilers shouldn't care if you have a return and I doubt we care about warnings on dumb compilers.

>+ * Used when accessible hans't strong defined role.
>+ */
>+ROLE(NOTHING,

I thought Alex only wanted comments in Role.h which seems reasonable to me.

>+/**
>+ * Map nsIAccessibleRole constants to strings. Used by
>+ * nsIAccessibleRetrieval::getStringRole() method.
>+ *
>+ * The array is expanded from macro defined in base/RoleMap.h
>+ */
>+
>+static const char kRoleNames[][20] = {

you could use a switch here too, or atleast make  it static to the one method since its the only consumer.

>@@ -462,7 +458,7 @@
>   NS_ASSERTION(nsAccUtils::IsTextInterfaceSupportCorrect(mGeckoAccessible),
>                "Does not support nsIAccessibleText when it should");
> #endif
>-  return (NSString*) AXRoles[mRole];
>+  return (NSString*) nsAccessibleWrap::MacRoleFor(mRole);

I doubt you still need the cast.

>+const NSString *
>+nsAccessibleWrap::MacRoleFor(mozilla::a11y::role aRole)

why not put it in line at the one caller like on windows?

>+{
>+  switch(aRole) {
>+#define ROLE(geckoRole, stringRole, atkRole, macRole, msaaRole, ia2Role) \
>+  case mozilla::a11y::role::geckoRole: return macRole;

should be in using mozilla::a11y so just roles::geckoRole

>+long
>+nsAccessibleWrap::GetMSAARoleFor(mozilla::a11y::role aRole)
>+{

>+long
>+nsAccessibleWrap::GetIA2RoleFor(mozilla::a11y::role aRole)

inline these too since they only have one caller

>+#ifndef _nsRoleMap_H_
>+#define _nsRoleMap_H_
>+
> #include "OLEACC.H"
> #include "AccessibleRole.h"
> 
>@@ -57,402 +61,4 @@
> const PRUint32 ROLE_SYSTEM_OUTLINEBUTTON = 0x40; // Not defined in all oleacc.h versions
> #endif

if you don't want to get rid of this stuff please file follow ups to get rid of the stuff remaining in this file.

>+#endif
>diff --git a/accessible/src/msaa/nsRootAccessibleWrap.cpp b/accessible/src/msaa/nsRootAccessibleWrap.cpp
>--- a/accessible/src/msaa/nsRootAccessibleWrap.cpp
>+++ b/accessible/src/msaa/nsRootAccessibleWrap.cpp
>@@ -21,6 +21,7 @@
>  *
>  * Contributor(s):
>  *   Alexander Surkov <surkov.alexander@gmail.com> (origianl author)
>+ *   Andrzej Skalski <askalski@mozilla.com>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either of the GNU General Public License Version 2 or later (the "GPL"),

nit, what is this about?

otherwise seems good.
Comment 41 alexander :surkov 2012-04-26 00:34:04 PDT
Comment on attachment 618346 [details] [diff] [review]
Fourth patch proposition

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

use -U 8 -p diff format

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +733,4 @@
>  
>      if (aAtkObj->role == ATK_ROLE_INVALID) {
>          // map to the actual value
> +        PRUint32 atkRole = nsAccessibleWrap::AtkRoleFor(accWrap->Role());

two spaces indentation please while you are here, you can indent whole 'if' block

@@ +1405,5 @@
> +/**
> + * Function mapping from cross platform roles to ATK roles, expanded from macro
> + * defined in base/RoleMap.h. The final return statement is required for
> + * -Werror=return-type flag enabled.
> + */

move the comment into header. btw, what this flag is about and what is for?

@@ +1408,5 @@
> + * -Werror=return-type flag enabled.
> + */
> +
> +PRUint32
> +nsAccessibleWrap::AtkRoleFor(mozilla::a11y::role aRole)

you are under using mozilla::a11y, so (role aRole) please

@@ +1412,5 @@
> +nsAccessibleWrap::AtkRoleFor(mozilla::a11y::role aRole)
> +{
> +  switch(aRole) {
> +#define ROLE(geckoRole, stringRole, atkRole, macRole, msaaRole, ia2Role) \
> +  case mozilla::a11y::role::geckoRole: return atkRole;

I'd prefer to follow the style like:
  case role::geckoRole: \
    return atkRole;

::: accessible/src/atk/nsAccessibleWrap.h
@@ +116,4 @@
>        return returnedString.get();
>      }
>  
> +    static PRUint32 AtkRoleFor(mozilla::a11y::role aRole);

nit: two spaces indent
why you don't want to make it return AtkRole type?

::: accessible/src/base/RoleMap.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * users are expected to declare the macro ROLE() with the following arguments.

u -> U

what they excpected for? and who is a user?

@@ +8,5 @@
> + */
> +
> +/**
> + * Used when accessible hans't strong defined role.
> + */

I don't think you really need keep comments here, since they dupe comments from Role.h

@@ +1392,5 @@
> +     "check rich option",
> +     ATK_ROLE_CHECK_BOX,
> +     NSAccessibilityCheckBoxRole,
> +     ROLE_SYSTEM_CHECKBUTTON,
> +     ROLE_SYSTEM_CHECKBUTTON) 

update to trunk, we have new roles like DEFINITION_LIST

::: accessible/src/base/nsAccessibilityService.cpp
@@ +113,5 @@
> + * nsIAccessibleRetrieval::getStringRole() method.
> + *
> + * The array is expanded from macro defined in base/RoleMap.h
> + */
> +

nit: no empty line

::: accessible/src/mac/mozAccessible.mm
@@ +458,4 @@
>    NS_ASSERTION(nsAccUtils::IsTextInterfaceSupportCorrect(mGeckoAccessible),
>                 "Does not support nsIAccessibleText when it should");
>  #endif
> +  return (NSString*) nsAccessibleWrap::MacRoleFor(mRole);

generate switch here instead

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +375,4 @@
>  #endif
>  
>    roles::Role role = xpAccessible->Role();
> +  PRUint32 msaaRole = GetMSAARoleFor(role);

generate switch instead

@@ +1203,2 @@
>    roles::Role role = Role();
> +  *aRole = GetIA2RoleFor(role);

same

::: accessible/src/msaa/nsRoleMap.h
@@ +61,1 @@
>  const PRUint32 ROLE_SYSTEM_OUTLINEBUTTON = 0x40; // Not defined in all oleacc.h versions

keep these constants where they are used

::: accessible/src/msaa/nsRootAccessibleWrap.cpp
@@ +21,4 @@
>   *
>   * Contributor(s):
>   *   Alexander Surkov <surkov.alexander@gmail.com> (origianl author)
> + *   Andrzej Skalski <askalski@mozilla.com>

huh?
Comment 42 Andrzej Skalski 2012-04-26 11:17:44 PDT
Created attachment 618728 [details] [diff] [review]
5th patch.

Ok, so I addressed all comments I could. Please do Mac testing and move switch on some mac machine, I really don't want to do that without being able to test it.

If it's landable, please do it, and let me do finishing touches in follow ups. It's already 15 files large.
Comment 43 Andrzej Skalski 2012-04-26 11:26:55 PDT
Created attachment 618734 [details] [diff] [review]
6th patch

removing declarations of inlined methods that has been removed
Comment 44 David Bolter [:davidb] 2012-04-26 11:40:47 PDT
Andrzej regarding:

(In reply to Trevor Saunders (:tbsaunde) from comment #40)
> Comment on attachment 618346 [details] [diff] [review]
> Fourth patch proposition
> 
> >@@ -734,9 +733,7 @@
> 
> in future please use -p -u 8 as diff arguments.

(In reply to alexander :surkov from comment #41)
> Comment on attachment 618346 [details] [diff] [review]
> Fourth patch proposition
> 
> Review of attachment 618346 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> use -U 8 -p diff format

I keep this in my ~/.hgrc

[defaults]
qnew = -Ue
diff=-U 8 -p
qdiff=-U 8
qseries=-sv
Comment 45 Hubert Figuiere [:hub] 2012-04-26 15:24:53 PDT
Comment on attachment 618734 [details] [diff] [review]
6th patch

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

Does not build on Mac. You remove nsRoleMap.h but you didn't update the mac/Makefile.in nor the source files that include it.
Comment 46 Andrzej Skalski 2012-04-27 03:19:47 PDT
Created attachment 618981 [details] [diff] [review]
7th patch

Hubert's comment addressed. Thanks.
Comment 47 alexander :surkov 2012-04-27 03:19:51 PDT
Created attachment 618982 [details] [diff] [review]
patch 7
Comment 48 alexander :surkov 2012-04-27 03:22:05 PDT
(In reply to Andrzej Skalski from comment #46)
> Created attachment 618981 [details] [diff] [review]
> 7th patch
> 
> Hubert's comment addressed. Thanks.

anything else is changed?
Comment 49 Andrzej Skalski 2012-04-27 03:28:07 PDT
Yes. Removed some files, manually expanded Get*For functions everywhere but in Atk. Updated comments, added MOZ_NOT_REACHED.
Comment 50 alexander :surkov 2012-04-27 03:29:57 PDT
(In reply to Andrzej Skalski from comment #49)
> Yes. Removed some files, manually expanded Get*For functions everywhere but
> in Atk. Updated comments, added MOZ_NOT_REACHED.

you describe changes from 6th to 7th? Diff between patches doesn't shows this. Btw, MOZ_NOT_REACHED and no Get*For functions were in 6th.
Comment 51 Andrzej Skalski 2012-04-27 04:04:43 PDT
no, between 5th and 6th. From 6th to 7th I only removed nsRoleMap.h references in mac, so it compiles (someone please test it).
Comment 52 alexander :surkov 2012-04-27 04:34:26 PDT
(In reply to Andrzej Skalski from comment #51)
> no, between 5th and 6th. From 6th to 7th I only removed nsRoleMap.h
> references in mac, so it compiles (someone please test it).

unfortunately we worked on the same patch independently (you asked me yesterday to take a look on mac build issue). 

since I fixed some nits and compilation warnings additionally, i.e. my patch changes are superset of your changes then I requested review on my patch and marked yours one as obsolete.
Comment 53 Andrzej Skalski 2012-04-27 04:52:40 PDT
Surkov, no problem, since I asked someone to fix mac build. Thanks for your help.
Comment 54 Trevor Saunders (:tbsaunde) 2012-04-28 15:47:52 PDT
Comment on attachment 618982 [details] [diff] [review]
patch 7

>diff --git a/accessible/public/nsIAccessibleRole.idl b/accessible/public/nsIAccessibleRole.idl
>--- a/accessible/public/nsIAccessibleRole.idl
>+++ b/accessible/public/nsIAccessibleRole.idl
>@@ -35,19 +35,16 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsISupports.idl"
> 
> /**
>  * Defines cross platform (Gecko) roles.
>- *
>- * @note - When adding a new role, be sure to also add it to nsRoleMap.h for
>- *         each platform.
>  */
> [scriptable, uuid(f134da65-39a8-4330-843c-5bd42780b34c)]
> interface nsIAccessibleRole : nsISupports

shouldn't you bump the uuid?

> getRoleCB(AtkObject *aAtkObj)
> {
>-    nsAccessibleWrap *accWrap = GetAccessibleWrap(aAtkObj);
>-    if (!accWrap) {
>-        return ATK_ROLE_INVALID;
>-    }
>+  nsAccessibleWrap *accWrap = GetAccessibleWrap(aAtkObj);

I thought you wanted these changed to type* name?

>+  if (!accWrap) {
>+    return ATK_ROLE_INVALID;
>+  }

get rid of braces?

> #ifdef DEBUG_A11Y
>-    NS_ASSERTION(nsAccUtils::IsTextInterfaceSupportCorrect(accWrap),
>-                 "Does not support nsIAccessibleText when it should");
>+  NS_ASSERTION(nsAccUtils::IsTextInterfaceSupportCorrect(accWrap),
>+      "Does not support nsIAccessibleText when it should");
> #endif

we need to ifdef this on DEBUG_A11Y because the function is only defined then right? want to file a bug to make this nicer?

> 
>-    if (aAtkObj->role == ATK_ROLE_INVALID) {
>-        // map to the actual value
>-        PRUint32 atkRole = atkRoleMap[accWrap->Role()];
>-        NS_ASSERTION(atkRoleMap[nsIAccessibleRole::ROLE_LAST_ENTRY] ==
>-                     kROLE_ATK_LAST_ENTRY, "ATK role map skewed");
>-        aAtkObj->role = static_cast<AtkRole>(atkRole);
>-    }
>-    return aAtkObj->role;
>+  if (aAtkObj->role == ATK_ROLE_INVALID) {
>+    // map to the actual value
>+    PRUint32 atkRole = nsAccessibleWrap::AtkRoleFor(accWrap->Role());
>+    aAtkObj->role = static_cast<AtkRole>(atkRole);
>+  }
>+  return aAtkObj->role;

I don't think caching the role is quiet correct because of aria-pressed, but that should be a different bug.

as a nit I'd prefer if (atkObj->role != UNKNOWN)
  return atkObj->role;

return atkObj->role = static_cast<AtkRole>(accWrap->Role());

>+    default:
>+      MOZ_NOT_REACHED("Unknown role.");
>+      return ATK_ROLE_UNKNOWN;

if you need that its fine with me, but since you just told the compiler it could never end up there I'd think the return isn't needed.

> NS_IMETHODIMP
> nsAccessibilityService::GetStringRole(PRUint32 aRole, nsAString& aString)
> {
>+  // Map nsIAccessibleRole constants to strings. Used by
>+  // nsIAccessibleRetrieval::getStringRole() method.
>+  // The array is expanded from macro defined in base/RoleMap.h

kind of funny to say the code in this method is used by this method.

>+  static const char kRoleNames[][20] = {
>+#define ROLE(geckoRole, stringRole, atkRole, macRole, msaaRole, ia2Role) stringRole,
>+#include "RoleMap.h"
>+#undef ROLE
>+  };

as a nit I'd prefer a switch, but gcc atleast generates exactly the same code as for an array in this case so up to you.  It doesn't even bother to use indicies instead of pointers :-(

>+#ifndef ROLE_SYSTEM_SPLITBUTTON
>+const PRUint32 ROLE_SYSTEM_SPLITBUTTON  = 0x3e; // Not defined in all oleacc.h versions
>+#endif

any idea when we actually need these?

>-               "MSAA role map skewed");
>+  a11y::role geckoRole = xpAccessible->Role();

I assume a11y:: is for some sort of ambiguity resolution?
Comment 55 alexander :surkov 2012-04-29 18:58:44 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #54)
> > [scriptable, uuid(f134da65-39a8-4330-843c-5bd42780b34c)]
> > interface nsIAccessibleRole : nsISupports
> 
> shouldn't you bump the uuid?

ok

> >+  nsAccessibleWrap *accWrap = GetAccessibleWrap(aAtkObj);
> 
> I thought you wanted these changed to type* name?

you caught me :)

> > #ifdef DEBUG_A11Y
> >-    NS_ASSERTION(nsAccUtils::IsTextInterfaceSupportCorrect(accWrap),
> >-                 "Does not support nsIAccessibleText when it should");
> >+  NS_ASSERTION(nsAccUtils::IsTextInterfaceSupportCorrect(accWrap),
> >+      "Does not support nsIAccessibleText when it should");
> > #endif
> 
> we need to ifdef this on DEBUG_A11Y because the function is only defined
> then right? want to file a bug to make this nicer?

yes/how to make it nicer?

> I don't think caching the role is quiet correct because of aria-pressed, but
> that should be a different bug.

the role change requires accessible recreation, so it should ok.

> as a nit I'd prefer if (atkObj->role != UNKNOWN)
>   return atkObj->role;
> 
> return atkObj->role = static_cast<AtkRole>(accWrap->Role());

ok

> >+    default:
> >+      MOZ_NOT_REACHED("Unknown role.");
> >+      return ATK_ROLE_UNKNOWN;
> 
> if you need that its fine with me, but since you just told the compiler it
> could never end up there I'd think the return isn't needed.

it should hit when Role.h is not in sync with RoleMap.h, right?

> > NS_IMETHODIMP
> > nsAccessibilityService::GetStringRole(PRUint32 aRole, nsAString& aString)
> > {
> >+  // Map nsIAccessibleRole constants to strings. Used by
> >+  // nsIAccessibleRetrieval::getStringRole() method.
> >+  // The array is expanded from macro defined in base/RoleMap.h
> 
> kind of funny to say the code in this method is used by this method.

it is :) it doesn't look like we need a comment here at all.

> >+  static const char kRoleNames[][20] = {
> >+#define ROLE(geckoRole, stringRole, atkRole, macRole, msaaRole, ia2Role) stringRole,
> >+#include "RoleMap.h"
> >+#undef ROLE
> >+  };
> 
> as a nit I'd prefer a switch, but gcc atleast generates exactly the same
> code as for an array in this case so up to you.  It doesn't even bother to
> use indicies instead of pointers :-(

ok

> 
> >+#ifndef ROLE_SYSTEM_SPLITBUTTON
> >+const PRUint32 ROLE_SYSTEM_SPLITBUTTON  = 0x3e; // Not defined in all oleacc.h versions
> >+#endif
> 
> any idea when we actually need these?

backward compatibility, not sure if we support those old oleacc.h versions still

> >-               "MSAA role map skewed");
> >+  a11y::role geckoRole = xpAccessible->Role();
> 
> I assume a11y:: is for some sort of ambiguity resolution?

yes, IA2 has role method and they are in conflict.
Comment 56 alexander :surkov 2012-04-29 19:09:33 PDT
Created attachment 619467 [details] [diff] [review]
patch8

patch to land if try server is ok
Comment 57 Trevor Saunders (:tbsaunde) 2012-04-29 19:48:41 PDT
> > >+  nsAccessibleWrap *accWrap = GetAccessibleWrap(aAtkObj);
> > 
> > I thought you wanted these changed to type* name?
> 
> you caught me :)

heh, I enjoyed seeing it from you ;-)

> > > #ifdef DEBUG_A11Y
> > >-    NS_ASSERTION(nsAccUtils::IsTextInterfaceSupportCorrect(accWrap),
> > >-                 "Does not support nsIAccessibleText when it should");
> > >+  NS_ASSERTION(nsAccUtils::IsTextInterfaceSupportCorrect(accWrap),
> > >+      "Does not support nsIAccessibleText when it should");
> > > #endif
> > 
> > we need to ifdef this on DEBUG_A11Y because the function is only defined
> > then right? want to file a bug to make this nicer?
> 
> yes/how to make it nicer?

perhaps make all of it built when DEBUG is defined?

> > I don't think caching the role is quiet correct because of aria-pressed, but
> > that should be a different bug.
> 
> the role change requires accessible recreation, so it should ok.

what about the case of an element that doesn't have aria-pressed then aria-pressed is set on it?  I don't believe we recreate in that case, but ARIATransformRole() will return a different role.

> > >+    default:
> > >+      MOZ_NOT_REACHED("Unknown role.");
> > >+      return ATK_ROLE_UNKNOWN;
> > 
> > if you need that its fine with me, but since you just told the compiler it
> > could never end up there I'd think the return isn't needed.
> 
> it should hit when Role.h is not in sync with RoleMap.h, right?

On really old gcc that don't have __builtin_unreachable I yes, otherwise MOZ_NOT_REACHED is defined to include __assume(0) or __builtin_unreachable() so the compiler is free to do as it likes.

> > 
> > >+#ifndef ROLE_SYSTEM_SPLITBUTTON
> > >+const PRUint32 ROLE_SYSTEM_SPLITBUTTON  = 0x3e; // Not defined in all oleacc.h versions
> > >+#endif
> > 
> > any idea when we actually need these?
> 
> backward compatibility, not sure if we support those old oleacc.h versions
> still

yeah, ok, I guess it take more effort to check than to just leave them.

> > >-               "MSAA role map skewed");
> > >+  a11y::role geckoRole = xpAccessible->Role();
> > 
> > I assume a11y:: is for some sort of ambiguity resolution?
> 
> yes, IA2 has role method and they are in conflict.

ok
Comment 58 alexander :surkov 2012-04-29 22:35:39 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #57)

> perhaps make all of it built when DEBUG is defined?

ok, it makes sense to get rid those DEBUG_A11Y introduced to reduce a noise.

> what about the case of an element that doesn't have aria-pressed then
> aria-pressed is set on it?  I don't believe we recreate in that case, but
> ARIATransformRole() will return a different role.

well, we have a bug then, the role is constant during accessible life cycle.

> On really old gcc that don't have __builtin_unreachable I yes, otherwise
> MOZ_NOT_REACHED is defined to include __assume(0) or __builtin_unreachable()
> so the compiler is free to do as it likes.

so what exactly happens here? Say I added a role into Role.h and didn't updated RoleMap.h

> > backward compatibility, not sure if we support those old oleacc.h versions
> > still
> 
> yeah, ok, I guess it take more effort to check than to just leave them.

yep, I'll take a look and file a bug if we have an issue
Comment 59 Trevor Saunders (:tbsaunde) 2012-04-30 02:04:27 PDT
(In reply to alexander :surkov from comment #58)
> (In reply to Trevor Saunders (:tbsaunde) from comment #57)
> 
> > perhaps make all of it built when DEBUG is defined?
> 
> ok, it makes sense to get rid those DEBUG_A11Y introduced to reduce a noise.
> 
> > what about the case of an element that doesn't have aria-pressed then
> > aria-pressed is set on it?  I don't believe we recreate in that case, but
> > ARIATransformRole() will return a different role.
> 
> well, we have a bug then, the role is constant during accessible life cycle.

what else depends on that being true?  I'd be inclined to allow changes in role like that during life cycle. in any case we should figure out making this correct in a different bug.

> > On really old gcc that don't have __builtin_unreachable I yes, otherwise
> > MOZ_NOT_REACHED is defined to include __assume(0) or __builtin_unreachable()
> > so the compiler is free to do as it likes.
> 
> so what exactly happens here? Say I added a role into Role.h and didn't
> updated RoleMap.h

On debug builds we should assert since we end up in the default case.
On opt builds we're telling the compiler it can assume that it will never enter the default case and so what happens is undefined which afaik is technically the same as now since I believe out of bounds array reads have undefined behavior though maybe a little more predictable.

> 
> > > backward compatibility, not sure if we support those old oleacc.h versions
> > > still
> > 
> > yeah, ok, I guess it take more effort to check than to just leave them.
> 
> yep, I'll take a look and file a bug if we have an issue

sounds good.
Comment 61 alexander :surkov 2012-04-30 02:27:25 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #59)

> > > what about the case of an element that doesn't have aria-pressed then
> > > aria-pressed is set on it?  I don't believe we recreate in that case, but
> > > ARIATransformRole() will return a different role.
> > 
> > well, we have a bug then, the role is constant during accessible life cycle.
> 
> what else depends on that being true?  I'd be inclined to allow changes in
> role like that during life cycle. in any case we should figure out making
> this correct in a different bug.

bug 750188 is filed
Comment 62 alexander :surkov 2012-04-30 02:36:20 PDT
(In reply to alexander :surkov from comment #58)
> (In reply to Trevor Saunders (:tbsaunde) from comment #57)
> 
> > perhaps make all of it built when DEBUG is defined?
> 
> ok, it makes sense to get rid those DEBUG_A11Y introduced to reduce a noise.

bug 750191 filed
Comment 63 alexander :surkov 2012-04-30 02:56:39 PDT
(In reply to alexander :surkov from comment #58)

> > > backward compatibility, not sure if we support those old oleacc.h versions
> > > still
> > 
> > yeah, ok, I guess it take more effort to check than to just leave them.
> 
> yep, I'll take a look and file a bug if we have an issue

filed bug 750196
Comment 65 Olli Pettay [:smaug] 2012-04-30 13:19:38 PDT
This seems to break building on GCC 4.6 / 64bit linux
Comment 66 Olli Pettay [:smaug] 2012-04-30 13:34:19 PDT
Created attachment 619667 [details] [diff] [review]
patch
Comment 67 Olli Pettay [:smaug] 2012-04-30 14:19:03 PDT
https://hg.mozilla.org/mozilla-central/rev/992588c2eab6
Comment 68 Eitan Isaacson [:eeejay] 2012-04-30 16:58:05 PDT
An unfortunate casualty of this patch is the removal of ROLE_LAST_ENTRY from the idl. I was using this to iterate over all valid roles in js. It seems completely normal to have a LAST_ENTRY at the end of an enum. I didn't really read through this whole bug, so I don't know all of the context. But it would be nice if that was re-introduced.
Comment 69 alexander :surkov 2012-04-30 22:10:07 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #68)
> An unfortunate casualty of this patch is the removal of ROLE_LAST_ENTRY from
> the idl. I was using this to iterate over all valid roles in js. It seems
> completely normal to have a LAST_ENTRY at the end of an enum. I didn't
> really read through this whole bug, so I don't know all of the context. But
> it would be nice if that was re-introduced.

ROLE_LAST_ENTRY was used for debugging proposes, to make sure role maps were updated. It never was supposed to be used outside. Honestly I've never heard that keeping last_entry is sort of usual practice for enums. I'd say there's something wrong with design if you need to traverse all constants (excluding debugging/tool proposes).
Comment 70 Trevor Saunders (:tbsaunde) 2012-04-30 23:32:50 PDT
(In reply to alexander :surkov from comment #69)
> (In reply to Eitan Isaacson [:eeejay] from comment #68)
> > An unfortunate casualty of this patch is the removal of ROLE_LAST_ENTRY from
> > the idl. I was using this to iterate over all valid roles in js. It seems
> > completely normal to have a LAST_ENTRY at the end of an enum. I didn't
> > really read through this whole bug, so I don't know all of the context. But
> > it would be nice if that was re-introduced.
> 
> ROLE_LAST_ENTRY was used for debugging proposes, to make sure role maps were
> updated. It never was supposed to be used outside. Honestly I've never heard
> that keeping last_entry is sort of usual practice for enums. I'd say there's
> something wrong with design if you need to traverse all constants (excluding
> debugging/tool proposes).

I've seen people do it before, maybe not teribly often but it happens.  I'd think something is a bit funny about a deisgn if it iterates over all possible roles, but not neccessarily wrong given the enviroment.

I'm not terribly enthused about reintroducing a LAS_ENTRY thing, but  it's small enough and out of the way enough I don't cre very much either.
Comment 71 neil@parkwaycc.co.uk 2012-05-01 01:26:41 PDT
You could always traverse constants using (for var x in CI.nsIAccessibleRole)
Comment 72 neil@parkwaycc.co.uk 2012-05-01 01:28:25 PDT
I mean for( instead of (for of course...
Comment 73 Eitan Isaacson [:eeejay] 2012-05-01 01:37:23 PDT
(In reply to neil@parkwaycc.co.uk from comment #71)
> You could always traverse constants using (for var x in CI.nsIAccessibleRole)

That is a good point. As long as no methods or non-role attributes are tacked on to this interface.
Comment 74 Eitan Isaacson [:eeejay] 2012-05-01 01:41:41 PDT
(In reply to alexander :surkov from comment #69)
> (In reply to Eitan Isaacson [:eeejay] from comment #68)
> > An unfortunate casualty of this patch is the removal of ROLE_LAST_ENTRY from
> > the idl. I was using this to iterate over all valid roles in js. It seems
> > completely normal to have a LAST_ENTRY at the end of an enum. I didn't
> > really read through this whole bug, so I don't know all of the context. But
> > it would be nice if that was re-introduced.
> 
> ROLE_LAST_ENTRY was used for debugging proposes, to make sure role maps were
> updated. It never was supposed to be used outside. Honestly I've never heard
> that keeping last_entry is sort of usual practice for enums. I'd say there's
> something wrong with design if you need to traverse all constants (excluding
> debugging/tool proposes).

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h#287
http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/modutil/error.h#93
http://mxr.mozilla.org/mozilla-central/source/widget/LookAndFeel.h#203
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsStyleConsts.h#346

Plus other public enums in glib/atk/libpng/cairo/android/etc. It makes sense to hold the value of the last marker to know how many roles there are, especially in an ever-expanding list like this. Not just for iterating over, but for pre-allocating arrays, asserting valid values, etc. Making the API more introspective seems to me like good design.

I don't care about this that much. It just caught me by surprise when it broke some work in progress. If it is badly needed I or someone else will open a bug.
Comment 75 alexander :surkov 2012-05-01 02:08:52 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #74)

> Plus other public enums in glib/atk/libpng/cairo/android/etc. It makes sense
> to hold the value of the last marker to know how many roles there are,
> especially in an ever-expanding list like this. Not just for iterating over,
> but for pre-allocating arrays, asserting valid values, etc. Making the API
> more introspective seems to me like good design.

well, I would change my mind if I had one good use case.
Comment 76 alexander :surkov 2012-05-01 02:14:02 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #74)
> > ROLE_LAST_ENTRY was used for debugging proposes, to make sure role maps were
> > updated. It never was supposed to be used outside. Honestly I've never heard
> > that keeping last_entry is sort of usual practice for enums. I'd say there's
> > something wrong with design if you need to traverse all constants (excluding
> > debugging/tool proposes).
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h#287
> http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/modutil/error.
> h#93
> http://mxr.mozilla.org/mozilla-central/source/widget/LookAndFeel.h#203
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsStyleConsts.h#346

I wasn't clean enough. I didn't mean nobody uses them (since we used it as long as we had a reason to do that). I meant I've never heard about last_entry approach for set of constants that is supposed for public usage, I keep in mind things like MSAA/IA2 constants for example.

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