Bug 214261 - Remove live ranges from Editor.h and EditorClient.h
Summary: Remove live ranges from Editor.h and EditorClient.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-13 10:06 PDT by Darin Adler
Modified: 2020-07-21 21:45 PDT (History)
18 users (show)

See Also:


Attachments
Patch (143.49 KB, patch)
2020-07-13 10:56 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (145.98 KB, patch)
2020-07-13 13:11 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (148.60 KB, patch)
2020-07-13 14:07 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (150.75 KB, patch)
2020-07-13 15:15 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (159.69 KB, patch)
2020-07-13 18:53 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (164.81 KB, patch)
2020-07-19 10:32 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (167.08 KB, patch)
2020-07-19 13:06 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (167.08 KB, patch)
2020-07-19 14:19 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (167.96 KB, patch)
2020-07-19 15:30 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (186.80 KB, patch)
2020-07-19 18:03 PDT, Darin Adler
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-07-13 10:06:45 PDT
Remove live ranges from Editor.h and EditorClient.h
Comment 1 Darin Adler 2020-07-13 10:56:01 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-07-13 13:11:01 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2020-07-13 14:07:45 PDT Comment hidden (obsolete)
Comment 4 Darin Adler 2020-07-13 15:15:51 PDT Comment hidden (obsolete)
Comment 5 Darin Adler 2020-07-13 18:53:15 PDT Comment hidden (obsolete)
Comment 6 Darin Adler 2020-07-19 10:32:07 PDT Comment hidden (obsolete)
Comment 7 Darin Adler 2020-07-19 13:06:22 PDT Comment hidden (obsolete)
Comment 8 Darin Adler 2020-07-19 14:19:14 PDT Comment hidden (obsolete)
Comment 9 Darin Adler 2020-07-19 15:30:30 PDT Comment hidden (obsolete)
Comment 10 Darin Adler 2020-07-19 18:03:11 PDT
Created attachment 404691 [details]
Patch
Comment 11 Radar WebKit Bug Importer 2020-07-20 16:03:36 PDT
<rdar://problem/65852960>
Comment 12 Darin Adler 2020-07-21 13:01:02 PDT
Looking for a reviewer. This passes all the tests; totally clean green EWS results.
Comment 13 Sam Weinig 2020-07-21 15:44:42 PDT
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).
Comment 14 Darin Adler 2020-07-21 16:42:28 PDT
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.
Comment 15 Darin Adler 2020-07-21 21:45:46 PDT
Committed r264692: <https://trac.webkit.org/changeset/264692>