[webkit-reviews] review denied: [Bug 72589] [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based ATs : [Attachment 121814] Patch proposal

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 72589: [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based
ATs
https://bugs.webkit.org/show_bug.cgi?id=72589

Attachment 121814: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=121814&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121814&action=review


This look fabulous! I have a bunch of really minor comments below and one or
two larger concerns.

> Source/WebKit2/ChangeLog:9
> +	   architecture of WK2 through AtkSocket ad AtkPlug.

ad -> and

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:2
> + * Copyright (C) 2011 Igalia S.L.

This should be 2012 now.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:12
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *	 notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *	 notice, this list of conditions and the following disclaimer in the
> + *	 documentation and/or other materials provided with the distribution.
> + *

Let's use LPGL here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:7
> + * Copyright (C) 2011 Igalia S.L.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright

2012 and LGPL again.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:30
> +#include <gtk/gtk.h>

I don't think you need the GTK+ include here.

> Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:2
> + * Copyright (C) 2011 Igalia S.L.

2012

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:2
> + * Copyright (C) 2011 Igalia S.L.

2012

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp:28
> +// The libatspi headers don't use G_BEGIN_DECLS
> +extern "C" {
> +#include <atspi/atspi.h>
> +}

Drop this down to the last include if possible to avoid breaking the block of
includes.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:524
> +#if PLATFORM(GTK)
> +    // Ensure the accessibility hierarchy is updated after loading.
> +    webPage->updateAccessibilityTree();
> +#endif

Are you trying to find the right time to hook into the DOM? If so this should
be in ::dispatchDidClearWindowObjectInWorld, I believe.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:1
> +/*

These files should be called
Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObject.cpp/.h because
they aren't GTK+ specific implementation files of cross-platform classes.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:2
> + * Copyright (C) 2011 Igalia S.L.

2012 here.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageAccessibilityObjectGtk.cpp:94
> +    if (index < 0 || index > 1)

index != 0 && index != 1?

> Tools/Scripts/run-gtk-tests:67
> +    def _lookupAtspi2Binary(self, jhbuild_path, filename):

I'm pretty sure PEP-8 naming conventions say this should be
_lookup_atspi2_binary or _find_atspi2_binary, but I'm not certain here.

> Tools/Scripts/run-gtk-tests:95
> +	   # Make sure there's no X's property AT_SPI_BUS previously set,

X's property AT_SPI_BUS -> AT_SPI_BUS X property?