[webkit-reviews] review requested: [Bug 33785] [Gtk] AtkHyperlink needs to be implemented : [Attachment 71406] 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 71406: Patch proposal + unit test
https://bugs.webkit.org/attachment.cgi?id=71406&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
Attaching a new patch addressing some of these issues. Others couldn't be
changed due to the reasons explained below...

(In reply to comment #11)
> (From update of attachment 71301 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=71301&action=review
> 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1940
> > +static AtkHyperlink* webkitAccessibleHypertextGetLink(AtkHypertext*
hypertext, gint index)
> 
> I think you can change the index parameter type to size_t and avoid the cast
below.

As discussed by IRC, I can't change that since it must be compliant with the
signature of the atk_hypertext_get_link() method, from the ATK interface. So
the cast must stay as well :-(
 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1943
> > +	 AccessibilityObject::AccessibilityChildrenVector children =
coreObject->children();
> 
> Could this just core(hypertext->children();

Sure! Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1966
> > +static gint webkitAccessibleHypertextGetNLinks(AtkHypertext* hypertext)
> 
> Avoid abbreviations here. webkitAccessibleHypertextGetLinkCount or
webkitAccessibleHypertextGetNumberOfLinks. The method could also return a
size_t.

As discussed in IRC, in this case it's better to leave it that way so (1) it
matches better the method it's implementing from the ATK interface and (2)
because it's the way it's done all around in the ATK wrapper, so changing that
in this case would lead to confusion in further readings of the code.
Hence, left as it was in the new patch.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1972
> > +	 for (unsigned i = 0; i < children.size(); i++) {
> 
> I think you should use size_t here instead of unsigned.

Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1985
> > +	 gint nLinks = webkitAccessibleHypertextGetNLinks(hypertext);
> > +	 if (!nLinks)
> > +	     return -1;
> 
> Should be numberOfLinks or perhaps linksCount. Avoid abbreviations. The type
should also probably be size_t.

Agreed to both points. Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1987
> > +	 for (gint i = 0; i < nLinks; i++) {
> 
> size_t again.

Done.

> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:102
> > +static gint webkitAccessibleHyperlinkActionGetNActions(AtkAction* action)
> 
> Avoid the abbreviation here as well.

Can't do it, same reasons as explained above (match method from ATK interface)

> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:165
> > +static gchar* webkitAccessibleHyperlinkGetUri(AtkHyperlink* link, gint
index)
> 
> Should be webkitAccessibleHyperlinkGetURI

Can't do it, same reasons as explained above (match method from ATK interface)
 
> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:168
> > +	 // FIXME Do NOT support more than one instance of an AtkObject
> 
> Very small nit! FIXME -> FIXME:

Done.
 
> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:303
> > +GType
> > +webkitAccessibleHyperlinkGetType(void)
> 
> Don't use a newline after GType here, if it can be avoided remove the void.

Opps! I missed that one, sorry. Fixed.

> > WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:342
> > +	 g_hash_table_remove(hashTable, (void*)linkImpl);
> 
> I could be wrong, but I don't think you need to cast to void*. If you do, you
should use a static_cast<void*>().

Actually you're right and I'm wrong :-). Fixed

Thanks for the review!