[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