<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: [Mac] Implement next/previous text marker functions using TextIterator"
href="https://bugs.webkit.org/show_bug.cgi?id=152728#c9">Comment # 9</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: [Mac] Implement next/previous text marker functions using TextIterator"
href="https://bugs.webkit.org/show_bug.cgi?id=152728">bug 152728</a>
from <span class="vcard"><a class="email" href="mailto:cfleizach@apple.com" title="chris fleizach <cfleizach@apple.com>"> <span class="fn">chris fleizach</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=268922&action=diff" name="attach_268922" title="patch">attachment 268922</a> <a href="attachment.cgi?id=268922&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=268922&action=review">https://bugs.webkit.org/attachment.cgi?id=268922&action=review</a>
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1457
> + for (; !iterator.atEnd(); iterator.advance()) {</span >
seems like int count = 1 and count++ can be part of the for loop
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1466
> + if (AccessibilityObject::replacedNodeNeedsCharacter(node.traverseToChildAt(subOffset))) {</span >
you can probably cache node.traverseToChildAt(subOffset) in a local variable so you don't have to call it twice
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1479
> + if (!currentLength)</span >
this check could be put up as the else for the if block at 1466 right?
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1562
> + // endOffset can be out of bound sometimes if the node is a replaced node or has brTag.</span >
out of "bounds"
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1565
> + if (endOffset > TextIterator::rangeLength(nodeRange.get()))</span >
you can store TextIterator::rangeLength(nodeRange.get()) so you don't have to call it twice
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.h:55
> + int characterOffset;</span >
what is startIndex and what is ignored used for?
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:805
> + return nil;</span >
nullptr since it's a C++ object
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:847
> + if (isTextMarkerIgnored(textMarker))</span >
doesn't this need to be in a while loop
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:856
> + if (isTextMarkerIgnored(textMarker))</span >
doesn't this need to be in a while loop
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:877
> + if (!textMarkerData.axID && !textMarkerData.ignored)</span >
can we use axID = 0 to mean ignored?
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:886
> + return nil;</span >
nullptr since its C++
<span class="quote">> LayoutTests/accessibility/mac/previous-next-text-marker.html:117
> + shouldBeTrue("!psw.accessibilityElementForTextMarker(start)");</span >
shouldBeFalse</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>