Closed Bug 1442280 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Disability Access APIs, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: surkov, Assigned: trisha, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 6 obsolete files)

Accessible::Role() should be replaced on Accessible::IsTableRole as it much faster, see https://searchfox.org/mozilla-central/source/accessible/base/Filters.cpp#36
Hi, I would like to work on this bug. Can I get some information on what is expected and how to get started?
(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.
Assignee: nobody → guptatrisha97
I can solve this bug as a contribution to the Mozilla Outreachy project, right?
(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 :)
36) a11y::role role = aAccessible->IsTableRole();

So basically, does this above changed code look sound enough? How do I move further with it now?
(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.
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?
(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.
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.
Attachment #8957260 - Flags: review?(surkov.alexander)
(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
Attached patch filters.patch (obsolete) — Splinter Review
Made the correct patch file, sorry for the confusion!
Attachment #8957260 - Attachment is obsolete: true
Attachment #8957260 - Flags: review?(surkov.alexander)
Attachment #8957271 - Flags: review?(surkov.alexander)
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/)
Attached patch filters.patch (obsolete) — Splinter Review
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! :)
Attachment #8957493 - Flags: review?(surkov.alexander)
(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?
Attachment #8957271 - Attachment is obsolete: true
Attachment #8957271 - Flags: review?(surkov.alexander)
Attached patch filters.patch (obsolete) — Splinter Review
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!
Attachment #8957772 - Flags: review?(surkov.alexander)
Attached patch bug1442280.patch (obsolete) — Splinter Review
This is the correct format of the patch with commit messages as well. Kindly review it for check in. Thank you.
Attachment #8957493 - Attachment is obsolete: true
Attachment #8957772 - Attachment is obsolete: true
Attachment #8957493 - Flags: review?(surkov.alexander)
Attachment #8957772 - Flags: review?(surkov.alexander)
Attachment #8958102 - Flags: review?(surkov.alexander)
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
Attached patch bug1442280.patch (obsolete) — Splinter Review
Added role declaration to the correct place. Please review.
Attachment #8958102 - Attachment is obsolete: true
Attachment #8958102 - Flags: review?(surkov.alexander)
Attachment #8958115 - Flags: review?(surkov.alexander)
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?
Attached patch bug1442280.patchSplinter Review
Here's the whole patch file. Please review and help me check-in
Attachment #8958369 - Flags: review?(surkov.alexander)
Attachment #8958115 - Attachment is obsolete: true
Attachment #8958115 - Flags: review?(surkov.alexander)
Comment on attachment 8958369 [details] [diff] [review]
bug1442280.patch

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

yep, that works, thansk!
Attachment #8958369 - Flags: review?(surkov.alexander) → review+
Keywords: checkin-needed
Is there something else here to do?
(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.
(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!
(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?
(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 :)
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7af306bede
Replaced aAccessible->Role() with Accessible::IsTable()method. r=surkov
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bc7af306bede
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: needinfo?(surkov.alexander)
what kind of info do you look?
Flags: needinfo?(surkov.alexander)
You need to log in before you can comment on or make changes to this bug.