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

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  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 117799: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=117799&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
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.