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 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 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?

Surkov, I'm fix your remarks, see patch2

Comment on attachment 264595 [details] [diff] [review] patch2 >+ switch (aRole){ Nit: space between ) and {

(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?

(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 on attachment 264596 [details] [diff] [review] added space:) >+ //expose accessible role as string please use java doc style.

fix comment

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.

I found several mistakes.

There is no STATE_ALERT_LOW, STATE_ALERT_MEDIUM, STATE_ALERT_HIGH and STATE_MARQUEED.

(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 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 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...

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

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

I shall try to optimize for the role.

(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.

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

Aaron, may be use any container?

I'll try to use nsClassHashtable.

Move to accessible retrieval per bug 268935.

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.

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 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.

(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 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.

(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.

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

(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 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.

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

(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.

(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 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.

checked in