1442280 - don't use Accessible:Role() in filters::GetRow

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[alexander :surkov (:asurkov)]

Accessible::Role() should be replaced on Accessible::IsTableRole as it much faster, see https://searchfox.org/mozilla-central/source/accessible/base/Filters.cpp#36

Comment 1

[Trisha Gupta [:trisha]]

Hi, I would like to work on this bug. Can I get some information on what is expected and how to get started?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Trisha from comment #1)
> Hi, I would like to work on this bug. Can I get some information on what is
> expected and how to get started?

hey, Trisha. I assigned bug to you. Instructions are in bug description (comment #0), if those look unclear, please don't hesitate to ask questions.

Comment 3

[Trisha Gupta [:trisha]]

I can solve this bug as a contribution to the Mozilla Outreachy project, right?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trisha from comment #3)
> I can solve this bug as a contribution to the Mozilla Outreachy project,
> right?

I'm not sure what are requirements of moz outreachy project, it'd better to ask them. I can say only that if you look for a good-first-bug/mentored moz bugs (for any reason), then you are in right place :)

Comment 5

[Trisha Gupta [:trisha]]

36) a11y::role role = aAccessible->IsTableRole();

So basically, does this above changed code look sound enough? How do I move further with it now?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trisha from comment #5)
> 36) a11y::role role = aAccessible->IsTableRole();
> 
> So basically, does this above changed code look sound enough? How do I move
> further with it now?

aAccessible->IsTableRole() returns a boolean, not a11y::role, and you can use whenever it need, i.e. you don't have to store its result in variable.

Comment 7

[Trisha Gupta [:trisha]]

So far I have come up with this. How does it look?

uint32_t
filters::GetRow(Accessible* aAccessible)
{
  if (aAccessible->IsTableRole() & roles::ROW)
    return eMatch | eSkipSubtree;

  // Look for rows inside rowgroup.
  if (aAccessible->IsTableRole() & roles::GROUPING)
    return eSkip;

  return eSkipSubtree;
}

I tried finding more documentation about aAccessible->IsTableRole() but could not find it anywhere. Can you give me some links please? Also, where can I find the header files for Role and States?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trisha from comment #7)
> So far I have come up with this. How does it look?
> 
> uint32_t
> filters::GetRow(Accessible* aAccessible)
> {
>   if (aAccessible->IsTableRole() & roles::ROW)

it doesn't make sense, because the IsTable() method returns a boolean, not a11y::role type

>     return eMatch | eSkipSubtree;
> 
>   // Look for rows inside rowgroup.
>   if (aAccessible->IsTableRole() & roles::GROUPING)

same here, however Accessible class doesn't have IsGrouping() method, so either you should add one or leave this line unchanged, because it may become too complicated for a good-first-bug.

>     return eSkip;
> 
>   return eSkipSubtree;
> }
> 
> I tried finding more documentation about aAccessible->IsTableRole() but
> could not find it anywhere. Can you give me some links please? Also, where
> can I find the header files for Role and States?

I was referring to Accessible::IsTable() method (https://searchfox.org/mozilla-central/source/accessible/generic/Accessible.h#642). You can use searchfox.org to search and browser the code.

Comment 9

[Trisha Gupta [:trisha]]

Attachment: Replaced aAccessible->Role() with Accessible::IsTable()method

Instead of storing aAccessible->Role() in a11y::role and then checking for roles::ROW, I have used the if condition to check for the row using Accessible::IsTable() method. This will make the code faster and hopefully fix the bug.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trisha from comment #9)
> Created attachment 8957260 [details] [diff] [review]
> Replaced aAccessible->Role() with Accessible::IsTable()method

it says it's not valid patch, did you follow these steps? https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch

Comment 11

[Trisha Gupta [:trisha]]

Attachment: filters.patch

Made the correct patch file, sorry for the confusion!

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 8957271 [details] [diff] [review]
filters.patch

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

::: filters.cpp
@@ +29,4 @@
>  uint32_t
>  filters::GetRow(Accessible* aAccessible)
>  {
> + if(aAccessible->IsTableRow())

nit: please add a space after 'if'

btw, you should make sure the patch is built successfully (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/)

Comment 13

[Trisha Gupta [:trisha]]

Attachment: filters.patch

I added the space as you mentioned. I asked in the #fx team IRC if this patch is acceptable and they said it is. Let me know if you want the patch to be in a specific version though, I'll make the changes then! :)

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trisha from comment #13)
> I asked in the #fx team IRC if this
> patch is acceptable and they said it is.

I'd say it's hardly can replace the build process :) I think your patch won't compile, but my point is it is good idea to test the patch (at least build it), before you ask for review.

> Let me know if you want the patch
> to be in a specific version though, I'll make the changes then! :)

I didn't catch about version specific patches. What is it?

Comment 15

[Trisha Gupta [:trisha]]

Attachment: filters.patch

I realized my fault. Don't understand why the patch did not give error then though. I have kept the role declaration but, the if condition, I have changed so as to deal with the current bug. Hope this is fine!

Comment 16

[Trisha Gupta [:trisha]]

Attachment: bug1442280.patch

This is the correct format of the patch with commit messages as well. Kindly review it for check in. Thank you.

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 8958102 [details] [diff] [review]
bug1442280.patch

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

::: accessible/base/Filters.cpp
@@ +34,3 @@
>  filters::GetRow(Accessible* aAccessible)
>  {
>    a11y::role role = aAccessible->Role();

please move 'role' variable declaration closer to a place where it is used

Comment 18

[Trisha Gupta [:trisha]]

Attachment: bug1442280.patch

Added role declaration to the correct place. Please review.

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 8958115 [details] [diff] [review]
bug1442280.patch

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

it seems this a diff from previous patch, could you please attach the whole patch?

Comment 20

[Trisha Gupta [:trisha]]

Attachment: bug1442280.patch

Here's the whole patch file. Please review and help me check-in

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 8958369 [details] [diff] [review]
bug1442280.patch

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

yep, that works, thansk!

Comment 22

[anirudh.pandev]

Is there something else here to do?

Comment 23

[alexander :surkov (:asurkov)]

(In reply to anirudh.pandev from comment #22)
> Is there something else here to do?

Nothing in particular. Your patch will be landed within a day or so. It'd be good to file a follow up bug to remove Role() call from this method completely. And if you like then you could pick it up for sure :) That one will be a bit more challenging than this one,  but still doable.

Comment 24

[anirudh.pandev]

(In reply to alexander :surkov from comment #23)
> (In reply to anirudh.pandev from comment #22)
> > Is there something else here to do?
> 
> Nothing in particular. Your patch will be landed within a day or so. It'd be
> good to file a follow up bug to remove Role() call from this method
> completely. And if you like then you could pick it up for sure :) That one
> will be a bit more challenging than this one,  but still doable.

Definately giving a try!

Comment 25

[Trisha Gupta [:trisha]]

(In reply to alexander :surkov from comment #23)
> (In reply to anirudh.pandev from comment #22)
> > Is there something else here to do?
> 
> Nothing in particular. Your patch will be landed within a day or so. It'd be
> good to file a follow up bug to remove Role() call from this method
> completely. And if you like then you could pick it up for sure :) That one
> will be a bit more challenging than this one,  but still doable.

Did @anirudh also file in a patch for this? 

Is it okay if I can also work on filing the bug to try to remove Role() call from this method as I have already done that in this?

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Trisha from comment #25)
> (In reply to alexander :surkov from comment #23)
> > (In reply to anirudh.pandev from comment #22)
> > > Is there something else here to do?
> > 
> > Nothing in particular. Your patch will be landed within a day or so. It'd be
> > good to file a follow up bug to remove Role() call from this method
> > completely. And if you like then you could pick it up for sure :) That one
> > will be a bit more challenging than this one,  but still doable.
> 
> Did @anirudh also file in a patch for this? 

no, I should have read commenter's names before answering questions.

> Is it okay if I can also work on filing the bug to try to remove Role() call
> from this method as I have already done that in this?

sure, just please make sure to not end up filing two bugs and/or starting work both on same bug :) we have too many bugs to clash over them :)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7af306bede
Replaced aAccessible->Role() with Accessible::IsTable()method. r=surkov

Comment 28

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/bc7af306bede

Comment 29

[alexander :surkov (:asurkov)]

what kind of info do you look?