Closed Bug 754869 Opened 8 years ago Closed 8 years ago

MustPrune should take nsAccessible*

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: surkov, Assigned: vinceyang15)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=hub@mozilla.com][lang=c++])

Attachments

(1 file, 2 obsolete files)

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.
(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?
please ask
(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!
(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.
(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?
Assignee: nobody → markcapella
Assignee: markcapella → nobody
Mid air collision ... didn't see comments from Xi ...
(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.
Hi, just wondering where I can find the nsIAccessible.h? Many thanks!
(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!
(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
Attachment #625404 - Flags: review? → review?(surkov.alexander)
Attachment #625404 - Flags: review?(surkov.alexander) → review?
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
Attachment #625404 - Flags: review?
Attached patch Patch2-MustPrune (obsolete) — Splinter Review
Attachment #625417 - Flags: review?(surkov.alexander)
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'
Attachment #625417 - Flags: review?(surkov.alexander) → review+
Attachment #625611 - Flags: review?(surkov.alexander)
Attachment #625404 - Attachment is obsolete: true
Attachment #625417 - Attachment is obsolete: true
Comment on attachment 625611 [details] [diff] [review]
Patch(v3)-MustPrune

r=me, thank you
Attachment #625611 - Flags: review?(surkov.alexander) → review+
Assignee: nobody → vinceyang15
https://hg.mozilla.org/mozilla-central/rev/595b826d6d3a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 757739
You need to log in before you can comment on or make changes to this bug.