[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
bugzilla-daemon at webkit.org
Wed Aug 24 09:27:41 PDT 2011
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?
More information about the webkit-reviews
mailing list