379435 - Expose accessible role and state as strings in nsIAccessibilityService

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Vasiliy Potapenko]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; ru; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: 

Expose accessible role and state as strings in nsIAccessibilityService

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Vasiliy Potapenko]

Attachment: patch adds two functions to the nsIAccessibilityService

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 264570 [details] [diff] [review]
patch adds two functions to the nsIAccessibilityService


>+  AString getStringRole(in unsigned long aRole);
>+  nsIDOMDOMStringList getStringStates(in unsigned long aStates, in unsigned long aExtraStates);

Please document them.

>+{
>+  switch (aRole){
>+    case nsIAccessibleRole::ROLE_NOTHING:
>+      aString = NS_LITERAL_STRING("nothing");

I would prefer to use aString.AssignLiteral();

>+NS_IMETHODIMP nsAccessibilityService::GetStringStates(PRUint32 aStates, PRUint32 aExtraStates, nsIDOMDOMStringList **aStringStates)
>+{
>+  nsAccessibleDOMStringList *stringStates = new nsAccessibleDOMStringList();
>+  NS_ENSURE_TRUE(stringStates, NS_ERROR_OUT_OF_MEMORY);

nit: add new line

>+  //states
>+  if (aStates & nsIAccessibleStates::STATE_UNAVAILABLE)
>+    stringStates -> Add(NS_LITERAL_STRING("unavailable"));

nit: there is not need spaces around ->

Probably I would add AddLiteral() method to nsAccessibleDOMStringList.

>+  //unknown states
>+  PRUint32 stringStatesLength;

nit: it's better to initialize it.

with above fixed r=me

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 264570 [details] [diff] [review]
patch adds two functions to the nsIAccessibilityService


>+  nsAccessibleDOMStringList *stringStates = new nsAccessibleDOMStringList();
>+  NS_ENSURE_TRUE(stringStates, NS_ERROR_OUT_OF_MEMORY);
>+  //states
>+  if (aStates & nsIAccessibleStates::STATE_UNAVAILABLE)
>+    stringStates -> Add(NS_LITERAL_STRING("unavailable"));

Neil, does it makes sense to add AddLiteral() method to nsAccessibleDOMStringList?

Aaron, does the patch idea goes with you?

Comment 4

[Vasiliy Potapenko]

Attachment: patch2

Comment 5

[Vasiliy Potapenko]

Surkov, I'm fix your remarks, see patch2

Comment 6

[[e-mail]]

Comment on attachment 264595 [details] [diff] [review]
patch2

>+  switch (aRole){
Nit: space between ) and {

Comment 7

[[e-mail]]

(In reply to comment #3)
>Neil, does it makes sense to add AddLiteral() method to
>nsAccessibleDOMStringList?
I'm not sure... ask someone with a better knowledge of the string APIs?

Comment 8

[Vasiliy Potapenko]

Attachment: added space:)

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #3)
> >Neil, does it makes sense to add AddLiteral() method to
> >nsAccessibleDOMStringList?
> I'm not sure... ask someone with a better knowledge of the string APIs?
> 

If you are ok then it's fine with me, I just thought code will be a bit readable.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 264596 [details] [diff] [review]
added space:)


>+  //expose accessible role as string

please use java doc style.

Comment 11

[Vasiliy Potapenko]

Attachment: patch3

fix comment

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 264651 [details] [diff] [review]
patch3

Just a nits :)

>+   * @param aRole - the accessible role constants (see the ROLE_* constants defined 
>+   *                in nsIAccessibleRole.)

nsIAccessibleRole defines role constants only. You do not need to mention prefix the ROLE_* here.

>+   * Returns list which contains accessible States and ExtraStates as a strings.

It doesn't make sense to distinguish states and extra states in comment since we keep them separately until we use int64 to represent states.

>+   *
>+   * @param aStates - accessible states (see nsIAccessibleStates::STATE_* constants)
>+   * @param aExtraStates - accessible extra states (see nsIAccessibleStates::EXT_STATE_* 
>+   *                       constants)

Please keep 80 charachters line length.

Aaron or Neil, please check english in comments.

Comment 13

[Vasiliy Potapenko]

Attachment: patch 4

I found several mistakes.

Comment 14

[alexander :surkov (:asurkov)]

There is no STATE_ALERT_LOW, STATE_ALERT_MEDIUM, STATE_ALERT_HIGH and STATE_MARQUEED.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> There is no STATE_ALERT_LOW, STATE_ALERT_MEDIUM, STATE_ALERT_HIGH and
> STATE_MARQUEED.
> 

and ROLE_LAST_ENTRY too

Comment 16

[Vasiliy Potapenko]

Attachment: patch

Comment 17

[Aaron Leventhal]

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

Hi, thanks for the first patch.

We want to reduce codesize as much as possible. I wonder if the compiler knows how to optimize any of this.

Comment 18

[[e-mail]]

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

>+  switch (aRole){
Nit: still no space between ) and {

>+    case nsIAccessibleRole::ROLE_NOTHING:
>+      aString.AssignLiteral("nothing");
>+      break;
...
>+    case nsIAccessibleRole::ROLE_COMBOBOX_LISTITEM:
>+      aString.AssignLiteral("combobox listitem");
>+      break;
>+    default:
>+      aString.AssignLiteral("unknown");
>+  }
>+  return NS_OK;
>+}
Different compilers do different things for switch statements. The values are not close enough to use a jump table, but it might still use a lookup table, or it might just emit a sequence of comparisons. The other optimisation the compiler might do here is to merge the call to AssignLiteral for each case, i.e. instead of generating N call statements it assigns the literal into a temporary and calls AssignLiteral once just before the return.

To manually optimize this you would have to create an array of structures of role and literal pairs, and loop through until you find the one you want.

>+  if (aStates & nsIAccessibleStates::STATE_UNAVAILABLE)
>+    stringStates->Add(NS_LITERAL_STRING("unavailable"));
...
>+  if (aStates & nsIAccessibleStates::STATE_CHECKABLE)
>+    stringStates->Add(NS_LITERAL_STRING("checkable"));
Again, you could create an array of structures of states and literal pairs...

Comment 19

[Aaron Leventhal]

Neil, should we just not bother and let the compiler handle the optimizations?

Comment 20

[Aaron Leventhal]

Note, for the roles, the values are close together. Only the state values are far appart.

Comment 21

[Vasiliy Potapenko]

I shall try to optimize for the role.

Comment 22

[[e-mail]]

(In reply to comment #19)
>Neil, should we just not bother and let the compiler handle the optimizations?
I wouldn't try too hard, in case it's not worthwhile.

(In reply to comment #20)
>Note, for the roles, the values are close together.
Not that close together when I looked - 1 to 100 isn't close.

Comment 23

[Aaron Leventhal]

Actually for role it makes sense since it's just an enum. For example we use tables for the mapping to MSAA/ATK and OS X. See http://lxr.mozilla.org/seamonkey/find?string=nsrolemap.h

Comment 24

[Vasiliy Potapenko]

Aaron, may be use any container?

Comment 25

[Vasiliy Potapenko]

I'll try to use nsClassHashtable.

Comment 26

[alexander :surkov (:asurkov)]

Move to accessible retrieval per bug 268935.

Comment 27

[alexander :surkov (:asurkov)]

What kind of optimization do you want to see? I think we shouldn't care about speed optimization here but rather probably we should bother about memory usage. These methods will be called once from a script when accessible wants to be shown in DOMi view. I do not know another usecases. Therefore it doesn't hit us if method won't be quick enough. If we will introduce array (or another structure) to map roles (states) to strings then it takes some additional memory and it's not worth to do it I guess.

Comment 28

[Aaron Leventhal]

Correct -- size optimzation is what matters here. Hash tables are too big. It will be fast enough with any of these methods.

We only need to really do this for roles, like what we do for role map. It's not worth it for states as you said.

Comment 29

[Vasiliy Potapenko]

Attachment: patch

Comment 30

[Aaron Leventhal]

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

That's not good, we're creating the whole array every time and not saving space.

Define a static array in a header file and then convert to a Mozilla string on the fly.

static char* kRoleNames[] = {"button", "checkbox", ...}

Then in method:
aString.Assign(kRoleNames[aRole]);

Although you might have to use a conversion function.

Comment 31

[Vasiliy Potapenko]

Attachment: patch

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #31)
> Created an attachment (id=266601) [details]
> patch
> 

You shouldn't define kRoleNames inside header file because your struct is internal usage. Also, please keep more empty lines to make code more readable.

Comment 33

[alexander :surkov (:asurkov)]

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


> 
> // for component registration
> // {663CA4A8-D219-4000-925D-D8F66406B626}
> #define NS_ACCESSIBLE_RETRIEVAL_CID \
>Index: accessible/src/base/nsAccessibilityService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v
>retrieving revision 1.220
>diff -u -8 -p -r1.220 nsAccessibilityService.cpp
>--- accessible/src/base/nsAccessibilityService.cpp	21 May 2007 13:57:54 -0000	1.220
>+++ accessible/src/base/nsAccessibilityService.cpp	30 May 2007 10:01:53 -0000
>@@ -915,16 +915,123 @@ NS_IMETHODIMP nsAccessibilityService::Ge
>   if (!accessibleDoc) {
>     *aAccessNode = nsnull;
>     return NS_ERROR_FAILURE;
>   }
> 
>   return accessibleDoc->GetCachedAccessNode(NS_STATIC_CAST(void*, aNode), aAccessNode);
> }
> 
>+NS_IMETHODIMP nsAccessibilityService::GetStringRole(PRUint32 aRole, nsAString& aString)
>+{
>+  if ( aRole > 115 ) {

check length of the array instead. And I believe you should fail if role is wrong.

>+    aString.AssignLiteral("unknown");
>+    return NS_OK;
>+  };
>+  aString.Assign(NS_ConvertUTF8toUTF16(kRoleNames[aRole]));

Use AppendUTF8toUTF16.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #33)

> >+{
> >+  if ( aRole > 115 ) {
> 
And I believe you should fail if role is
> wrong.

No, "unknown" is better because you plan to call the method from a script.

Comment 35

[[e-mail]]

(In reply to comment #33)
>(From update of attachment 266601 [details] [diff] [review])
>>+  aString.Assign(NS_ConvertUTF8toUTF16(kRoleNames[aRole]));
>Use AppendUTF8toUTF16.
s/Append/Copy/

Comment 36

[[e-mail]]

(In reply to comment #33)
>(From update of attachment 266601 [details] [diff] [review])
>>+  if ( aRole > 115 ) {
>check length of the array instead.
Using NS_ARRAY_LENGTH

Comment 37

[[e-mail]]

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

>+static char* kRoleNames[] = {
Depending on the average length of a name it's probably more efficient to write static char kRoleNames[][20] instead. sr=me if you fix this and the comments already mentioned previously.

Comment 38

[[e-mail]]

(In reply to comment #37)
>static char kRoleNames[][20]
Add a const too, for good measure.

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #32)
> (In reply to comment #31)
> > Created an attachment (id=266601) [details] [details]
> > patch
> > 
> 
> You shouldn't define kRoleNames inside header file because your struct is
> internal usage. 

Ignore my comment per Aaron's comment #30.

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> (In reply to comment #33)
> >(From update of attachment 266601 [details] [diff] [review])
> >>+  aString.Assign(NS_ConvertUTF8toUTF16(kRoleNames[aRole]));
> >Use AppendUTF8toUTF16.
> s/Append/Copy/
> 

Right, thank you.

Comment 41

[Vasiliy Potapenko]

Attachment: fixed

Comment 42

[alexander :surkov (:asurkov)]

Comment on attachment 266727 [details] [diff] [review]
fixed


>+  if ( aRole >= NS_ARRAY_LENGTH(kRoleNames) ) {
>+    aString.AssignLiteral("unknown");
>+    return NS_OK;
>+  };

you don't need ';' here.

>+  stringStates -> GetLength(&stringStatesLength);

you do not need spaces between '->'

I'll fix it before checking in.

Comment 43

[Vasiliy Potapenko]

Attachment: I fixed yours nits

Comment 44

[alexander :surkov (:asurkov)]

checked in