Closed Bug 345275 Opened 15 years ago Closed 15 years ago

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

Categories

(Firefox :: Disability Access, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nian.liu, Assigned: nian.liu)

References

Details

Attachments

(3 files, 1 obsolete file)

currentlly defined in nsAccessible.h which makes confused
Attached patch patchSplinter Review
Attachment #229941 - Flags: review?(aaronleventhal)
Status: NEW → ASSIGNED
Assignee: nobody → nian.liu
Status: ASSIGNED → NEW
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 :)
Attachment #229941 - Flags: review?(aaronleventhal) → review+
Summary: make eAction_*'s definition to where they belong → make eAction_*'s definition to where they belong and remove e*_Action enum
Attached patch patch v2Splinter Review
patch addressing aaron's comment
Attachment #230075 - Flags: superreview?(neil)
Attachment #230075 - Flags: review?(aaronleventhal)
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.
Attachment #230075 - Flags: superreview?(neil) → superreview+
Comment on attachment 230075 [details] [diff] [review]
patch v2

Don't forget Neil's comments about whitespace when you check in.
Attachment #230075 - Flags: review?(aaronleventhal) → review+
Attached patch patch checked inSplinter Review
Attachment #230387 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.