[webkit-reviews] review granted: [Bug 197413] [WPE] Add initial accessibility support using ATK : [Attachment 370043] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 197413: [WPE] Add initial accessibility support using ATK
https://bugs.webkit.org/show_bug.cgi?id=197413

Attachment 370043: Patch

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




--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 370043
  --> https://bugs.webkit.org/attachment.cgi?id=370043
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