[webkit-reviews] review granted: [Bug 214261] Remove live ranges from Editor.h and EditorClient.h : [Attachment 404691] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Adler <darin at apple.com>'s request
for review:
Bug 214261: Remove live ranges from Editor.h and EditorClient.h
https://bugs.webkit.org/show_bug.cgi?id=214261

Attachment 404691: Patch

https://bugs.webkit.org/attachment.cgi?id=404691&action=review




--- Comment #13 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 404691
  --> https://bugs.webkit.org/attachment.cgi?id=404691
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:309
> +	       if
(createLiveRange(*misspellingRange)->compareBoundaryPoints(Range::END_TO_END,
createLiveRange(start)).releaseReturnValue() > 0)

It kills me that we are creating these temporary live ranges just to do
compareBoundaryPoints, but I know the plan is to come back and remove these
eventually, so I will take it.

> Source/WebCore/dom/Range.cpp:1836
> -SimpleRange makeSimpleRange(const Range& range)
> +SimpleRange makeRange(const Range& range)

Not sure how I feel about this. It very much is not making a Range.

What is the impetus behind this rename? Is the ultimate goal to rename
SimpleRange to Range, and the existing Range to LiveRange?

> Source/WebCore/editing/Editor.cpp:2610
> +void Editor::markMisspellingsOrBadGrammar(const VisibleSelection& selection,
bool checkSpelling, Optional<SimpleRange>& firstMisspellingRange)

firstMisspellingRange should be a returned value not a out reference. (not new,
but might be nice to fix since you are touching it). Same is true for a few
more functions below.

> Source/WebCore/editing/Editor.cpp:3495
> +template<typename T> static auto& start(T&& range, FindOptions options)
> +{
> +    return options.contains(Backwards) ? range.end : range.start;
> +}

I think you want these to take a T& range, because you would never want this
function to take ownership of range given it is returning references to range's
members.

> Source/WebCore/editing/Editor.h:449
> +    WEBCORE_EXPORT Optional<SimpleRange> rangeOfString(const String&, const
Optional<SimpleRange>&, FindOptions);

Given that it's not immediately clear what the OptionalRange parameter is for,
I might consider adding a parameter name here to clarify.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1386
> +	   if (position < startPosition)
> +	       position = startPosition;
> +	   if (position > endPosition)
> +	       position = endPosition;

Would position = std::max(position, startPosition) work for this? Or even
position = std::clamp(position, startPosition, endPosition) (honestly not sure
if that even compiles).