<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#c9">Comment # 9</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=299261&action=diff" name="attach_299261" title="patch">attachment 299261</a> <a href="attachment.cgi?id=299261&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=299261&action=review">https://bugs.webkit.org/attachment.cgi?id=299261&action=review</a>
<span class="quote">> Source/WebCore/dom/Range.cpp:1269
> -void Range::collectSelectionRects(Vector<SelectionRect>& rects)
> +void Range::collectSelectionRectsWithoutUnionInteriorLines(Vector<SelectionRect>& rects, int& maxLineNumber)</span >
Why isn't this function simply returning the maxLineNumber?
<span class="quote">> Source/WebCore/dom/Range.cpp:1432
> + int maxLineNumber;
> + collectSelectionRectsWithoutUnionInteriorLines(rects, maxLineNumber);
> + const size_t numberOfRects = rects.size();
> + </span >
A pattern like this where we rely on a function being called to set a value to maxLineNumber is dangerous.
If someone adds an early return in collectSelectionRectsWithoutUnionInteriorLines, for example,
we may end up never initializing maxLineNumber.
It's better if collectSelectionRectsWithoutUnionInteriorLines returned this value
so that maxLineNumber is always assigned some value, and the code leaves no ambiguity in that regard.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1819
> + // Hit test the position make sure it's visible.</span >
*to* make sure.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1822
> + FloatRect rect = result.absoluteCaretBounds();
> + IntPoint center = IntPoint(rect.center());
> + result = frame.visiblePositionForPoint(center);</span >
Are you sure we really need to do this? This is a very funky operation.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1839
> + WebCore::TextGranularity webGranularity = static_cast<WebCore::TextGranularity>(granularity);</span >
"web" prefix is usually used for WK/WebView types, not WebCore types.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1925
> + // Try to search for a word range within the selection range that matches the passed in text.
> + if (RefPtr<Range> wordRange = wordRangeForPositionMatchesText(startPosition, range, text, selectionRange))
> + range = wordRange;</span >
Why can't we just use findPlainText instead? That'd just return a range that includes the text inside a given range.
<span class="quote">> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1940
> + if (!resultRange || resultRange->collapsed())
> + resultRange = range;</span >
Why do we want to do this?</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>