Bug 215982 – Remove comparePositions and make VisiblePosition improvements

• Component: HTML Editing
• Status: RESOLVED FIXED

[Darin Adler]

Remove comparePositions and make VisiblePosition improvements

[Darin Adler]

Created attachment 407561 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Created attachment 407705 [details]
Patch

[Sam Weinig]

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?

[Darin Adler]

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.

[Darin Adler]

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!

[Darin Adler]

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?

[Sam Weinig]

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

[Sam Weinig]

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?

[Darin Adler]

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

[Radar WebKit Bug Importer]

<rdar://problem/68231268>