Bug 197413 – [WPE] Add initial accessibility support using ATK

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Carlos Garcia Campos]

WPE could also use ATK optionally to have accessibility support.

[Carlos Garcia Campos]

Created attachment 368557 [details]
WIP patch

This is wip patch, but mostly complete for a first implementation. It depends on new API in libwpe that hasn't landed yet.

[Carlos Garcia Campos]

Created attachment 370043 [details]
Patch

[EWS Watchlist]

Attachment 370043 [details] did not pass style-queue:


ERROR: Tools/wpe/backends/WebKitAccessibleApplication.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 103 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

(In reply to Build Bot from comment #3)
> Attachment 370043 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/wpe/backends/WebKitAccessibleApplication.cpp:26:  Found header
> this file implements before WebCore config.h. Should be: config.h, primary
> header, blank line, and then alphabetically sorted.  [build/include_order]
> [4]
> Total errors found: 1 in 103 files

Fix it.

[Michael Catanzaro]

Comment on attachment 370043 [details]
Patch

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

> Source/WebKit/PlatformWPE.cmake:291
>  list(APPEND WebKit_LIBRARIES
>      PRIVATE
> +        ${ATK_LIBRARIES}

Heh, there's that PRIVATE again... I know this is a preexisting problem, but that's broken.

> Source/WebKit/UIProcess/API/wpe/WPEView.h:63
> +    ~View();

Should we make this class noncopyable now? Probably?

> Source/WebKit/UIProcess/API/wpe/WPEView.h:87
> +    WebKitWebViewAccessible* accessible();

I would make it const, so it can be used in const functions.

> Source/WebKit/UIProcess/API/wpe/WPEView.h:110
> +    GRefPtr<WebKitWebViewAccessible> m_accessible;

And make it mutable, so it can be set in WPEView::accessible once that becomes const.

> Source/WebKit/WebProcess/WebPage/wpe/WebPageWPE.cpp:43
> +    // entry point to the Web process, and send a message to the UI

web process

> Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp:69
> +    atkUtilClass->get_toolkit_version = []() -> const gchar* {
> +        return "";
> +    };

You could g_strdup_printf("%d.%d.%d", wpe_get_major_version(), wpe_get_minor_version(), wpe_get_micro_version()), store it in a CString cstr, and return cstr.data()?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h:76
> -#elif PLATFORM(GTK) || PLATFORM(EFL)
> +#elif USE(ATK)

Can't believe we missed this... I did a git grep to make sure there are no more places. Seems you get the credit for completely removing EFL in the end.

> Tools/wpe/backends/ViewBackend.cpp:232
> +    atkUtilClass->get_toolkit_version = []() -> const gchar* {
> +        return "";
> +    };

Ditto regarding version.

> Tools/wpe/backends/WebKitAccessibleApplication.cpp:88
> +    g_type_class_add_private(klass, sizeof(WebKitAccessibleApplicationPrivate));

You know this is deprecated? Modern GObjects should use G_ADD_PRIVATE() in the G_DEFINE_TYPE() up above, then use webkit_accessible_application_get_instance_private() to get the priv struct. Anyway, your choice.

> Tools/wpe/jhbuild.modules:292
> +  <meson id="atk" mesonargs="-Dintrospection=false">
> +    <branch module="pub/GNOME/sources/atk/2.32/atk-2.32.0.tar.xz" version="2.32.0"
> +            repo="ftp.gnome.org"
> +            hash="sha256:cb41feda7fe4ef0daa024471438ea0219592baf7c291347e5a858bb64e4091cc"/>
> +    <dependencies>
> +      <dep package="glib"/>
> +    </dependencies>
> +  </meson>
> +
> +  <meson id="at-spi2-core" mesonargs="-Dintrospection=no">

Seriously, one uses -Dintrospection=false and the other -Dintrospection=no? :S

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #4)
> (In reply to Build Bot from comment #3)
> > Attachment 370043 [details] did not pass style-queue:
> > 
> > 
> > ERROR: Tools/wpe/backends/WebKitAccessibleApplication.cpp:26:  Found header
> > this file implements before WebCore config.h. Should be: config.h, primary
> > header, blank line, and then alphabetically sorted.  [build/include_order]
> > [4]
> > Total errors found: 1 in 103 files
> 
> Fix it.

At your command! Unfortunately, it's not possible because we don't have a config.h header in Tools/wpe/backends.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 370043 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370043&action=review
> 
> > Source/WebKit/PlatformWPE.cmake:291
> >  list(APPEND WebKit_LIBRARIES
> >      PRIVATE
> > +        ${ATK_LIBRARIES}
> 
> Heh, there's that PRIVATE again... I know this is a preexisting problem, but
> that's broken.

I have no idea what this is for, I just copy-paste when I have to do CMake stuff. So, tell me what I should here, just remove it?

> > Source/WebKit/UIProcess/API/wpe/WPEView.h:63
> > +    ~View();
> 
> Should we make this class noncopyable now? Probably?

Why now?

> > Source/WebKit/UIProcess/API/wpe/WPEView.h:87
> > +    WebKitWebViewAccessible* accessible();
> 
> I would make it const, so it can be used in const functions.
> 
> > Source/WebKit/UIProcess/API/wpe/WPEView.h:110
> > +    GRefPtr<WebKitWebViewAccessible> m_accessible;
> 
> And make it mutable, so it can be set in WPEView::accessible once that
> becomes const.
> 
> > Source/WebKit/WebProcess/WebPage/wpe/WebPageWPE.cpp:43
> > +    // entry point to the Web process, and send a message to the UI
> 
> web process
> 
> > Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp:69
> > +    atkUtilClass->get_toolkit_version = []() -> const gchar* {
> > +        return "";
> > +    };
> 
> You could g_strdup_printf("%d.%d.%d", wpe_get_major_version(),
> wpe_get_minor_version(), wpe_get_micro_version()), store it in a CString
> cstr, and return cstr.data()?

This is in the web process, we could just expose versions numbers to the build instead. I know we could still use it, but it's kind of a layering violation IMO.

> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h:76
> > -#elif PLATFORM(GTK) || PLATFORM(EFL)
> > +#elif USE(ATK)
> 
> Can't believe we missed this... I did a git grep to make sure there are no
> more places. Seems you get the credit for completely removing EFL in the end.
> 
> > Tools/wpe/backends/ViewBackend.cpp:232
> > +    atkUtilClass->get_toolkit_version = []() -> const gchar* {
> > +        return "";
> > +    };
> 
> Ditto regarding version.

Tooling backends don't use the public api either. We don't even have WTF there.

> > Tools/wpe/backends/WebKitAccessibleApplication.cpp:88
> > +    g_type_class_add_private(klass, sizeof(WebKitAccessibleApplicationPrivate));
> 
> You know this is deprecated? Modern GObjects should use G_ADD_PRIVATE() in
> the G_DEFINE_TYPE() up above, then use
> webkit_accessible_application_get_instance_private() to get the priv struct.
> Anyway, your choice.

Yes, I know it, but I think we still depend on a previous version.

> > Tools/wpe/jhbuild.modules:292
> > +  <meson id="atk" mesonargs="-Dintrospection=false">
> > +    <branch module="pub/GNOME/sources/atk/2.32/atk-2.32.0.tar.xz" version="2.32.0"
> > +            repo="ftp.gnome.org"
> > +            hash="sha256:cb41feda7fe4ef0daa024471438ea0219592baf7c291347e5a858bb64e4091cc"/>
> > +    <dependencies>
> > +      <dep package="glib"/>
> > +    </dependencies>
> > +  </meson>
> > +
> > +  <meson id="at-spi2-core" mesonargs="-Dintrospection=no">
> 
> Seriously, one uses -Dintrospection=false and the other -Dintrospection=no?
> :S

Yes :-/

[Zan Dobersek]

Comment on attachment 370043 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/wpe/WPEView.h:63
>>> +    ~View();
>> 
>> Should we make this class noncopyable now? Probably?
> 
> Why now?

It's already non-copyable because of the std::unique_ptr<> members.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #7)
> I have no idea what this is for, I just copy-paste when I have to do CMake
> stuff. So, tell me what I should here, just remove it?

Yeah, remove it.

> > > Source/WebKit/UIProcess/API/wpe/WPEView.h:63
> > > +    ~View();
> > 
> > Should we make this class noncopyable now? Probably?
> 
> Why now?

Rule of three. What behavior would be sensible if it was copyable?

Zan's right, of course, in that it's already not copyable. I usually like to clarify this by deleting the copy constructor and copy assignment operator, but whatever, it's not a big deal and we're far afield of the change under consideration now.

> > > Tools/wpe/backends/WebKitAccessibleApplication.cpp:88
> > > +    g_type_class_add_private(klass, sizeof(WebKitAccessibleApplicationPrivate));
> > 
> > You know this is deprecated? Modern GObjects should use G_ADD_PRIVATE() in
> > the G_DEFINE_TYPE() up above, then use
> > webkit_accessible_application_get_instance_private() to get the priv struct.
> > Anyway, your choice.
> 
> Yes, I know it, but I think we still depend on a previous version.

I doubt it, G_ADD_PRIVATE() has been around since 2.38.

[Carlos Garcia Campos]

Created attachment 370303 [details]
Patch for landing

[EWS Watchlist]

Attachment 370303 [details] did not pass style-queue:


ERROR: Tools/wpe/backends/WebKitAccessibleApplication.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 103 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Carlos Garcia Campos from comment #7)
> > >
> > > > Source/WebKit/UIProcess/API/wpe/WPEView.h:63
> > > > +    ~View();
> > > 
> > > Should we make this class noncopyable now? Probably?
> > 
> > Why now?
> 
> Rule of three. What behavior would be sensible if it was copyable?
> 
> Zan's right, of course, in that it's already not copyable. I usually like to
> clarify this by deleting the copy constructor and copy assignment operator,
> but whatever, it's not a big deal and we're far afield of the change under
> consideration now.

You can mark a class/struct with the WTF_MAKE_NONCOPYABLE()
macro. A good thing of doing it that way is that it will still
prevent accidental copying of instances even if the members
eventually changed and suddenly it would be possible to copy
them.

[Carlos Garcia Campos]

Created attachment 370305 [details]
Patch for landing

[EWS Watchlist]

Attachment 370305 [details] did not pass style-queue:


ERROR: Tools/wpe/backends/WebKitAccessibleApplication.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 370307 [details]
Patch for landing

[EWS Watchlist]

Attachment 370307 [details] did not pass style-queue:


ERROR: Tools/wpe/backends/WebKitAccessibleApplication.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 370310 [details]
Patch for landing

[EWS Watchlist]

Attachment 370310 [details] did not pass style-queue:


ERROR: Tools/wpe/backends/WebKitAccessibleApplication.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Committed r245565: <https://trac.webkit.org/changeset/245565>