345275 - make eAction_*'s definition to where they belong and remove e*_Action enum

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Nian Liu(n/a in a long time)]

currentlly defined in nsAccessible.h which makes confused

Comment 1

[Nian Liu(n/a in a long time)]

Attachment: patch

Comment 2

[Aaron Leventhal]

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

+  enum { eAction_Select=0, eAction_Click=0 };
Why are two actions defined to be 0 in the same interface?

enum { eNo_Action=0, eSingle_Action=1, eDouble_Action=2 };
Can you remove those? It makes no sense to use those for something just returning the number of actions. Not sure why I allowed that in :) That's as bad as defining TWO=2 :)

Comment 3

[Nian Liu(n/a in a long time)]

Attachment: patch v2

patch addressing aaron's comment

Comment 4

[[e-mail]]

Comment on attachment 230075 [details] [diff] [review]
patch v2

A couple of issues that aaron can answer better than I can:

>+  enum { eAction_ShowLongDescription=1 }; 
I happened to notice a trailing space on this line! I was also wondering wheter we should space out around the = sign. But what I was really wondering is whether something should be returning 2 actions for GetNumActions.

>   if (isContainer)
>-    *_retval = eDouble_Action;
>+    *_retval = 2;
>   else
>-    *_retval = eSingle_Action;
>+    *_retval = 1;
I've no idea what module style is but I would have used ?: here.

Comment 5

[Aaron Leventhal]

Comment on attachment 230075 [details] [diff] [review]
patch v2

Don't forget Neil's comments about whitespace when you check in.

Comment 6

[Nian Liu(n/a in a long time)]

Attachment: patch removes extra trailing space

Comment 7

[Ginn Chen]

Attachment: patch checked in