754869 - MustPrune should take nsAccessible*

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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]

(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 (:asurkov)]

please ask

Comment 3

[Xi Yang]

(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 (:asurkov)]

(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]

(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]]

Mid air collision ... didn't see comments from Xi ...

Comment 7

[alexander :surkov (:asurkov)]

(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]

Hi, just wondering where I can find the nsIAccessible.h? Many thanks!

Comment 9

[Mark Capella [:capella]]

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]

(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 (:asurkov)]

(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]

Attachment: change the accessible/src/base/nsAccUtils::MustPrune to nsAccessible*

Comment 13

[alexander :surkov (:asurkov)]

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]

Attachment: Patch2-MustPrune

Comment 15

[alexander :surkov (:asurkov)]

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]

Attachment: Patch(v3)-MustPrune

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 625611 [details] [diff] [review]
Patch(v3)-MustPrune

r=me, thank you

Comment 18

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/595b826d6d3a

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/595b826d6d3a