Note: There are a few cases of duplicates in user autocompletion which are being worked on.

MustPrune should take nsAccessible*

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: Xi Yang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
(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?
(Reporter)

Comment 2

5 years ago
please ask
(Assignee)

Comment 3

5 years ago
(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!
(Reporter)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
(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 ...
(Reporter)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
Hi, just wondering where I can find the nsIAccessible.h? Many thanks!
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
(Assignee)

Comment 10

5 years ago
(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!
(Reporter)

Comment 11

5 years ago
(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
(Assignee)

Comment 12

5 years ago
Created attachment 625404 [details] [diff] [review]
change the accessible/src/base/nsAccUtils::MustPrune to nsAccessible*
Attachment #625404 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #625404 - Flags: review? → review?(surkov.alexander)
(Assignee)

Updated

5 years ago
Attachment #625404 - Flags: review?(surkov.alexander) → review?
(Reporter)

Comment 13

5 years ago
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?
(Assignee)

Comment 14

5 years ago
Created attachment 625417 [details] [diff] [review]
Patch2-MustPrune
Attachment #625417 - Flags: review?(surkov.alexander)
(Reporter)

Comment 15

5 years ago
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+
(Assignee)

Comment 16

5 years ago
Created attachment 625611 [details] [diff] [review]
Patch(v3)-MustPrune
Attachment #625611 - Flags: review?(surkov.alexander)
(Reporter)

Updated

5 years ago
Attachment #625404 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #625417 - Attachment is obsolete: true
(Reporter)

Comment 17

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

r=me, thank you
Attachment #625611 - Flags: review?(surkov.alexander) → review+
(Reporter)

Updated

5 years ago
Assignee: nobody → vinceyang15
(Reporter)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/595b826d6d3a
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/595b826d6d3a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 757739
You need to log in before you can comment on or make changes to this bug.