[webkit-reviews] review granted: [Bug 44778] AX: Support AccessibilityTextMarkers in DRT : [Attachment 65926] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 7 14:20:55 PDT 2010


David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 44778: AX: Support AccessibilityTextMarkers in DRT
https://bugs.webkit.org/show_bug.cgi?id=44778

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

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=65926&action=prettypatch

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2527
> +    // Used only by DumpRenderTree (so far)
Nit: Need period at the end of the sentence.

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2528
> +    if ([attribute isEqualToString: @"AXStartTextMarkerForTextMarkerRange"])
{
Nit: No space after "isEqualToString:" and its argument.  (I guess this is the
prevailing style in this file, but it should eventually be fixed.)

> WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2533
> +    if ([attribute isEqualToString: @"AXEndTextMarkerForTextMarkerRange"]) {

Ditto.

> WebKitTools/DumpRenderTree/AccessibilityTextMarker.cpp:49
> +    return JSValueMakeBoolean(context,
toTextMarker(thisObject)->isEqual(toTextMarker(otherMarker)));
This method might read better as:

{
    if (argumentCount != 1)
	return JSValueMakeBoolean(context, false);

    JSObjectRef otherMarker = JSValueToObject(context, arguments[0],
exception);
    return JSValueMakeBoolean(context,
toTextMarker(thisObject)->isEqual(toTextMarker(otherMarker)));
}

> WebKitTools/DumpRenderTree/AccessibilityTextMarker.cpp:103
> +    return JSValueMakeBoolean(context,
toTextMarkerRange(thisObject)->isEqual(toTextMarkerRange(otherMarker)));
Same comment as isMarkerEqualCallback().

> WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp:727
> +    return 0;
Would it be clearer to say this instead:

    return AccessibilityTextMarkerRange(0);

Maybe the implicit constructor is okay, though.

> WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp:750
> +AccessibilityUIElement
AccessibilityUIElement::accessibilityElementForTextMarker(AccessibilityTextMark
er*)
Should this return a pointer instead of an object?  I guess there is at least
one other method that returns a AccessibilityUIElement object.

> WebKitTools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:690
> +			developmentRegion = English;
This change should not be checked in.

> WebKitTools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:692
> +			knownRegions = (
This change should not be checked in.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:28
> +#import "AccessibilityTextMarker.h"
Nit: Alphabetize the #Import statements here.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:32
> +#if SUPPORTS_AX_TEXTMARKERS
Not sure if this is needed since it's always defined as 1 for PLATFORM(MAC),
but it's fine as-is.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:38
> +	   CFRetain(m_textMarker);
It would be nice to use a RetainPtr<>() in AccessibilityTextMarker instead of
having to manually retain/release the object in the implementation.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:45
> +	   CFRetain(m_textMarker);
Ditto.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:51
> +	   CFRelease(m_textMarker);
Ditto.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:66
> +	   CFRetain(m_textMarkerRange);
It would be nice to use a RetainPtr<>() in AccessibilityTextMarkerRange instead
of having to manually retain/release the object in the implementation.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:73
> +	   CFRetain(m_textMarkerRange);
Ditto.

> WebKitTools/DumpRenderTree/mac/AccessibilityTextMarkerMac.mm:79
> +	   CFRelease(m_textMarkerRange);
Ditto.

> LayoutTests/platform/mac/accessibility/element-for-text-marker.html:13
> +<div id="text1" tabindex=0>text block</div>
Nit:  tabindex="0"

> LayoutTests/platform/mac/accessibility/element-for-text-marker.html:21
> +    description("This tests the text marker system will return the right
element when given a text marker");
Nit:  Change "right element" to "correct element".
Nit:  Add a period to the end of the sentence.

> LayoutTests/platform/mac/accessibility/text-marker-length.html:12
> +<div id="text" tabindex=0>text block</div>
Nit:  tabindex="0"

r=me but consider the comments above.


More information about the webkit-reviews mailing list