Bug 98373 - [GTK] accessibility/placeholder.html is failing
: [GTK] accessibility/placeholder.html is failing
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Joanmarie Diggs (irc: joanie)
: Gtk, LayoutTestFailure
Depends on:
Blocks: 98347
  Show dependency treegraph
 
Reported: 2012-10-04 00:43 PDT by Zan Dobersek
Modified: 2012-12-09 17:10 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.10 KB, patch)
2012-12-09 12:21 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (6.25 KB, patch)
2012-12-09 12:43 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2012-10-04 00:43:24 PDT
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
Comment 1 Joanmarie Diggs (irc: joanie) 2012-10-16 12:51:26 PDT
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.
Comment 2 Joanmarie Diggs (irc: joanie) 2012-12-09 12:21:50 PST
Created attachment 178434 [details]
Patch
Comment 3 Martin Robinson 2012-12-09 12:30:53 PST
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.
Comment 4 Joanmarie Diggs (irc: joanie) 2012-12-09 12:43:22 PST
Created attachment 178435 [details]
Patch
Comment 5 Joanmarie Diggs (irc: joanie) 2012-12-09 12:45:27 PST
(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!
Comment 6 Martin Robinson 2012-12-09 12:47:46 PST
Comment on attachment 178435 [details]
Patch

Great!
Comment 7 WebKit Review Bot 2012-12-09 17:10:31 PST
Comment on attachment 178435 [details]
Patch

Clearing flags on attachment: 178435

Committed r137099: <http://trac.webkit.org/changeset/137099>
Comment 8 WebKit Review Bot 2012-12-09 17:10:35 PST
All reviewed patches have been landed.  Closing bug.