[Webkit-unassigned] [Bug 33785] [Gtk] AtkHyperlink needs to be implemented

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33785


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71406|review?                     |review-
               Flag|                            |




--- Comment #13 from Martin Robinson <mrobinson at webkit.org>  2010-10-25 10:50:32 PST ---
(From update of attachment 71406)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.