[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 #9 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 299261
  --> https://bugs.webkit.org/attachment.cgi?id=299261
patch

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

> Source/WebCore/dom/Range.cpp:1269
> -void Range::collectSelectionRects(Vector<SelectionRect>& rects)
> +void Range::collectSelectionRectsWithoutUnionInteriorLines(Vector<SelectionRect>& rects, int& maxLineNumber)

Why isn't this function simply returning the maxLineNumber?

> Source/WebCore/dom/Range.cpp:1432
> +    int maxLineNumber;
> +    collectSelectionRectsWithoutUnionInteriorLines(rects, maxLineNumber);
> +    const size_t numberOfRects = rects.size();
> +    

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.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1819
> +    // Hit test the position make sure it's visible.

*to* make sure.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1822
> +    FloatRect rect = result.absoluteCaretBounds();
> +    IntPoint center = IntPoint(rect.center());
> +    result = frame.visiblePositionForPoint(center);

Are you sure we really need to do this? This is a very funky operation.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1839
> +    WebCore::TextGranularity webGranularity = static_cast<WebCore::TextGranularity>(granularity);

"web" prefix is usually used for WK/WebView types, not WebCore types.

> 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;

Why can't we just use findPlainText instead? That'd just return a range that includes the text inside a given range.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1940
> +    if (!resultRange || resultRange->collapsed())
> +        resultRange = range;

Why do we want to do this?

-- 
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/20170125/01e53050/attachment.html>