[Webkit-unassigned] [Bug 166955] AX: WKContentView needs to implement UITextInput methods to make speak selection highlighting work

[bugzilla-daemon at webkit.org]

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

--- Comment #25 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 299952
  --> https://bugs.webkit.org/attachment.cgi?id=299952
patch

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

Wow, the code looks much nicer! & great test cases!
Please address the comments below and we're good to go!

> Source/WebCore/editing/TextIterator.cpp:2659
> -static std::optional<std::pair<size_t, size_t>> findPlainTextOffset(SearchBuffer& buffer, CharacterIterator& findIterator, bool searchForward)
> +static std::optional<std::pair<size_t, size_t>> findPlainTextOffset(SearchBuffer& buffer, CharacterIterator& findIterator, const std::function<bool(size_t&, size_t&, size_t, size_t)>& found)

It's cleaner if this function just returned void and instead each lambda kept matchStart, matchEnd themselves.
We should probably rename this function to findPlainTextMatches() to signify the fact we just calls the callback.

Once we did that, this function's lambda can just be std::function<bool(size_t, size_t)>.

> Source/WebCore/editing/TextIterator.cpp:2677
> -            if (searchForward) // Look for the last match when searching backwards instead.
> +            if (found(matchStart, matchLength, lastCharacterInBufferOffset - matchStartOffset, newMatchLength))

Once we made the change above, we should just call this function callback.
Basically, for backwards searching and your function, this function will always return false.
For forward searching, it always returns true (since the first match should be returned).

> Source/WebCore/editing/TextIterator.cpp:2688
> -Ref<Range> findPlainText(const Range& range, const String& target, FindOptions options)
> +static Ref<Range> findPlainTextWithFunc(const Range& range, const String& target, FindOptions options, const std::function<bool(size_t&, size_t&, size_t, size_t)>& func)

Please don't use abbreviations like "func".
I'd call this function findPlainTextMatchesInRange or something instead of suffixing it with the last argument's type.

> Source/WebCore/editing/TextIterator.cpp:2720
> +        size_t newDistance = std::min(abs((signed)(start - targetOffset)), abs((signed)(start + length - targetOffset)));

Please don't use C-style casing like (signed).

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1829
> +static VisiblePosition visiblePositionForPositionWithOffset(const VisiblePosition& position, int32_t offset)
> +{
> +    SelectionDirection direction = offset < 0 ? DirectionBackward : DirectionForward;
> +    VisiblePosition result = position;
> +    result.setAffinity(DOWNSTREAM);
> +    for (int i = 0; i < abs(offset); ++i) {
> +        result = direction == DirectionForward ? result.next() : result.previous();
> +        if (result.isNull())
> +            break;
> +    }
> +    return result;
> +}

Please use visiblePositionForIndex(root, indexForVisiblePosition(position, root) + offset) instead
where root is commonAncestorContainer() of the range or its rootEditableElement().

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1830
> +    

There's a whitespace here.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1835
> +    

All these functions have whitespace in blank lines. Please remove them all.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1841
> +    VisiblePosition position = visiblePositionForPositionWithOffset(selectionStart, offset);

Use auto.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1844
> +    RefPtr<Range> range = enclosingTextUnitOfGranularity(position, static_cast<WebCore::TextGranularity>(granularity), direction);

Use auto.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1869
> +    RefPtr<Range> selectionRange = frame.selection().selection().toNormalizedRange();

Normalizing Range is expensive. Why don't we call it in inside the if (rangeText != text) below?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1878
> +    VisiblePosition startPosition = visiblePositionForPositionWithOffset(selectionStart, offset);
> +    VisiblePosition endPosition = visiblePositionForPositionWithOffset(startPosition, length);
> +    RefPtr<Range> range = Range::create(*frame.document(), startPosition, endPosition);

Use auto.

> LayoutTests/editing/text-iterator/range-of-string-closest-to-position.html:27
> +    

Please remove these whitespace in blank lines.

-- 
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/20170128/41073ecc/attachment.html>