<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: WKContentView needs to implement UITextInput methods to make speak selection highlighting work"
href="https://bugs.webkit.org/show_bug.cgi?id=166955#c25">Comment # 25</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: WKContentView needs to implement UITextInput methods to make speak selection highlighting work"
href="https://bugs.webkit.org/show_bug.cgi?id=166955">bug 166955</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=299952&action=diff" name="attach_299952" title="patch">attachment 299952</a> <a href="attachment.cgi?id=299952&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=299952&action=review">https://bugs.webkit.org/attachment.cgi?id=299952&action=review</a>
Wow, the code looks much nicer! & great test cases!
Please address the comments below and we're good to go!
<span class="quote">> 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)</span >
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)>.
<span class="quote">> Source/WebCore/editing/TextIterator.cpp:2677
> - if (searchForward) // Look for the last match when searching backwards instead.
> + if (found(matchStart, matchLength, lastCharacterInBufferOffset - matchStartOffset, newMatchLength))</span >
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).
<span class="quote">> 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)</span >
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.
<span class="quote">> Source/WebCore/editing/TextIterator.cpp:2720
> + size_t newDistance = std::min(abs((signed)(start - targetOffset)), abs((signed)(start + length - targetOffset)));</span >
Please don't use C-style casing like (signed).
<span class="quote">> 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;
> +}</span >
Please use visiblePositionForIndex(root, indexForVisiblePosition(position, root) + offset) instead
where root is commonAncestorContainer() of the range or its rootEditableElement().
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1830
> + </span >
There's a whitespace here.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1835
> + </span >
All these functions have whitespace in blank lines. Please remove them all.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1841
> + VisiblePosition position = visiblePositionForPositionWithOffset(selectionStart, offset);</span >
Use auto.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1844
> + RefPtr<Range> range = enclosingTextUnitOfGranularity(position, static_cast<WebCore::TextGranularity>(granularity), direction);</span >
Use auto.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1869
> + RefPtr<Range> selectionRange = frame.selection().selection().toNormalizedRange();</span >
Normalizing Range is expensive. Why don't we call it in inside the if (rangeText != text) below?
<span class="quote">> 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);</span >
Use auto.
<span class="quote">> LayoutTests/editing/text-iterator/range-of-string-closest-to-position.html:27
> + </span >
Please remove these whitespace in blank lines.</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>