Last Comment Bug 754869 - MustPrune should take nsAccessible*
: MustPrune should take nsAccessible*
Status: RESOLVED FIXED
[good first bug][mentor=hub@mozilla.c...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Xi Yang
:
Mentors:
Depends on: 757739
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-05-14 08:52 PDT by alexander :surkov
Modified: 2012-05-23 00:37 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
change the accessible/src/base/nsAccUtils::MustPrune to nsAccessible* (1.90 KB, patch)
2012-05-19 07:06 PDT, Xi Yang
no flags Details | Diff | Splinter Review
Patch2-MustPrune (3.01 KB, patch)
2012-05-19 09:11 PDT, Xi Yang
surkov.alexander: review+
Details | Diff | Splinter Review
Patch(v3)-MustPrune (2.80 KB, patch)
2012-05-21 05:00 PDT, Xi Yang
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-05-14 08:52:39 PDT
accessible/src/base/nsAccUtils::MustPrune takes nsIAccessible*. nsIAccessible can replaced on nsAccessible what allows to call nsAccessible::Role instead of nsIAccessible::GetRole. This change makes us a little bit faster since we don't spend a time for excess call.
Comment 1 Xi Yang 2012-05-17 05:48:03 PDT
(In reply to alexander :surkov from comment #0)
> accessible/src/base/nsAccUtils::MustPrune takes nsIAccessible*.
> nsIAccessible can replaced on nsAccessible what allows to call
> nsAccessible::Role instead of nsIAccessible::GetRole. This change makes us a
> little bit faster since we don't spend a time for excess call.

Hi Alexander,

I'm a rookie, and I would like to work on this problem. 
All I need is a few tips, would you like to help me out?
Comment 2 alexander :surkov 2012-05-17 06:14:42 PDT
please ask
Comment 3 Xi Yang 2012-05-17 06:24:23 PDT
(In reply to alexander :surkov from comment #2)
> please ask

1. Should I read every source files in the directory, i.e the accessible/src/base/?
2. If I make any modifications, how can I test my code? Should I rebuild the whole project, or just some parts of it? 

Thanks!
Comment 4 alexander :surkov 2012-05-17 06:27:41 PDT
(In reply to Xi Yang from comment #3)
> (In reply to alexander :surkov from comment #2)
> > please ask
> 
> 1. Should I read every source files in the directory, i.e the
> accessible/src/base/?

search through accessible/src

> 2. If I make any modifications, how can I test my code? Should I rebuild the
> whole project, or just some parts of it? 

cd objdir/accessible; make; cd ../toolkit/library; make

if you don't have build errors then your patch should be ok. Reviewer will check it on correctness.
Comment 5 Xi Yang 2012-05-17 06:33:18 PDT
(In reply to alexander :surkov from comment #4)
> (In reply to Xi Yang from comment #3)
> > (In reply to alexander :surkov from comment #2)
> > > please ask
> > 
> > 1. Should I read every source files in the directory, i.e the
> > accessible/src/base/?
> 
> search through accessible/src
> 
> > 2. If I make any modifications, how can I test my code? Should I rebuild the
> > whole project, or just some parts of it? 
> 
> cd objdir/accessible; make; cd ../toolkit/library; make
> 
> if you don't have build errors then your patch should be ok. Reviewer will
> check it on correctness.

Is it possible to verify the correctness of my patch on my own?
Comment 6 Mark Capella [:capella] 2012-05-17 06:39:36 PDT
Mid air collision ... didn't see comments from Xi ...
Comment 7 alexander :surkov 2012-05-17 06:41:00 PDT
(In reply to Xi Yang from comment #5)

> Is it possible to verify the correctness of my patch on my own?

as I said if there's no build errors then your patch should be correct. You could run a11y mochitests but it doesn't seem like mochitests are good in this case.
Comment 8 Xi Yang 2012-05-17 14:16:07 PDT
Hi, just wondering where I can find the nsIAccessible.h? Many thanks!
Comment 9 Mark Capella [:capella] 2012-05-17 14:21:23 PDT
Xi Yang, you can use Mozilla cross reference search like:

http://mxr.mozilla.org/mozilla-central/search?string=nsIAccessible.h&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Comment 10 Xi Yang 2012-05-17 14:23:51 PDT
(In reply to Mark Capella [:capella] from comment #9)
> Xi Yang, you can use Mozilla cross reference search like:
> 
> http://mxr.mozilla.org/mozilla-central/search?string=nsIAccessible.
> h&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

That's brilliant! thanks!
Comment 11 alexander :surkov 2012-05-17 20:07:26 PDT
(In reply to Xi Yang from comment #8)
> Hi, just wondering where I can find the nsIAccessible.h? Many thanks!

there's no nsIAccessible.h, it's auto generated from nsIAccessible.idl
Comment 12 Xi Yang 2012-05-19 07:06:29 PDT
Created attachment 625404 [details] [diff] [review]
change the accessible/src/base/nsAccUtils::MustPrune to nsAccessible*
Comment 13 alexander :surkov 2012-05-19 08:16:39 PDT
Comment on attachment 625404 [details] [diff] [review]
change the accessible/src/base/nsAccUtils::MustPrune to nsAccessible*

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

I'd like to look at another patch

::: accessible/src/base/nsAccUtils.cpp
@@ +493,5 @@
>    return text.Length();
>  }
>  
>  bool
> +nsAccUtils::MustPrune(nsAccessible *aAccessible)

nit: type* name

@@ +498,3 @@
>  { 
> +  PRUint32 role = nsIAccessibleRole::ROLE_NOTHING;
> +  if (aAccessible) role = aAccessible->Role();

Role() returns a constant from Role.h (not nsIAccessibleRole constant), also please change constants below

it's better to use either roles:Role or role type instead PRUint32 (Trevor, which option is nicer?)

the style should be:
if (aAccessible)
  role = aAccessible->Role();

but in this case you don't need to check aAccessible

::: accessible/src/base/nsAccUtils.h
@@ +327,5 @@
>    /**
>     * Return true if the given accessible can't have children. Used when exposing
>     * to platform accessibility APIs, should the children be pruned off?
>     */
> +  static bool MustPrune(nsAccessible *aAccessible);

type* name
Comment 14 Xi Yang 2012-05-19 09:11:33 PDT
Created attachment 625417 [details] [diff] [review]
Patch2-MustPrune
Comment 15 alexander :surkov 2012-05-20 18:27:47 PDT
Comment on attachment 625417 [details] [diff] [review]
Patch2-MustPrune

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

r=me with comments addressed

::: accessible/src/base/nsAccUtils.cpp
@@ +498,3 @@
>  { 
> +  mozilla::a11y::roles::Role role = mozilla::a11y::roles::NOTHING;
> +  role = aAccessible->Role();

do instead:
roles::Role role = aAccessbile->Role();

@@ +511,5 @@
> +    role == mozilla::a11y::roles::TOGGLE_BUTTON ||
> +    role == mozilla::a11y::roles::GRAPHIC ||
> +    role == mozilla::a11y::roles::SLIDER ||
> +    role == mozilla::a11y::roles::PROGRESSBAR ||
> +    role == mozilla::a11y::roles::SEPARATOR;

you don't need mozilla::a11y since this file has 'using namespace mozilla::a11y'
Comment 16 Xi Yang 2012-05-21 05:00:21 PDT
Created attachment 625611 [details] [diff] [review]
Patch(v3)-MustPrune
Comment 17 alexander :surkov 2012-05-21 06:15:27 PDT
Comment on attachment 625611 [details] [diff] [review]
Patch(v3)-MustPrune

r=me, thank you
Comment 19 Ed Morley [:emorley] 2012-05-22 06:43:38 PDT
https://hg.mozilla.org/mozilla-central/rev/595b826d6d3a

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