[webkit-reviews] review denied: [Bug 65900] Correctly report selected text range for accessibility APIs for role=textbox : [Attachment 104966] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alice Boxhall
<aboxhall at chromium.org>'s request for review:
Bug 65900: Correctly report selected text range for accessibility APIs for
role=textbox
https://bugs.webkit.org/show_bug.cgi?id=65900

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=104966&action=review


review- because of the missing null check. There are other smaller mistakes.

> LayoutTests/accessibility/textbox-role-reports-selection-expected.txt:8
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS

It’s much better to have tests that log some data rather than just the word
PASS. See the style of the shouldBe macros, which log what is being tested, not
just "pass". Something to keep in mind when working on a new test.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2538
> +const Element*
AccessibilityRenderObject::rootEditableElementForPosition(const Position&
position) const

The use of const here in const Element* doesn’t make much sense to me. Why is
the element returned a const*? Does the caller not have permission to make
changes to that element?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2545
> +    for (const Node* n = position.containerNode(); n && n !=
rootEditableElement; n = n->parentNode()) {

It would be better to write this loop using parentElement. That way we could
achieve type safety without at typecast.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2563
> +    const AccessibilityObject* axObjectForNode =
axObjectCache()->getOrCreate(node->renderer());

What if node->renderer() is 0?