[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 #71301|0                           |1
        is obsolete|                            |
  Attachment #71406|                            |review?
               Flag|                            |




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

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!

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