[webkit-reviews] review granted: [Bug 67230] [mac] Add text search API for getting the DOM range of a text match : [Attachment 105701] Add -[WebView DOMRangeOfString:relativeTo:options:]

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted mitz at webkit.org's request for review:
Bug 67230: [mac] Add text search API for getting the DOM range of a text match
https://bugs.webkit.org/show_bug.cgi?id=67230

Attachment 105701: Add -[WebView DOMRangeOfString:relativeTo:options:]
https://bugs.webkit.org/attachment.cgi?id=105701&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105701&action=review


> Source/WebCore/editing/Editor.cpp:2928
> +PassRefPtr<Range> Editor::findString(const String& target, Range*
referenceRange, FindOptions options)

I think it’s confusing that both functions are named findString, but one
modifies the selection and the other does not. I would prefer that the two
functions have different names rather than be differentiated only by
overloading.

> Source/WebCore/editing/Editor.cpp:2945
> +	       searchRange->setStart(startInReferenceRange ?
referenceRange->startPosition() : referenceRange->endPosition(), ec);

You can just call setStart without passing (ec) now. It will assert if the
range is detached. It’s the new hotness!

> Source/WebCore/editing/Editor.cpp:2947
> +	       searchRange->setEnd(startInReferenceRange ?
referenceRange->endPosition() : referenceRange->startPosition(), ec);

You can just call setEnd without passing (ec) now. It will assert if the range
is detached. It’s the new hotness!

> Source/WebCore/editing/Editor.cpp:2965
> +	       searchRange->setStart(referenceRange->endPosition(), ec);

Ditto.

> Source/WebCore/editing/Editor.cpp:2967
> +	       searchRange->setEnd(referenceRange->startPosition(), ec);

Ditto.

> Source/WebCore/editing/Editor.cpp:2980
> +    if (resultRange->collapsed(ec) && shadowTreeRoot) {

You can just call collapsed without passing (ec) now. It will assert if the
range is detached. It’s the new hotness!

> Source/WebCore/editing/Editor.cpp:2985
> -	       searchRange->setStartAfter(shadowTreeRoot->shadowAncestorNode(),
exception);
> +	       searchRange->setStartAfter(shadowTreeRoot->shadowAncestorNode(),
ec);
>	   else
> -	       searchRange->setEndBefore(shadowTreeRoot->shadowAncestorNode(),
exception);
> +	       searchRange->setEndBefore(shadowTreeRoot->shadowAncestorNode(),
ec);

If you wanted you could implement the ASSERT_NO_EXCEPTION thing for
setStartAfter and setEndBefore too. While it would require modifying Range.h it
would be forward looking and cool.

> Source/WebCore/editing/Editor.cpp:2998
> +    if (resultRange->collapsed(ec) && options & WrapAround) {

Ditto.

> Source/WebCore/editing/Editor.cpp:3006
> +    return resultRange->collapsed(ec) ? 0 : resultRange.release();

Same here.

> Source/WebCore/page/Page.cpp:521
> +    // We cheat a bit and just research with wrap on

Probably need to spell it re-search so it’s not confused with the word
research.

Sentence needs a period.