Bug 230255 – [GTK][a11y] Connect UI process a11y tree with the web process when building with ATSPI

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Add a plug object in web process to connect with the AtkSocket from the UI process.

[Carlos Garcia Campos]

Created attachment 438458 [details]
Patch

[Carlos Garcia Campos]

hmm, it seems DerivedSources/WebKit/WebPageProxyMessages.h hasn't been regenerated even though Source/WebKit/UIProcess/WebPageProxy.messages.in has been modified. I guess this needs a clean build, but it shouldn't be needed.

[Carlos Garcia Campos]

Ok, I see the error now... it was my fault, I'll submit a new patch.

[Carlos Garcia Campos]

Created attachment 438464 [details]
Patch

[Adrian Perez]

Comment on attachment 438464 [details]
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”.

[Carlos Garcia Campos]

Comment on attachment 438464 [details]
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.

Well, this can only fail if the same object path is registered twice for the same interface. We ensure we don't register the same object twice, but it's still impossible to happen because the object path is unique (contains a UUID).

>> Source/WebCore/accessibility/atspi/AccessibilityAtspi.h:38
>> +class AccessibilityAtspi {
> 
> This can be “class AccesibilityAtspi final“ =)

This is not a derived class and no other class will derive from this, so no virtual functions at all. Does final do anything useful in this case?

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

They are not, except ChildCount that will be implemented in follow up commit. This is just a filler used to connect UI and web process trees, so the name and description are not really useful for ATs.

[Carlos Garcia Campos]

Committed r283304 (242329@main): <https://commits.webkit.org/242329@main>

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #6)
> Comment on attachment 438464 [details]
> 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.
> 
> Well, this can only fail if the same object path is registered twice for the
> same interface. We ensure we don't register the same object twice, but it's
> still impossible to happen because the object path is unique (contains a
> UUID).

Nice, thanks for the clarification—I didn't have the time to go and
check the GLib code to see under which circumstances it would fail O:-)

> >> Source/WebCore/accessibility/atspi/AccessibilityAtspi.h:38
> >> +class AccessibilityAtspi {
> > 
> > This can be “class AccesibilityAtspi final“ =)
> 
> This is not a derived class and no other class will derive from this, so no
> virtual functions at all. Does final do anything useful in this case?

Well, IMO it is still valuable to add the type qualifier because it
prevents accidentally making subclasses, or if it's made a subclass
of something else in the future prevents missing virtual method
implementations, and so on.

It would be actually a good thing if C++ would have a toggle to make
all classes “final” unless written otherwise in the code ;-)

> >> 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.
> 
> They are not, except ChildCount that will be implemented in follow up
> commit. This is just a filler used to connect UI and web process trees, so
> the name and description are not really useful for ATs.

Thanks for the clarification. You could have written this as a comment
in the code because anybody looking at it will wonder why some values
are empty. Maybe it's obvious if one is deep into a11y, but it would
not hurt having the note there.