Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries, and change the Accessible class to use this index for its role map entry variable

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: michael.li11702, Assigned: michael.li11702)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 15 obsolete attachments)

58 bytes, text/x-review-board-request
yzen
: review+
Details | Review
58 bytes, text/x-review-board-request
yzen
: review+
Details | Review
58 bytes, text/x-review-board-request
yzen
: review+
Details | Review
Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → mili
(Assignee)

Comment 1

2 years ago
Created attachment 8768827 [details] [diff] [review]
Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries, and change the Accessible class to use this index for its role map entry variable
Attachment #8768827 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 2

2 years ago
I don't need the extra 
>#include "ARIAMap.h"
in accessible/html/HTMLSelectAccessible.cpp, I forgot to take that out in the patch.
Could you put here some reasoning behind the work please? I'm sure you were discussing that offline, but it's worth to have a bug description for the record.
(Assignee)

Comment 4

2 years ago
The Accessible class was using a const pointer to store the address of its role map entry, but we don't use that pointer to access anything else other than what it's set to in Accessible::SetRoleMapEntry(), i.e., we never use the bracket operator or something similar with the pointer. So, we can replace the pointer with an integer index, which causes less issues in terms of memory management.
Comment on attachment 8768827 [details] [diff] [review]
Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries, and change the Accessible class to use this index for its role map entry variable

this is big enough and contains somewhat separate things so it would be good to break it into pieces.

The part that replaces accesses of mRoleMapEntry with a local variable and a member function seems mostly ok, so split that out first I guess.

>diff --git a/accessible/base/ARIAMap.cpp b/accessible/base/ARIAMap.cpp
>index 3a21725..bdcdf56 100644
>--- a/accessible/base/ARIAMap.cpp
>+++ b/accessible/base/ARIAMap.cpp
>@@ -882,16 +882,57 @@ aria::GetRoleMap(dom::Element* aEl)
>     }
>   }
> 
>   // Always use some entry if there is a non-empty role string
>   // To ensure an accessible object is created
>   return &sLandmarkRoleMap;
> }
> 
>+uint8_t
>+aria::GetRoleMapIndex(dom::Element* aEl)

you should use this in GetRoleMap() to reduce dupplication.

>+const nsRoleMapEntry*
>+aria::GetRoleMapFromIndex(uint8_t aRoleMapIndex) {

{ on next line

>+++ b/accessible/base/nsAccessibilityService.cpp
>+    document->BindToDocument(newAcc, aria::GetRoleMapIndex(content->AsElement()));
>     return newAcc;
>   }
> 
>   const nsRoleMapEntry* roleMapEntry = aria::GetRoleMap(content->AsElement());
>+  uint8_t roleMapEntryIndex = aria::GetRoleMapIndex(content->AsElement());

its not great to dupplicate the roleMap stuff that's already here.  One option might be to keep passing a pointer to Accessible::SetRoleMapEntry() and just allow translating a pointer to a index, or you might think of something else reasonable, but it should be possible to do something better than this.

>+bool 
>+Accessible::HasARIARole() const {
>+  return mRoleMapEntryIndex != aria::NO_ROLE_MAP_ENTRY_INDEX;

why are you uninlining it? it seems short enough it should still be inline.

>+Accessible::ARIARoleMap() const {
>+  return aria::GetRoleMapFromIndex(mRoleMapEntryIndex);

same

>   /**
>-   * Non-null indicates author-supplied role; possibly state & value as well
>+   * Non-NO_ROLE_MAP_ENTRY_INDEX indicates author-supplied role; possibly state
>+   * & value as well
>    */
>-  const nsRoleMapEntry* mRoleMapEntry;
>+  uint8_t mRoleMapEntryIndex;

it would be good to move it next to the bitfields to get better packing, but you can do that separately after making it an index.

>       mListAccessible = new HTMLComboboxListAccessible(mParent, mContent, mDoc);
>-      Document()->BindToDocument(mListAccessible, nullptr);
>+      Document()->BindToDocument(mListAccessible, mozilla::a11y::aria::NO_ROLE_MAP_ENTRY_INDEX);

mozilla::a11y:: shouldn't be needed here, and in several other spots.
Attachment #8768827 - Flags: review?(tbsaunde+mozbugs) → review-
(Assignee)

Comment 6

2 years ago
Created attachment 8769236 [details] [diff] [review]
Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries. r?tbsaunde
Attachment #8769236 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 7

2 years ago
Created attachment 8769239 [details] [diff] [review]
Change the Accessible class to use an index instead of raw pointer to store its role map entry variable. r?tbsaunde
Attachment #8769239 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 8

2 years ago
Created attachment 8769253 [details] [diff] [review]
Change DocAccessible::BindToDocument to use an index for the role map entry. r?tbsaunde
Attachment #8769253 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 9

2 years ago
Created attachment 8769258 [details] [diff] [review]
Change various classes to use new definitions of ARIARoleMap(), BindToDocument(), and SetRoleMap(). r?tbsaunde
Attachment #8769258 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Updated

2 years ago
Attachment #8768827 - Attachment is obsolete: true
So, in general it looks like some of these patches still do multiple things that can be broken into their own steps.  Also patches should only depend on previous patches, not any future patches, and I'm pretty sure this series doesn't do that.

not all parts apply, but https://github.com/git/git/blob/master/Documentation/SubmittingPatches is a worthwhile read.
Comment on attachment 8769239 [details] [diff] [review]
Change the Accessible class to use an index instead of raw pointer to store its role map entry variable. r?tbsaunde

>bug 1285272 - Change the Accessible class to use an index instead of raw pointer to store its role map entry variable. r?tbsaunde

So, this is basically two things first replace a number of direct uses of mRoleMapEntry with calls to ARIARoleMap() then after this and any other preparatory steps change mRoleMapEntry to a uint8_t.

>+inline bool 
>+Accessible::HasARIARole() const {

{ on next line

>+inline const nsRoleMapEntry* 
>+Accessible::ARIARoleMap() const {

same
Attachment #8769239 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8769258 [details] [diff] [review]
Change various classes to use new definitions of ARIARoleMap(), BindToDocument(), and SetRoleMap(). r?tbsaunde

>@@ -1122,32 +1122,34 @@ nsAccessibilityService::CreateAccessible(nsINode* aNode,
>                                               frame->GetParent()).IsEmpty()) {
>       if (aIsSubtreeHidden)
>         *aIsSubtreeHidden = true;
> 
>       return nullptr;
>     }
> 
>     newAcc = new HyperTextAccessibleWrap(content, document);
>-    document->BindToDocument(newAcc, aria::GetRoleMap(content->AsElement()));
>+    document->BindToDocument(newAcc, aria::GetRoleMapIndex(content->AsElement()));
>     return newAcc;
>   }
> 
>-  const nsRoleMapEntry* roleMapEntry = aria::GetRoleMap(content->AsElement());
>+  uint8_t roleMapEntryIndex = aria::GetRoleMapIndex(content->AsElement());
>+  const nsRoleMapEntry* roleMapEntry =
>+    aria::GetRoleMapFromIndex(roleMapEntryIndex);

hmm, so now roleMapEntry and roleMapEntryIdx start out equal but then they can diverge when roleMapEntryIdx is changed, but roleMapEntry isn't.  That might be ok, though its at least rather dangerous, and quiet possibly buggy.

It seems like its tricky to completely switch away from pointers here, so I'm tempted to say the best route is to keep passing a nsRoleMapEntry* to bindToDocument() and then convert that to an integer in SetRoleMapEntry().  Its not ideal, but should be a smaller patch and will certainly work.  However I'm not sure there aren't alternatives so you can try to think of one if you want.
Attachment #8769258 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 13

2 years ago
Created attachment 8769768 [details] [diff] [review]
Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries. r?tbsaunde Also add functionality to convert between raw pointers and indexes for role map entries.
Attachment #8769768 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 14

2 years ago
Created attachment 8769798 [details] [diff] [review]
Prepare to change Accessible's role map entry variable from a raw pointer to an index. r?tbsaunde
Attachment #8769798 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 15

2 years ago
Created attachment 8769823 [details] [diff] [review]
Replace use of mRoleMapEntry with calls to ARIARoleMap(). r?tbsaunde
Attachment #8769823 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 16

2 years ago
Created attachment 8769825 [details] [diff] [review]
Change BindToDocument() to use a role map entry index instead of raw pointer. r?tbsaunde
Attachment #8769825 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 17

2 years ago
Created attachment 8769830 [details] [diff] [review]
Change various classes to use new role map index functions and updated BindToDocument function. r?tbsaunde
Attachment #8769830 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Updated

2 years ago
Attachment #8769258 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8769253 - Attachment is obsolete: true
Attachment #8769253 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Updated

2 years ago
Attachment #8769239 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8769236 - Attachment is obsolete: true
Attachment #8769236 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8769825 [details] [diff] [review]
Change BindToDocument() to use a role map entry index instead of raw pointer. r?tbsaunde

>   void BindToDocument(Accessible* aAccessible,
>-                      const nsRoleMapEntry* aRoleMapEntry);
>+                      uint8_t aRoleMapEntryIndex);

I'm pretty sure this won't work independent of the later patches.  DId you my previous comment on that subject / did you check that this patch builds without the following ones?
Attachment #8769825 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8769823 [details] [diff] [review]
Replace use of mRoleMapEntry with calls to ARIARoleMap(). r?tbsaunde

>+Accessible::HasARIARole() const
>+{
>+  return mRoleMapEntryIndex != aria::NO_ROLE_MAP_ENTRY_INDEX;

this isn't related to the subject of the patch.
Attachment #8769823 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8769798 [details] [diff] [review]
Prepare to change Accessible's role map entry variable from a raw pointer to an index. r?tbsaunde

># HG changeset patch
># User Michael Li <michael.li11702@gmail.com>
>
>bug 1285272 - Prepare to change Accessible's role map entry variable from a raw pointer to an index. r?tbsaunde

this patch doesn't prepare, it actually does it.

I haven't carefully looked over this version in full, but I've got to head out for a while.

I'm not sure if you do, but you should try and look over patches carefully before posting them to save yourself time.
(Assignee)

Comment 21

2 years ago
I did read your previous comment, I just didn't realize each patch had to build on their own, my bad.
(Assignee)

Updated

2 years ago
Attachment #8769798 - Attachment is obsolete: true
Attachment #8769798 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Updated

2 years ago
Attachment #8769823 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8769825 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8769830 - Attachment is obsolete: true
Attachment #8769830 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 22

2 years ago
Created attachment 8770278 [details] [diff] [review]
Replace direct use of mRoleMapEntry with calls to ARIARoleMap(). r?tbsaunde
Attachment #8770278 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 23

2 years ago
Created attachment 8770279 [details] [diff] [review]
Change Accessible's role map entry variable from a raw pointer to an index. r?tbsaunde
Attachment #8770279 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 24

2 years ago
Attachment 8769768 [details] [diff] is still the first patch of the series, I didn't make any changes to it.
Attachment #8770279 - Flags: review?(tbsaunde+mozbugs) → review?(yzenevich)
Attachment #8770278 - Flags: review?(tbsaunde+mozbugs) → review?(yzenevich)
Comment on attachment 8769768 [details] [diff] [review]
Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries. r?tbsaunde Also add functionality to convert between raw pointers and indexes for role map entries.

>+const nsRoleMapEntry*
>+aria::GetRoleMapFromIndex(uint8_t aRoleMapIndex) {

{ on next line
Attachment #8769768 - Flags: review?(tbsaunde+mozbugs) → review?(yzenevich)
Comment on attachment 8769768 [details] [diff] [review]
Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries. r?tbsaunde Also add functionality to convert between raw pointers and indexes for role map entries.

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

looks good with some nits.

::: accessible/base/ARIAMap.h
@@ +202,5 @@
>   */
>  extern nsRoleMapEntry gEmptyRoleMap;
>  
>  /**
> + * Constants for the role map entry index to indicate that the role map entry 

nit: whitespace at the end of the line

@@ +221,5 @@
>   */
>  const nsRoleMapEntry* GetRoleMap(dom::Element* aEl);
>  
>  /**
> + * Get the role map entry pointer's index for a given DOM node. This will use 

Nit: whitespace at the end.

@@ +233,5 @@
> +uint8_t GetRoleMapIndex(dom::Element* aEl);
> +
> +/**
> + * Get the role map entry pointer for a given role map entry index.
> + * 

nit: here and below - whitespace.
Attachment #8769768 - Flags: review?(yzenevich) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #25)
> Comment on attachment 8769768 [details] [diff] [review]
> Add functionality in ARIAMap to use integer index instead of a raw pointer
> for role map entries. r?tbsaunde Also add functionality to convert between
> raw pointers and indexes for role map entries.
> 
> >+const nsRoleMapEntry*
> >+aria::GetRoleMapFromIndex(uint8_t aRoleMapIndex) {
> 
> { on next line

Note also this comment.
Comment on attachment 8770278 [details] [diff] [review]
Replace direct use of mRoleMapEntry with calls to ARIARoleMap(). r?tbsaunde

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

this looks good but could you re-base it as it does not apply for me:

error: patch failed: accessible/generic/Accessible-inl.h:12
error: accessible/generic/Accessible-inl.h: patch does not apply
Attachment #8770278 - Flags: review?(yzenevich) → review+
Comment on attachment 8770279 [details] [diff] [review]
Change Accessible's role map entry variable from a raw pointer to an index. r?tbsaunde

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

looks good thanks!

::: accessible/generic/Accessible-inl.h
@@ +23,5 @@
>  
>    return ARIATransformRole(roleMapEntry->role);
>  }
>  
> +inline bool 

nit: whitespace

@@ +59,5 @@
>  
>    return ARIATransformRole(roleMapEntry->role);
>  }
>  
> +inline void 

nit: whitespace
Attachment #8770279 - Flags: review?(yzenevich) → review+
(Assignee)

Comment 30

2 years ago
Created attachment 8772019 [details] [diff] [review]
Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries.
Attachment #8772019 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8769768 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
Created attachment 8772021 [details] [diff] [review]
Replace direct use of mRoleMapEntry with calls to ARIARoleMap().
Attachment #8772021 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8770278 - Attachment is obsolete: true
(Assignee)

Comment 32

2 years ago
Created attachment 8772030 [details] [diff] [review]
Change Accessible's role map entry variable from a raw pointer to an index.
Attachment #8772030 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8770279 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8772021 - Flags: review+ → review?(yzenevich)
Hi Michael, I now can't apply the "Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries." patch. Could you check that you're up to date with the latest trunk?
Flags: needinfo?(mili)
(Assignee)

Comment 34

2 years ago
Created attachment 8772434 [details]
bug 1285272 - Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries.

Review commit: https://reviewboard.mozilla.org/r/65244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65244/
(Assignee)

Comment 35

2 years ago
Created attachment 8772435 [details]
bug 1285272 - Replace direct use of mRoleMapEntry with calls to ARIARoleMap().

Review commit: https://reviewboard.mozilla.org/r/65246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65246/
(Assignee)

Comment 36

2 years ago
Created attachment 8772436 [details]
bug 1285272 - Change Accessible's role map entry variable from a raw pointer to an index.

Review commit: https://reviewboard.mozilla.org/r/65248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65248/
(Assignee)

Updated

2 years ago
Attachment #8772434 - Flags: review?(yzenevich)
(Assignee)

Updated

2 years ago
Flags: needinfo?(mili)
(Assignee)

Updated

2 years ago
Attachment #8772019 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8772030 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8772021 - Attachment is obsolete: true
Attachment #8772021 - Flags: review?(yzenevich)
(Assignee)

Updated

2 years ago
Attachment #8772435 - Flags: review?(yzenevich)
Attachment #8772436 - Flags: review?(yzenevich)
Comment on attachment 8772434 [details]
bug 1285272 - Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries.

https://reviewboard.mozilla.org/r/65244/#review62926

looks good thanks
Attachment #8772434 - Flags: review?(yzenevich) → review+
Comment on attachment 8772435 [details]
bug 1285272 - Replace direct use of mRoleMapEntry with calls to ARIARoleMap().

https://reviewboard.mozilla.org/r/65246/#review62928

looks good
Attachment #8772435 - Flags: review?(yzenevich) → review+
Comment on attachment 8772436 [details]
bug 1285272 - Change Accessible's role map entry variable from a raw pointer to an index.

https://reviewboard.mozilla.org/r/65248/#review62930

thanks. Make sure that all commits have r=yzen at the end before checking in.
Attachment #8772436 - Flags: review?(yzenevich) → review+
(Assignee)

Comment 40

2 years ago
Comment on attachment 8772434 [details]
bug 1285272 - Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65244/diff/1-2/
(Assignee)

Comment 41

2 years ago
Comment on attachment 8772435 [details]
bug 1285272 - Replace direct use of mRoleMapEntry with calls to ARIARoleMap().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65246/diff/1-2/
(Assignee)

Comment 42

2 years ago
Comment on attachment 8772436 [details]
bug 1285272 - Change Accessible's role map entry variable from a raw pointer to an index.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65248/diff/1-2/
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 43

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c219897749
Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries. r=yzen
https://hg.mozilla.org/integration/mozilla-inbound/rev/d03640d31c5f
Replace direct use of mRoleMapEntry with calls to ARIARoleMap(). r=yzen
https://hg.mozilla.org/integration/mozilla-inbound/rev/3baa895f4402
Change Accessible's role map entry variable from a raw pointer to an index. r=yzen
Keywords: checkin-needed

Comment 44

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e4c219897749
https://hg.mozilla.org/mozilla-central/rev/d03640d31c5f
https://hg.mozilla.org/mozilla-central/rev/3baa895f4402
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

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