Bug 215982

One thing not explicitly mentioned in the change log that makes this improvement possible is that all the Position operators now consistently work properly with shadow trees. Before, comparePositions would handle shadow trees correctly, but operators like < and > would not. Now they are all consistently correct in this respect so we can switch to the simpler idiom by using the operators directly. In the few cases where this is not possible, we still don't need a comparePositions function: we can call documentOrder or one of the documentOrder-related functions instead.

Created attachment 407705 [details]
Patch

Comment on attachment 407705 [details]
Patch

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

> Source/WebCore/editing/ios/EditorIOS.mm:372
> +    VisibleSelection selection;
>      selection.setBase(visiblePos);
>      selection.setExtent(visiblePos);

Any reason not to use the VisibleSelection constructor that takes VisiblePositions here? e.g. VisibleSelection selection { visiblePos, visiblePos };? I guess the affinity of selection might be different?

Comment on attachment 407705 [details]
Patch

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

>> Source/WebCore/editing/ios/EditorIOS.mm:372
>>      selection.setExtent(visiblePos);
> 
> Any reason not to use the VisibleSelection constructor that takes VisiblePositions here? e.g. VisibleSelection selection { visiblePos, visiblePos };? I guess the affinity of selection might be different?

We almost certainly could and should do that.

Comment on attachment 407705 [details]
Patch

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

>>> Source/WebCore/editing/ios/EditorIOS.mm:372
>>>      selection.setExtent(visiblePos);
>> 
>> Any reason not to use the VisibleSelection constructor that takes VisiblePositions here? e.g. VisibleSelection selection { visiblePos, visiblePos };? I guess the affinity of selection might be different?
> 
> We almost certainly could and should do that.

Not only that, there’s one that takes a single VisiblePosition and uses it for both base and extent, and we should use that!

This is passing tests. The "failure" on iOS-wk2 is a progression, probably a test doing better after r266399 that needs an updated expectation for this platform.

Sam, looks like you read the patch but didn’t review yet. Should I upload a new version with the VisibleSelection improvement?

(In reply to Darin Adler from comment #7)
> This is passing tests. The "failure" on iOS-wk2 is a progression, probably a
> test doing better after r266399 that needs an updated expectation for this
> platform.
> 
> Sam, looks like you read the patch but didn’t review yet. Should I upload a
> new version with the VisibleSelection improvement?

No, sorry, was just not on a computer last night. Finishing off the review now, no need to upload a new one.

Comment on attachment 407705 [details]
Patch

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

> Source/WebCore/editing/TextAffinity.h:31
> +enum Affinity : bool { Upstream, Downstream };

I think these would read clearer at callsites as an enum class, but I could see arguments either way and this is a clear improvement.

> Source/WebCore/editing/VisibleUnits.cpp:1065
> +    if (auto box = visiblePosition.inlineBoxAndOffset().box) {

A thought for the future. Given how often callers of inlineBoxAndOffset() want just the box, is there a worthwhile optimization of having a version that doesn't return the offset? Can any computation be avoided?

Committed r266487: <https://trac.webkit.org/changeset/266487>

<rdar://problem/68231268>