[webkit-reviews] review denied: [Bug 25679] [GTK] Improve accessibility of focusable lists : [Attachment 72945] Part 3 - emit 'selected' and 'focused' signals

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 25679: [GTK] Improve accessibility of focusable lists
https://bugs.webkit.org/show_bug.cgi?id=25679

Attachment 72945: Part 3 - emit 'selected' and 'focused' signals
https://bugs.webkit.org/attachment.cgi?id=72945&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72945&action=review

> WebCore/ChangeLog:14
> +	   'focus-event', along with apropriate detail along with each of

spelling -> apropriate

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:55
> +    AccessibilityRenderObject::AccessibilityChildrenVector items =
object->children();

this should be AccessibilityObject::AXChildrenVector

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:56
> +    SelectElement* select =
toSelectElement(static_cast<Element*>(object->node()));

you should check if select == 0 after this line

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:58
> +    if (changedItemIndex < 0 || changedItemIndex >= (int)items.size())

use static cast here

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:65
> +    static RefPtr<AccessibilityObject> oldFocusedObject;

this static should be at the top of the method. i also believe there's a
standard way to initialize statics, that you should look for

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:83
> +	   g_signal_emit_by_name(axItem, "focus-event", isSelected);

do you want to emit focus-event every time this happens? or do you want to
limit it to when axItem != oldFocusedItem