[webkit-reviews] review granted: [Bug 234787] Add a helper function to compute an element's accessibility role in core DOM code : [Attachment 448253] More refactoring

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 234787: Add a helper function to compute an element's accessibility role in
core DOM code
https://bugs.webkit.org/show_bug.cgi?id=234787

Attachment 448253: More refactoring

https://bugs.webkit.org/attachment.cgi?id=448253&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 448253
  --> https://bugs.webkit.org/attachment.cgi?id=448253
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.