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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1943
> +    AccessibilityObject::AccessibilityChildrenVector children =
coreObject->children();

Could this just core(hypertext->children();

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

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

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

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

size_t again.

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:102
> +static gint webkitAccessibleHyperlinkActionGetNActions(AtkAction* action)

Avoid the abbreviation here as well.

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

Should be webkitAccessibleHyperlinkGetURI

> WebCore/accessibility/gtk/WebKitAccessibleHyperlink.cpp:168
> +    // FIXME Do NOT support more than one instance of an AtkObject

Very small nit! FIXME -> FIXME:

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

> 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*>().