[webkit-reviews] review denied: [Bug 98373] [GTK] accessibility/placeholder.html is failing : [Attachment 178434] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Joanmarie Diggs (irc: joanie)
<jdiggs at igalia.com>'s request for review:
Bug 98373: [GTK] accessibility/placeholder.html is failing
https://bugs.webkit.org/show_bug.cgi?id=98373

Attachment 178434: Patch
https://bugs.webkit.org/attachment.cgi?id=178434&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.