[webkit-reviews] review granted: [Bug 230255] [GTK][a11y] Connect UI process a11y tree with the web process when building with ATSPI : [Attachment 438464] Patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 230255: [GTK][a11y] Connect UI process a11y tree with the web process when
building with ATSPI
https://bugs.webkit.org/show_bug.cgi?id=230255

Attachment 438464: Patch

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




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

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

> Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp:72
> +		   auto id =
g_dbus_connection_register_object(m_connection.get(), path.utf8().data(),
interface.first, interface.second, rootObject.ptr(), nullptr, nullptr);

This does not handle the case in which object registration fails (i.e. the
returned
value is 0). OTOH there is not much we can do here to recover from that
failure,
and should be a rare event,  but it may be good to at least log the error to
help
debugging in the future. At any rate, we should not store zero-ids in the
“registeredObjects” vector and use them later assuming that they are valid ids.

> Source/WebCore/accessibility/atspi/AccessibilityAtspi.h:38
> +class AccessibilityAtspi {

This can be “class AccesibilityAtspi final“ =)

> Source/WebCore/accessibility/atspi/AccessibilityRootAtspi.cpp:110
> +	       return g_variant_new_string("");

Values returned here look like stub code. Either add a TODO here to
complete it in a follow-up patch, or a comment explaining why these
do not need anything more complex.

> Source/WebCore/accessibility/atspi/AccessibilityRootAtspi.h:34
> +class AccessibilityRootAtspi : public
ThreadSafeRefCounted<AccessibilityRootAtspi> {

This class can also be “final”.