Bug 67230 – [mac] Add text search API for getting the DOM range of a text match

• Component: WebKit API
• Status: RESOLVED FIXED

Add text search API for getting the DOM range of a text match

<rdar://problem/9281695>

Created attachment 105701 [details]
Add -[WebView DOMRangeOfString:relativeTo:options:]

[WebKit Review Bot]

Attachment 105701 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/mac/WebView/WebViewInternal.h:60:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 105701 [details]
Add -[WebView DOMRangeOfString:relativeTo:options:]

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.

Comment on attachment 105701 [details]
Add -[WebView DOMRangeOfString:relativeTo:options:]

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

Thanks for the review! I am going to switch to the ExceptionCode-less variants and if there are no complications even add new ones.

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

I’m going to use a different name here and in the Page class.

Created attachment 105718 [details]
Add -[WebView DOMRangeOfString:relativeTo:options:]

[Darin Adler]

Comment on attachment 105718 [details]
Add -[WebView DOMRangeOfString:relativeTo:options:]

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

> Source/WebCore/dom/Range.h:107
> -    void setStart(const Position&, ExceptionCode&);
> -    void setEnd(const Position&, ExceptionCode&);
> +    void setStart(const Position&, ExceptionCode& = ASSERT_NO_EXCEPTION);
> +    void setEnd(const Position&, ExceptionCode& = ASSERT_NO_EXCEPTION);

I think that these should not even take ExceptionCode& since they are internal functions, not DOM API. ExceptionCode& is ugly for internal use, except where it's intended to be used for helper functions that in turn are used implement DOM API functions. ASSERT_NO_EXCEPTION is OK for now, but later we should just get rid of that.

[WebKit Review Bot]

Attachment 105718 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/mac/WebView/WebViewInternal.h:60:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Added in <http://trac.webkit.org/r94124>.