Bug 98373 - [GTK] accessibility/placeholder.html is failing
: [GTK] accessibility/placeholder.html is failing
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: Gtk, LayoutTestFailure
:
: 98347
  Show dependency treegraph
 
Reported: 2012-10-04 00:43 PST by
Modified: 2012-12-09 17:10 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2012-10-16 12:51:26 PST -------
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 From 2012-12-09 12:21:50 PST -------
Created an attachment (id=178434) [details]
Patch
------- Comment #3 From 2012-12-09 12:30:53 PST -------
(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.

> 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 From 2012-12-09 12:43:22 PST -------
Created an attachment (id=178435) [details]
Patch
------- Comment #5 From 2012-12-09 12:45:27 PST -------
(In reply to comment #3)
> (From update of attachment 178434 [details] [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 From 2012-12-09 12:47:46 PST -------
(From update of attachment 178435 [details])
Great!
------- Comment #7 From 2012-12-09 17:10:31 PST -------
(From update of attachment 178435 [details])
Clearing flags on attachment: 178435

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