Bug 214261 – Remove live ranges from Editor.h and EditorClient.h

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Darin Adler]

Remove live ranges from Editor.h and EditorClient.h

[Darin Adler]

Created attachment 404157 [details]
Patch

[Darin Adler]

Created attachment 404169 [details]
Patch

[Darin Adler]

Created attachment 404172 [details]
Patch

[Darin Adler]

Created attachment 404184 [details]
Patch

[Darin Adler]

Created attachment 404201 [details]
Patch

[Darin Adler]

Created attachment 404675 [details]
Patch

[Darin Adler]

Created attachment 404680 [details]
Patch

[Darin Adler]

Created attachment 404685 [details]
Patch

[Darin Adler]

Created attachment 404689 [details]
Patch

[Darin Adler]

Created attachment 404691 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/65852960>

[Darin Adler]

Looking for a reviewer. This passes all the tests; totally clean green EWS results.

[Sam Weinig]

Comment on attachment 404691 [details]
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).

[Darin Adler]

Comment on attachment 404691 [details]
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.

Yes, I promise to return and finish the job.

>> Source/WebCore/dom/Range.cpp:1836
>> +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?

I don’t know if I am thinking clearly about this. My concept is that once you have a SimpleRange, it’s trivial to make a live range or a static range from it, so a simple range is the minimal range you can make. But for some reason that sounded better in my head than it does talking to you now.

And yes, once Range is renamed to LiveRange we could consider a different name for SimpleRange.

I think I should rename this back to makeSimpleRange for now.

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

I agree; didn’t want to do it because I was worried about the patch growing out of control. But I will do it.

>> Source/WebCore/editing/Editor.cpp:3495
>> +}
> 
> 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.

Yes, that makes sense. I wanted it to work on both const and non-const, and obviously T& achieves that.

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

Got it.

I don’t know what to name it. It’s the range to search within, if omitted the entire document will be searched. Part of the problem is that function name "rangeOfString" doesn’t make it obvious that it’s a function that involves searching.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1386
>> +            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).

Yes, I think it would.

[Darin Adler]

Committed r264692: <https://trac.webkit.org/changeset/264692>