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

[bugzilla-daemon at webkit.org]

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


Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71406|0                           |1
        is obsolete|                            |
  Attachment #72159|                            |review?
               Flag|                            |




--- Comment #14 from Mario Sanchez Prada <msanchez at igalia.com>  2010-10-28 01:32:39 PST ---
Created an attachment (id=72159)
 --> (https://bugs.webkit.org/attachment.cgi?id=72159&action=review)
Patch proposal + unit test

(In reply to comment #13)
> (From update of attachment 71406 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71406&action=review
> 
> Looks great, but r- pending further investigation and some small cleanup.

Attaching a new patch now, and I honestly think it's way better thank to your feedback. Thanks!

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

Fixed.

> > 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.

True, and I finally implemented it as it's way better than using this global hashtable:

It basically consists of just having a property in the WebKitAccessible instance implementing the AtkHyperlinkImpl interface to keep a reference to the WebKitAccessibleHyperlink instance, so we can just create and assign that property the first time it's required to create the AtkHyperlink, and reuse it later. Also, from the side of the WebKitAccessibleHyperlink instance, a weak reference is kept to the WebKitAccessible instance, so we can safely unref that AtkHyperlink when the AtkObject is destroyed too.

It's cleaner, safer and IMHO better, and it works like a charm. Thanks a lot for the suggestion

> > 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.

Ooops! Fixed.

> > 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.

Got it. Fixed.

> > 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?

No, we actually don't need it, just spinning the main loop would be enough. Fixed (in fashion with the new trend :-))

Asking for review again then...

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