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