Closed Bug 675879 Opened 13 years ago Closed 12 years ago

clean up nsIAccessible action methods

Categories

(Core :: Disability Access APIs, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: surkov, Assigned: oussamabadr)

References

(Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 3 obsolete files)

numActions -> actionCount

not sure whether we should rename getActionName to getActionNameAt and others.
Trevor, what do you think about adding 'at' postfix to method names?
(In reply to alexander :surkov from comment #1)
> Trevor, what do you think about adding 'at' postfix to method names?

I don't have much of an opinion either way.  Treating the actions like an array feels funny to me in general, but given windows and linux both want it it that way I guess we should go with that api like that.
Yep, it's sort of weird. So let's do not do extra work we aren't sure about.

So do numActions -> actionCount for now.
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++]
Alexander, Can you put more details?
are you talking about renaming action methods in nsIAccessible( getActionName; getActionDescription; doAction) by putting "At" as a  postfix ?
(In reply to Oussama BADR from comment #4)
> Alexander, Can you put more details?
> are you talking about renaming action methods in nsIAccessible(
> getActionName; getActionDescription; doAction) by putting "At" as a  postfix
> ?

no, just change numActions to actionCount (per comment #3)
No longer blocks: cleanxpcoma11y
Ok, you can assign me in, thanks in advance!
Assignee: nobody → oussamabadr
Attached patch Patch to fix Bug 675879. (obsolete) — Splinter Review
Attachment #625902 - Flags: review?(surkov.alexander)
Comment on attachment 625902 [details] [diff] [review]
Patch to fix Bug 675879.

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

you need to change UUID of nsIAccessible interface and you didn't changed implementation (look for GetNumActions method)
Attachment #625902 - Flags: review?(surkov.alexander)
Attached patch Patch V2 to fix Bug 675879. (obsolete) — Splinter Review
Attachment #626419 - Flags: review?(surkov.alexander)
Comment on attachment 626419 [details] [diff] [review]
Patch V2 to fix Bug 675879.

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

you didn't change all instances of 'numActions' in mochitests folder (for example, actions/test_general.html), canceling review, I need to look at next patch.

::: accessible/public/nsIAccessible.idl
@@ +289,5 @@
>  
>    /**
>     * The number of accessible actions associated with this accessible
>     */
> +  readonly attribute PRUint8 actionCount;

you didn't change the uuid of nsIAccessible interface, please do that

::: accessible/src/base/nsAccessible.cpp
@@ +1867,5 @@
>  
>    *aActionCount = ActionCount();
>    return NS_OK;
>  }
> + 

whitespace on empty line
Attachment #626419 - Flags: review?(surkov.alexander)
Attachment #625902 - Attachment is obsolete: true
Over the weekend an issue occurred with the BMO database which resulted in duplication of dependencies. The dependency issue may have resulted in "Depends On" and "Blocks" values being removed while updating a bug. This issue should now be resolved, however dependencies may need to be manually restored to some bugs.

This bug had dependencies removed during the failure period and will need verification that the dependency removal(s) were intentional. Please help out by taking a look at this bug and adding anything back that was mistakenly removed.
> >    /**
> >     * The number of accessible actions associated with this accessible
> >     */
> > +  readonly attribute PRUint8 actionCount;
> 
> you didn't change the uuid of nsIAccessible interface, please do that

I didn't match what I have to do for UUID, I have found this line for UUID in nsIAccessible interface :
...
[scriptable, uuid(46d422d1-c92f-4536-bdef-f77bc8350ec7)]
interface nsIAccessible : nsISupports
...
really sorry, but I have lost my fingers in!!!
Attachment #626419 - Attachment is obsolete: true
(In reply to Oussama BADR from comment #12)

> [scriptable, uuid(46d422d1-c92f-4536-bdef-f77bc8350ec7)]
> interface nsIAccessible : nsISupports

yes, you need to replace this uuid on new one
Attached patch Patch V3 to fix Bug 675879. (obsolete) — Splinter Review
Attachment #627150 - Flags: review?(surkov.alexander)
(In reply to alexander :surkov from comment #10)
> Comment on attachment 626419 [details] [diff] [review]
> -----------------------------------------------------------------
> 
> you didn't change all instances of 'numActions' in mochitests folder (for
> example, actions/test_general.html), canceling review, I need to look at
> next patch.
> 

to check out 'numActions' instances in all accessible folders, I have used 'grep' command in my Ubuntu OS instead of Mozilla mxr web tool, the last one didn't display all occurrences!
Comment on attachment 627150 [details] [diff] [review]
Patch V3 to fix Bug 675879.

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

::: accessible/tests/mochitest/actions/test_general.html
@@ +45,5 @@
>        testActions(actionsArray);
>  
>        getAccessible("onclick_img").takeFocus();
> +is(getAccessible("link1").actionCount, 1, "links should have one action");
> +is(getAccessible("link2").actionCount, 1, "link with onclick handler should have 1 action");

please make proper indentation while you are here
Attachment #627150 - Flags: review?(surkov.alexander) → review+
...with right indentations!
Attachment #627171 - Flags: review?(surkov.alexander)
Comment on attachment 627171 [details] [diff] [review]
Patch V4 to fix Bug 675879.

you don't need to rerequest review for minor changes.

I'll land the patch, thank you for the fix
Attachment #627171 - Flags: review?(surkov.alexander) → review+
welcome!

but, why did I change the UUID of nsIAccessible interface?
(In reply to Oussama BADR from comment #19)
> welcome!
> 
> but, why did I change the UUID of nsIAccessible interface?

Let's you have a c++ extension that use that interface, you do query interface by passing that UUID then you get back an interface pointer and you call a method. Since interface was changed then you may end up with calling wrong code or crash. You need to read how methods calls are implemented in c++ for more information. If you changed UUID then that QueryInterface will return null pointer.
thks for explanations!!

may be I should find out a new bug, but (I think) I'm not ready to do it, so please led me to the next bug!
Attachment #627150 - Attachment is obsolete: true
(In reply to Oussama BADR from comment #21)

> may be I should find out a new bug, but (I think) I'm not ready to do it, so
> please led me to the next bug!

thank you, I cc'ed you on bug 672514.
https://hg.mozilla.org/mozilla-central/rev/ca31a899ce4e
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 838076
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: