[webkit-reviews] review denied: [Bug 31044] [Gtk] assertion when webkit_accessible_get_index_in_parent attempts to get parent of the web view : [Attachment 42361] proposed patch

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied Joanmarie Diggs
<joanmarie.diggs at gmail.com>'s request for review:
Bug 31044: [Gtk] assertion when webkit_accessible_get_index_in_parent attempts
to get parent of the web view
https://bugs.webkit.org/show_bug.cgi?id=31044

Attachment 42361: proposed patch
https://bugs.webkit.org/attachment.cgi?id=42361&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
> +static AtkObject* webkit_accessible_get_parent(AtkObject* object)
> +{
> +    AccessibilityObject* coreParent = core(object)->parentObjectUnignored();

> +    if (!coreParent && core(object)->isWebArea())
> +	   return atkParentOfWebView(object);
> +
> +    if (!coreParent)
> +	   return NULL;

return 0;

> +
> +    return coreParent->wrapper();
> +}
> +
>  static gint webkit_accessible_get_n_children(AtkObject* object)
>  {
>      return core(object)->children().size();
> @@ -254,7 +266,16 @@ static gint
webkit_accessible_get_index_in_parent(AtkObject* object)
>      AccessibilityObject* coreObject = core(object);
>      AccessibilityObject* parent = coreObject->parentObjectUnignored();
>  
> -    g_return_val_if_fail(parent, 0);
> +    if (!parent && core(object)->isWebArea()) {
> +	   AtkObject* atkParent = atkParentOfWebView(object);
> +	   if (!atkParent)
> +	       return -1;
> +
> +	   unsigned count = atk_object_get_n_accessible_children(atkParent);
> +	   for (unsigned i = 0; i < count; ++i)
> +	       if (atk_object_ref_accessible_child(atkParent, i) == object)

This is doing a g_object_ref on each child, and then letting it go, so it's
leaking all of them basically. You need to use GOwnPtr here, or similar, so
that the references go away when you exit the function.

> +		   return i;
> +    }
>  
>      AccessibilityObject::AccessibilityChildrenVector children =
parent->children();
>      unsigned count = children.size();
> @@ -263,7 +284,7 @@ static gint
webkit_accessible_get_index_in_parent(AtkObject* object)
>	       return i;
>      }
>  
> -    return 0;
> +    return -1;
>  }
>  
>  static AtkAttributeSet* addAttributeToSet(AtkAttributeSet* attributeSet,
const char* name, const char* value)
> -- 
> 1.6.3.3
> 

r- for those two issues, but looks good to me otherwise.