[GTK] accessibility/placeholder.html is failing

[Zan Dobersek]

accessibility/placeholder.html is failing on all GTK platforms.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=accessibility%2Fplaceholder.html

[Joanmarie Diggs]

On the surface, this is failing due to the need to implement AccessibilityUIElement::stringAttributeValue()

Having said that, I'm not seeing the placeholder text exposed to us when viewing the test case in Accerciser. So this looks like something that never got implemented in the first place.

I'll take this one.

[Joanmarie Diggs]

Created attachment 178434 [details]
Patch

[Martin Robinson]

Comment on attachment 178434 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178434&action=review

Looks good, but I have a few very small quibbles.

> Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:40
> +static inline String coreAttributeToAtkAttribute(JSStringRef attribute)

It's probably okay to skip the "inline" here. The compiler typically works it out and it's not imperative for performance.

> Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:47
> +    if (attrString == "AXPlaceholderValue")

attrString -> attributeString (see below).

> Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:783
> +    String atkAttributeName = coreAttributeToAtkAttribute(attribute);

Maybe an early return here if String.isEmpty()?

> Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:786
> +    for (GSList* attrs = atk_object_get_attributes(ATK_OBJECT(m_element)); attrs; attrs = attrs->next) {
> +        AtkAttribute* attr = static_cast<AtkAttribute*>(attrs->data);

Small nit: Variable names are not typically abbreviated in WebKit apart from the most obvious (such as i or it for iterator), so these should be 'attributes' and 'attribute' respectively.

[Joanmarie Diggs]

Created attachment 178435 [details]
Patch

[Joanmarie Diggs]

(In reply to comment #3)
> (From update of attachment 178434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178434&action=review
> 
> Looks good, but I have a few very small quibbles.
> 
> > Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:40
> > +static inline String coreAttributeToAtkAttribute(JSStringRef attribute)
> 
> It's probably okay to skip the "inline" here. The compiler typically works it out and it's not imperative for performance.

Done.
 
> > Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:47
> > +    if (attrString == "AXPlaceholderValue")
> 
> attrString -> attributeString (see below).

Done.

> > Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:783
> > +    String atkAttributeName = coreAttributeToAtkAttribute(attribute);
> 
> Maybe an early return here if String.isEmpty()?

Done.

> > Tools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:786
> > +    for (GSList* attrs = atk_object_get_attributes(ATK_OBJECT(m_element)); attrs; attrs = attrs->next) {
> > +        AtkAttribute* attr = static_cast<AtkAttribute*>(attrs->data);
> 
> Small nit: Variable names are not typically abbreviated in WebKit apart from the most obvious (such as i or it for iterator), so these should be 'attributes' and 'attribute' respectively.

Done with a small alteration ('attribute' is the argument, so I went with 'atkAttributes' and 'atkAttribute' respectively).

Thanks for the review!

[Martin Robinson]

Comment on attachment 178435 [details]
Patch

Great!

[WebKit Review Bot]

Comment on attachment 178435 [details]
Patch

Clearing flags on attachment: 178435

Committed r137099: <http://trac.webkit.org/changeset/137099>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.