1285272 - 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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Michael Li]

Attachment: 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

Comment 2

[Michael Li]

I don't need the extra 
>#include "ARIAMap.h"
in accessible/html/HTMLSelectAccessible.cpp, I forgot to take that out in the patch.

Comment 3

[alexander :surkov (:asurkov)]

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.

Comment 4

[Michael Li]

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 5

[Trevor Saunders (:tbsaunde)]

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.

Comment 6

[Michael Li]

Attachment: Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries. r?tbsaunde

Comment 7

[Michael Li]

Attachment: Change the Accessible class to use an index instead of raw pointer to store its role map entry variable. r?tbsaunde

Comment 8

[Michael Li]

Attachment: Change DocAccessible::BindToDocument to use an index for the role map entry. r?tbsaunde

Comment 9

[Michael Li]

Attachment: Change various classes to use new definitions of ARIARoleMap(), BindToDocument(), and SetRoleMap(). r?tbsaunde

Comment 10

[Trevor Saunders (:tbsaunde)]

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 11

[Trevor Saunders (:tbsaunde)]

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

Comment 12

[Trevor Saunders (:tbsaunde)]

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.

Comment 13

[Michael Li]

Attachment: 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.

Comment 14

[Michael Li]

Attachment: Prepare to change Accessible's role map entry variable from a raw pointer to an index. r?tbsaunde

Comment 15

[Michael Li]

Attachment: Replace use of mRoleMapEntry with calls to ARIARoleMap(). r?tbsaunde

Comment 16

[Michael Li]

Attachment: Change BindToDocument() to use a role map entry index instead of raw pointer. r?tbsaunde

Comment 17

[Michael Li]

Attachment: Change various classes to use new role map index functions and updated BindToDocument function. r?tbsaunde

Comment 18

[Trevor Saunders (:tbsaunde)]

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?

Comment 19

[Trevor Saunders (:tbsaunde)]

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.

Comment 20

[Trevor Saunders (:tbsaunde)]

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 <[e-mail]>
>
>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.

Comment 21

[Michael Li]

I did read your previous comment, I just didn't realize each patch had to build on their own, my bad.

Comment 22

[Michael Li]

Attachment: Replace direct use of mRoleMapEntry with calls to ARIARoleMap(). r?tbsaunde

Comment 23

[Michael Li]

Attachment: Change Accessible's role map entry variable from a raw pointer to an index. r?tbsaunde

Comment 24

[Michael Li]

Attachment 8769768 [details] [diff] is still the first patch of the series, I didn't make any changes to it.

Comment 25

[Trevor Saunders (:tbsaunde)]

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

Comment 26

[Yura Zenevich [:yzen] (please use "needinfo")]

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.

Comment 27

[Yura Zenevich [:yzen] (please use "needinfo")]

(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 28

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 29

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 30

[Michael Li]

Attachment: Add functionality in ARIAMap to use integer index instead of a raw pointer for role map entries.

Comment 31

[Michael Li]

Attachment: Replace direct use of mRoleMapEntry with calls to ARIARoleMap().

Comment 32

[Michael Li]

Attachment: Change Accessible's role map entry variable from a raw pointer to an index.

Comment 33

[Yura Zenevich [:yzen] (please use "needinfo")]

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?

Comment 34

[Michael Li]

Attachment: 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/

Comment 35

[Michael Li]

Attachment: 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/

Comment 36

[Michael Li]

Attachment: 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/

Comment 37

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 38

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 39

[Yura Zenevich [:yzen] (please use "needinfo")]

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.

Comment 40

[Michael Li]

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/

Comment 41

[Michael Li]

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/

Comment 42

[Michael Li]

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/

Comment 43

[Pulsebot]

Pushed by [e-mail]:
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

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4c219897749
https://hg.mozilla.org/mozilla-central/rev/d03640d31c5f
https://hg.mozilla.org/mozilla-central/rev/3baa895f4402