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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(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...