[webkit-reviews] review denied: [Bug 121684] [ATK] Expose aria-describedby with ATK_RELATION_DESCRIBED_BY : [Attachment 221655] patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has denied Krzysztof Czech
<k.czech at samsung.com>'s request for review:
Bug 121684: [ATK] Expose aria-describedby with ATK_RELATION_DESCRIBED_BY
https://bugs.webkit.org/show_bug.cgi?id=121684

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

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=221655&action=review


I think the patch looks good already, but I think there are some things that
would need some changes before landing.

> LayoutTests/ChangeLog:8
> +	   Slightly modified test so that it could test aria-describedby with
multiple id references.

What about "extending" the test VS modifying it? I mean, keeping the "one ID"
only case and adding the new one with two IDs on top of it.

> LayoutTests/accessibility/aria-describedby-on-input-expected.txt:3
> -Allows you to specify the number of minutes after which the computer will
self-destruct.
> +Allows you to specify the number of minutes after
> +which the computer will self-destruct.

Why is this line break showing up here? I would expect the two strings to be
concatenated together with a " " in the middle.

I looked into the implementation in DRT/WKTR, but still is not obvious to me...


> Source/WebCore/accessibility/AccessibilityNodeObject.h:185
> +    virtual void elementsFromAttribute(Vector<Element*>& elements, const
QualifiedName&) const override;

I think we normally write OVERRIDE (uppercase letters). Also, why do you need
to make this method virtual if the implementation in the parent is going to be
an empty one?

I see the point of being able to call it from a AccessibilityObject, but I'm
not sure that alone justifies doing this, unless it made sense to move the
whole method (implementation too) up to AccessibilityObject

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:248
> +		   AccessibilityObject* axObject =
coreObject->document()->axObjectCache()->getOrCreate(element);

The paranoid in me would add a null check for the document, but I don't think
it's possible that you ever reach a null doc if you got this far (so I'm ok)

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:819
> +	   if (atk_relation_get_relation_type(relation) ==
ATK_RELATION_DESCRIBED_BY) {

Nit. I would do if (atk_relation_get_relation_type(relation) !=
ATK_RELATION_DESCRIBED_BY) continue; here, to avoid nesting too much below

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:820
> +	       GPtrArray* targetList = atk_relation_get_target(relation);

Not sure if this targetList can ever be NULL. Looking at AtkRelationSet code
directly suggests that the internal pointer relation->target (returned by
get_target()), could be NULL, although it's not clear whether that will ever
happen in a real-life scenario.

In any case, I would add a null check here, maybe with an "early continue" as
well ("if (!targetList) continue;")