[webkit-reviews] review denied: [Bug 33785] [Gtk] AtkHyperlink needs to be implemented : [Attachment 71406] Patch proposal + unit test

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 33785: [Gtk] AtkHyperlink needs to be implemented
https://bugs.webkit.org/show_bug.cgi?id=33785

Attachment 71406: Patch proposal + unit test
https://bugs.webkit.org/attachment.cgi?id=71406&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71406&action=review

Looks great, but r- pending further investigation and some small cleanup.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1969
> +    if (!children.size())
> +	 return 0;

The indentation looks wrong here.

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:44
> +// Use this hash table to help reusing instances of the
> +// WebKitAccessibleHyperlink class throughout the
> +// webkitAccessibleHyperlinkGetOrCreate method
> +static GHashTable* hashTable = 0;

I offered a suggestion via Jabber about how this might be avoided.

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:151
> +    if (!ATK_IS_ACTION(WEBKIT_ACCESSIBLE_HYPERLINK(action)->linkImpl))
> +	   return FALSE;
> +
> +    AccessibilityObject* coreObject = core(action);
> +    if (!coreObject)
> +	   return FALSE;

The return type of this method is gchar*. I don't think FALSE is correct here.

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:194
> +    gint baseLength = TextIterator::rangeLength(range);

I think when not exposed to the API, we prefer to use C types. So int.

> WebKit/gtk/tests/testatk.c:979
> +    g_idle_add((GSourceFunc)bail_out, loop);
> +    g_main_loop_run(loop);

Do we need this extra main loop or can we just spin the main main loop?