[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


Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #121814|0                           |1
        is obsolete|                            |
 Attachment #123143|                            |review?
               Flag|                            |




--- Comment #35 from Mario Sanchez Prada <msanchez at igalia.com>  2012-01-19 09:47:50 PST ---
Created an attachment (id=123143)
 --> (https://bugs.webkit.org/attachment.cgi?id=123143&action=review)
Patch Proposal

(In reply to comment #33)
> (From update of attachment 121814 [details])
> 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.

Thanks! I think the new patch addresses all the issus you pointed out. Just making a couple of minor comments on some of them. Find them below...

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:30
> > +#include <gtk/gtk.h>
> 
> I don't think you need the GTK+ include here.

Fixed (moved to the implementation cpp file)

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

Fixed.

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

Fixed.

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

Hmm... can't do this without getting check-webkit-style complaining about unsorted list of includes, so I left it at the beginning of the list of global (system) 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.

You're right. Thanks for pointing it out. Fixed.

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

Ok. Fixed. Updated callers.

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

Fixed.

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

Fixed.

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

Fixed (used _lookup_atspi2_binary).

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

Fixed.

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