[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 #117752|0                           |1
        is obsolete|                            |
 Attachment #117752|review?                     |
               Flag|                            |
 Attachment #117799|                            |review?
               Flag|                            |




--- Comment #4 from Mario Sanchez Prada <msanchez at igalia.com>  2011-12-04 11:37:55 PST ---
Created an attachment (id=117799)
 --> (https://bugs.webkit.org/attachment.cgi?id=117799&action=review)
Patch proposal

Attaching a new patch addressing the issues pointed out by Gustavo, as well as some improvements/fixes I found useful during this time...

(In reply to comment #3)
> [...]
> > Source/WebKit2/ChangeLog:11
> > +        Implement the part in the UIProcess, by making the WebView widget
> 
> I think you can remove 'Implement the part in the UIProcess' pattern and have just the explanation.

Done.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:136
> > +    if (priv->accessible) {
> > +        g_object_unref(priv->accessible);
> > +        priv->accessible = 0;
> > +    }
> > +
> 
> Is there a reason why we don't use GRefPtr for these?

There's one reason, but it's not a good one: I just forgot.

Fixed now.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:357
> > +    // If the socket as already been created and embedded a plug ID, return it.
> 
> s/as/has/

Fixed

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:378
> > +    gchar* plugIDString = g_strdup(plugID.utf8().data());
> 
> GOwnPtr?

You're right. Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.cpp:68
> > +        /* If the widget is no longer alive, save some remote calls
> > +           (because of AtkSocket's implementation of ref_state_set())
> > +           and just return that this AtkObject is defunct. */
> 
> Comments should use C++ style, with // at the start of each line.

Oops! Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBaseAccessible.h:55
> > +struct _WebKitWebViewBaseAccessible {
> > +    AtkSocket parent;
> > +    /*< private >*/
> > +    WebKitWebViewBaseAccessiblePrivate* priv;
> > +};
> 
> No need for padding?

This WebKitWebViewBaseAccessible.h file is not meant to be included in the webkit2gtk headers, as it's gonna be only used internally inside WebKit. In other words, from the outside (e.g. epiphany, the Atk bridge...) you will never see instances of WebKitWebViewBaseAccessible, but instances of AtkObject. So, I don't think there's need for adding any extra padding in this case.

> > Source/WebKit2/UIProcess/API/gtk/tests/AccessibilityTestServer.cpp:38
> > +    // Sent a message to the parent process telling we're ready.
> 
> s/Sent/Send/

Done.

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