[webkit-reviews] review denied: [Bug 230256] [GTK][a11y] Add initial implementation of accessible interface when building with ATSPI : [Attachment 439730] Patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 230256: [GTK][a11y] Add initial implementation of accessible interface when
building with ATSPI
https://bugs.webkit.org/show_bug.cgi?id=230256

Attachment 439730: Patch

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




--- Comment #3 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 439730
  --> https://bugs.webkit.org/attachment.cgi?id=439730
Patch

I have one suggestion below. Nice that this adds many tests, good work
with that!

View in context: https://bugs.webkit.org/attachment.cgi?id=439730&action=review

> Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp:175
> +static const RoleNameEntry roleNames[] = {

Given that AccessibilityRole is an enum, it seems quite wasteful to do
a linear lookup every time that the mapping has to be done. Ideally we
would use the AccsessibilityRole::Foo values as indexes into an array,
which would be trivial in but this is one of the things where C++ gets
in the way

The next best thing we can do without relying on ugly casts and that
the compiler supports C99 designated initializers while parsing C++
is using WTF::SortedArrayMap, which does binary search over the keys:

   struct RoleNameEntry {
       const char *name;
       const char *localizedName;
   };

   static constexpr std::pair<AccessibilityRole, RoleNameEntry> roleNamesList[]
= {
       { AccessibilityRole::Unknown, { "unknown", N_("unknown") } },
       // ... more entries ...
   };

Then for looking up items:

   const char* AccessibilityAtspi::localizedRoleName(AccessibilityRole role) {
       static constexpr SortedArrayMap roleNames { roleNamesList };
       if (auto entry = roleNames.tryGet(role))
	   return entry->localizedName;
       return _("unknown");
   }

This at least will be O(log n) instead of O(n) for each lookup.

(Or, if we really would need the speed, using macros to generate a big switch
statement—JSC has some of that :])