[Webkit-unassigned] [Bug 72589] [GTK] Expose accessibility hierarchy in WebKit2 to ATK/AT-SPI based ATs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72589


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #121814|review?                     |review-
               Flag|                            |




--- Comment #33 from Martin Robinson <mrobinson at webkit.org>  2012-01-18 09:38:34 PST ---
(From update of attachment 121814)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.