...and then use it in accessibility code, as well as a handful of other places where we currently map an element's role attribute to an `AccessibilityRole` enum type. webkit.org/b/234669 for more context.
<rdar://problem/87041580>
Created attachment 448187 [details] For EWS
Comment on attachment 448187 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=448187&action=review > Source/WebCore/dom/ElementAccessibility.h:195 > +using ARIARoleMap = HashMap<String, AccessibilityRole, ASCIICaseInsensitiveHash>; > +ARIARoleMap& ariaRoleMap(); > + > +using ARIAReverseRoleMap = HashMap<AccessibilityRole, String, DefaultHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>>; > +ARIAReverseRoleMap& reverseAriaRoleMap(); Instead of exposing maps, we should expose mapping functions: AccessibilityRole parseAccessibilityRole(StringView); ASCIILiteral accessibiltyRoleString(AccessibilityRole); Any HashMap usage should be a secret in the implementation file. This lets us do things like just use a contexpr array for the "reverse role map", looking up a name for consecutively-numbered enumeration does not require a hash table. And for parseAccessibilityRole we may want to use a SortedArrayMap instead. The initial patch can leaves the map implementations just as is, but we can then change that subsequently with a localized patch, since it’s all private to the implementation file.
Might need to take a different type rather than StringView, at first at least.
(In reply to Darin Adler from comment #3) > Comment on attachment 448187 [details] > For EWS > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448187&action=review > > > Source/WebCore/dom/ElementAccessibility.h:195 > > +using ARIARoleMap = HashMap<String, AccessibilityRole, ASCIICaseInsensitiveHash>; > > +ARIARoleMap& ariaRoleMap(); > > + > > +using ARIAReverseRoleMap = HashMap<AccessibilityRole, String, DefaultHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>>; > > +ARIAReverseRoleMap& reverseAriaRoleMap(); > > Instead of exposing maps, we should expose mapping functions: > > AccessibilityRole parseAccessibilityRole(StringView); > ASCIILiteral accessibiltyRoleString(AccessibilityRole); > > Any HashMap usage should be a secret in the implementation file. This lets > us do things like just use a contexpr array for the "reverse role map", > looking up a name for consecutively-numbered enumeration does not require a > hash table. And for parseAccessibilityRole we may want to use a > SortedArrayMap instead. > > The initial patch can leaves the map implementations just as is, but we can > then change that subsequently with a localized patch, since it’s all private > to the implementation file. Sounds good! I'll refactor it so that the hash maps are just implementation details. It also occurred to me that we don't actually need to expose a `parseAccessibilityRole` function as well, since the only use of it is internal to `accessibilityRole()`.
Created attachment 448253 [details] More refactoring
Comment on attachment 448253 [details] More refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=448253&action=review > Source/WebCore/dom/ElementAccessibility.cpp:55 > + const RoleEntry roles[] = { static constexpr > Source/WebCore/dom/ElementAccessibility.h:35 > + Annotation = 1, We really should get rid of this “1” later. > Source/WebCore/dom/ElementAccessibility.h:189 > +String accessibiltyRoleString(AccessibilityRole); Return type here should be based on how callers use this. To minimize memory use ideally we would return ASCIILiteral, not any kind of already allocated string. But if callers are performance sensitive then we could use a String return type like this so the strings can be preallocated. Further, I’d we do that we could return a const String& or const AtomString& to possibly save reference count churn. Returning any of these 3 allows us to cache the strings and return the already allocated ones as we currently do. Could be good for memory use to cache AtomString so we share memory with any actual strings used in element attributes. Costs extra memory for the entries in the AtomString table. I think I’d go with ASCIILiteral to prioritize memory efficiency over performance here. There is a typo here in the word “accessibility”, missing the “i” in “lit”. > Source/WebCore/page/ModalContainerObserver.cpp:207 > + if (auto role = accessibilityRole(element)) { Could use value_or to avoid the nesting and local variable; not sure if it’s better.
Comment on attachment 448253 [details] More refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=448253&action=review >> Source/WebCore/dom/ElementAccessibility.cpp:55 >> + const RoleEntry roles[] = { > > static constexpr 👍🏻 Made this `static constexpr`. >> Source/WebCore/dom/ElementAccessibility.h:35 >> + Annotation = 1, > > We really should get rid of this “1” later. Indeed — I left a few FIXMEs in this patch regarding this bit (in particular: the fact that it relies on being able to `static_cast` an `AccessibilityRole` to an integer value of 0, which AFAIK is UB). >> Source/WebCore/dom/ElementAccessibility.h:189 >> +String accessibiltyRoleString(AccessibilityRole); > > Return type here should be based on how callers use this. To minimize memory use ideally we would return ASCIILiteral, not any kind of already allocated string. > > But if callers are performance sensitive then we could use a String return type like this so the strings can be preallocated. Further, I’d we do that we could return a const String& or const AtomString& to possibly save reference count churn. Returning any of these 3 allows us to cache the strings and return the already allocated ones as we currently do. Could be good for memory use to cache AtomString so we share memory with any actual strings used in element attributes. Costs extra memory for the entries in the AtomString table. > > I think I’d go with ASCIILiteral to prioritize memory efficiency over performance here. > > There is a typo here in the word “accessibility”, missing the “i” in “lit”. Fixed the typo! Re: ASCIILiteral as the return type — I initially wanted to return that, but avoided doing so because I needed custom hash traits for ASCIILiteral in order to use that as a hash value: ``` namespace WTF { template<> struct HashTraits<ASCIILiteral> : GenericHashTraits<ASCIILiteral> { static ASCIILiteral emptyValue() { return ""_s; } static void constructDeletedValue(ASCIILiteral& slot) { slot = ASCIILiteral::null(); } static bool isDeletedValue(const ASCIILiteral& slot) { return slot.isNull(); } }; } // namespace WTF … using ARIAReverseRoleMap = HashMap<AccessibilityRole, ASCIILiteral, DefaultHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, HashTraits<ASCIILiteral>>; ``` That said, it probably makes more sense in the (slightly longer term) to avoid using ASCIILiteral as a hash value altogether and instead just have a `constexpr` helper function that's a big switch case (or something similar to that), so I'm not sure we'll even need the hash traits after then. If it's okay, I'll leave a FIXME for now to eliminate the hash table lookup when going from role => string and return an ASCIILiteral from `accessibilityRoleString()`? >> Source/WebCore/page/ModalContainerObserver.cpp:207 >> + if (auto role = accessibilityRole(element)) { > > Could use value_or to avoid the nesting and local variable; not sure if it’s better. I'll change this to `accessibilityRole(element).value_or(AccessibilityRole::Unknown)` to avoid the nesting.
Created attachment 448259 [details] Patch
Comment on attachment 448253 [details] More refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=448253&action=review >>> Source/WebCore/dom/ElementAccessibility.h:189 >>> +String accessibiltyRoleString(AccessibilityRole); >> >> Return type here should be based on how callers use this. To minimize memory use ideally we would return ASCIILiteral, not any kind of already allocated string. >> >> But if callers are performance sensitive then we could use a String return type like this so the strings can be preallocated. Further, I’d we do that we could return a const String& or const AtomString& to possibly save reference count churn. Returning any of these 3 allows us to cache the strings and return the already allocated ones as we currently do. Could be good for memory use to cache AtomString so we share memory with any actual strings used in element attributes. Costs extra memory for the entries in the AtomString table. >> >> I think I’d go with ASCIILiteral to prioritize memory efficiency over performance here. >> >> There is a typo here in the word “accessibility”, missing the “i” in “lit”. > > Fixed the typo! > > Re: ASCIILiteral as the return type — I initially wanted to return that, but avoided doing so because I needed custom hash traits for ASCIILiteral in order to use that as a hash value: > > ``` > namespace WTF { > > template<> struct HashTraits<ASCIILiteral> : GenericHashTraits<ASCIILiteral> { > static ASCIILiteral emptyValue() { return ""_s; } > > static void constructDeletedValue(ASCIILiteral& slot) { slot = ASCIILiteral::null(); } > static bool isDeletedValue(const ASCIILiteral& slot) { return slot.isNull(); } > }; > > } // namespace WTF > > … > > using ARIAReverseRoleMap = HashMap<AccessibilityRole, ASCIILiteral, DefaultHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, HashTraits<ASCIILiteral>>; > ``` > > That said, it probably makes more sense in the (slightly longer term) to avoid using ASCIILiteral as a hash value altogether and instead just have a `constexpr` helper function that's a big switch case (or something similar to that), so I'm not sure we'll even need the hash traits after then. > > If it's okay, I'll leave a FIXME for now to eliminate the hash table lookup when going from role => string and return an ASCIILiteral from `accessibilityRoleString()`? I think all of this might be needed only because ASCIILiteral doesn’t have a default constructor; we should try adding that and see if it works without explicitly specifying any hash traits. Since we are using it as a *value* in a hash table, not a key, we don’t need a deleted value. If we wanted to make ASCIILiteral work as a key in a hash table that would be more work.
(In reply to Darin Adler from comment #10) > Comment on attachment 448253 [details] > > That said, it probably makes more sense in the (slightly longer term) to avoid using ASCIILiteral as a hash value altogether and instead just have a `constexpr` helper function that's a big switch case (or something similar to that), so I'm not sure we'll even need the hash traits after then. > > > > If it's okay, I'll leave a FIXME for now to eliminate the hash table lookup when going from role => string and return an ASCIILiteral from `accessibilityRoleString()`? > > I think all of this might be needed only because ASCIILiteral doesn’t have a > default constructor; we should try adding that and see if it works without > explicitly specifying any hash traits. Since we are using it as a *value* in > a hash table, not a key, we don’t need a deleted value. If we wanted to make > ASCIILiteral work as a key in a hash table that would be more work. Hm...I'll give that a try, but I suspect it will crash because the constructor `String(ASCIILiteral)` expects a non-null ASCIILiteral, since it calls into length() without a null check: ``` Ref<StringImpl> StringImpl::createFromLiteral(ASCIILiteral literal) { return createFromLiteral(literal.characters(), literal.length()); } ``` ...and `length()` is just a wrapper around `strlen`.
(In reply to Wenson Hsieh from comment #11) > Hm...I'll give that a try, but I suspect it will crash because the > constructor `String(ASCIILiteral)` expects a non-null ASCIILiteral, since it > calls into length() without a null check: You can work around that by using find instead of get.
(In reply to Darin Adler from comment #12) > (In reply to Wenson Hsieh from comment #11) > > Hm...I'll give that a try, but I suspect it will crash because the > > constructor `String(ASCIILiteral)` expects a non-null ASCIILiteral, since it > > calls into length() without a null check: > > You can work around that by using find instead of get. You can work around that by turning the null ASCIILiteral into an empty string one for the return value. The question is what you want to return when the role is not in the table. I think we just want to crash.
(In reply to Darin Adler from comment #13) > (In reply to Darin Adler from comment #12) > > (In reply to Wenson Hsieh from comment #11) > > > Hm...I'll give that a try, but I suspect it will crash because the > > > constructor `String(ASCIILiteral)` expects a non-null ASCIILiteral, since it > > > calls into length() without a null check: > > > > You can work around that by using find instead of get. > > You can work around that by turning the null ASCIILiteral into an empty > string one for the return value. Yes — that should work. > > The question is what you want to return when the role is not in the table. I > think we just want to crash. Looking more carefully through the table, it seems `AccessibilityRole::Unknown` does not have a corresponding entry in this map, so we should probably fall back to the empty string (instead of null) in that case to avoid crashing.
Comment on attachment 448259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448259&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2243 > + // FIXME: This seems like it depends on undefined behavior, since 0 is not a valid AccessibilityRole enum value. > + if (static_cast<int>(*role)) > + return *role; > > return AccessibilityRole::Unknown; I’d go further and say it’s no longer needed given your use of optional. This can just be: return *role;
Created attachment 448263 [details] Return ASCIILiteral
Created attachment 448266 [details] For EWS
This seems like a good change. Sorry we missed review on it. When you find some time, could you please rebase it?
Comment on attachment 448266 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=448266&action=review > Source/WebCore/dom/ElementAccessibility.h:4 > + * Redistribution and use in source and binary forms, with or without Can we leave these in the Accessibility folder and prefix with AX?