Last Comment Bug 675879 - clean up nsIAccessible action methods
: clean up nsIAccessible action methods
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Oussama BADR
:
: alexander :surkov
Mentors:
Depends on: 838076
Blocks: cleanxpcoma11y
  Show dependency treegraph
 
Reported: 2011-08-02 01:08 PDT by alexander :surkov
Modified: 2013-02-04 22:28 PST (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix Bug 675879. (3.47 KB, patch)
2012-05-21 23:17 PDT, Oussama BADR
no flags Details | Diff | Splinter Review
Patch V2 to fix Bug 675879. (4.36 KB, patch)
2012-05-23 06:44 PDT, Oussama BADR
no flags Details | Diff | Splinter Review
Patch V3 to fix Bug 675879. (23.54 KB, patch)
2012-05-25 02:51 PDT, Oussama BADR
surkov.alexander: review+
Details | Diff | Splinter Review
Patch V4 to fix Bug 675879. (23.55 KB, patch)
2012-05-25 04:11 PDT, Oussama BADR
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-08-02 01:08:50 PDT
numActions -> actionCount

not sure whether we should rename getActionName to getActionNameAt and others.
Comment 1 alexander :surkov 2012-04-25 00:10:06 PDT
Trevor, what do you think about adding 'at' postfix to method names?
Comment 2 Trevor Saunders (:tbsaunde) 2012-04-28 21:42:34 PDT
(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.
Comment 3 alexander :surkov 2012-04-29 19:17:06 PDT
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.
Comment 4 Oussama BADR 2012-05-19 03:47:49 PDT
Alexander, Can you put more details?
are you talking about renaming action methods in nsIAccessible( getActionName; getActionDescription; doAction) by putting "At" as a  postfix ?
Comment 5 alexander :surkov 2012-05-19 08:18:09 PDT
(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)
Comment 6 Oussama BADR 2012-05-19 17:37:35 PDT
Ok, you can assign me in, thanks in advance!
Comment 7 Oussama BADR 2012-05-21 23:17:25 PDT
Created attachment 625902 [details] [diff] [review]
Patch to fix Bug 675879.
Comment 8 alexander :surkov 2012-05-22 03:13:31 PDT
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)
Comment 9 Oussama BADR 2012-05-23 06:44:55 PDT
Created attachment 626419 [details] [diff] [review]
Patch V2 to fix Bug 675879.
Comment 10 alexander :surkov 2012-05-23 07:23:37 PDT
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
Comment 11 David Lawrence [:dkl] 2012-05-24 07:58:35 PDT
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.
Comment 12 Oussama BADR 2012-05-24 09:33:00 PDT
> >    /**
> >     * 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!!!
Comment 13 alexander :surkov 2012-05-25 01:20:09 PDT
(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
Comment 14 Oussama BADR 2012-05-25 02:51:13 PDT
Created attachment 627150 [details] [diff] [review]
Patch V3 to fix Bug 675879.
Comment 15 Oussama BADR 2012-05-25 03:00:09 PDT
(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 16 alexander :surkov 2012-05-25 03:50:06 PDT
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
Comment 17 Oussama BADR 2012-05-25 04:11:59 PDT
Created attachment 627171 [details] [diff] [review]
Patch V4 to fix Bug 675879.

...with right indentations!
Comment 18 alexander :surkov 2012-05-25 04:13:26 PDT
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
Comment 19 Oussama BADR 2012-05-25 04:24:18 PDT
welcome!

but, why did I change the UUID of nsIAccessible interface?
Comment 20 alexander :surkov 2012-05-25 06:11:01 PDT
(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.
Comment 21 Oussama BADR 2012-05-26 04:01:20 PDT
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!
Comment 22 alexander :surkov 2012-05-26 06:41:54 PDT
(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.
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-05-26 15:31:26 PDT
https://hg.mozilla.org/mozilla-central/rev/ca31a899ce4e

Note You need to log in before you can comment on or make changes to this bug.