[Webkit-unassigned] [Bug 152728] AX: [Mac] Implement next/previous text marker functions using TextIterator

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=152728

--- Comment #3 from chris fleizach <cfleizach at apple.com> ---
Comment on attachment 268269
  --> https://bugs.webkit.org/attachment.cgi?id=268269
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268269&action=review

> Source/WebCore/ChangeLog:8
> +        Implemented a way to navigate to previous/next text marker using Range and TextIterator.

can you add a few sentences as to why you're doing this

> Source/WebCore/accessibility/AXObjectCache.cpp:1431
> +    Node* currNode = nullptr;

currNode -> currentNode

> Source/WebCore/accessibility/AXObjectCache.cpp:1448
> +                if (preNode)

if (Node* preNode = previousNode(currNode))

> Source/WebCore/accessibility/AXObjectCache.cpp:1457
> +        int currLen = iterator.text().length();

currLen = currentLength

> Source/WebCore/accessibility/AXObjectCache.cpp:1469
> +            

extra newline can be removed

> Source/WebCore/accessibility/AXObjectCache.cpp:1509
> +    RefPtr<Range> range =  Range::create(*document);

too many spaces 

"= Range"

> Source/WebCore/accessibility/AXObjectCache.cpp:1512
> +    if (ec)

return ec ? nullptr : range;

> Source/WebCore/accessibility/AXObjectCache.cpp:1626
> +    if (node->parentNode() && node->parentNode()->renderer() &&  node->parentNode()->renderer()->isBody() && !node->previousSibling())

too many spaces between the &&
"node->parentNode()->renderer() &&  node->parentNode()->renderer()->isBody()"

> Source/WebCore/accessibility/AXObjectCache.h:65
> +    CharacterOffset(Node* n, int offset, int remaining)

you might be able to put default values for these parameters and then remove the default constructor
CharacterOffset() { }

--> 
CharacterOffset(Node* n = nullptr, int offset = 0, int remaining = 0)

> Source/WebCore/accessibility/AccessibilityObject.cpp:724
> +    return AXObjectCache::rangeForNodeContents(node);

seems like you could probably just do

return AXObjectCache::rangeForNodeContents(node());

and then handle the nil case inside rangeForNodeContents

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:790
> +static AccessibilityObject* accessibilityObjectForTextMarker(AXObjectCache* cache, CFTypeRef textMarker)

these methods might be better off inside WebAXObjectWrapperBase so that they can be used by iOS and OSX

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:830
> +- (id)textMarkerForNode:(Node&)node Offset:(int)offset

Offset -> offset

> LayoutTests/ChangeLog:7
> +

we should add a test for navigation within a user-select:none area

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160105/f02c263f/attachment.html>