Bug 215551 – Create documentOrder function, start refactoring to use it instead of Range::compare functions

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

Create documentOrder function, start refactoring to use it instead of Range::compare functions

[Darin Adler]

Created attachment 406686 [details]
Patch

[Darin Adler]

Won’t be landing this for another week, but might be ready for review now.

[Darin Adler]

Comment on attachment 406686 [details]
Patch

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

> Source/WebCore/dom/Node.h:676
> +enum class PartialOrdering : uint8_t { Less, Equivalent, Greater, Unordered };

This was based on my misunderstanding of std::partial_ordering, which is not an enumeration. The switch statements won’t be possible if I change this to be a class like std::partial_ordering is, but I suspect I should do that so we can eventually switch over to std::partial_ordering with minimal changes.

[Darin Adler]

Created attachment 406755 [details]
Patch

[Darin Adler]

I’ll be landing this on Saturday, but would appreciate a review sooner. This is passing all tests on EWS now.

[Darin Adler]

Would love to get this reviewed so I can land it in a couple days.

[Chris Dumez]

Comment on attachment 406755 [details]
Patch

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

r=me

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1947
>      if (isNativeTextControl() && is<HTMLTextFormControlElement>(*node))

The is<HTMLTextFormControlElement>(*node) could be faster if we now leveraged the fact that we already know node is an Element. is<HTMLTextFormControlElement>(*node) does an is<WebCore::Element>(node) check internally when you pass it a Node.

> Source/WebCore/dom/Node.h:676
> +class PartialOrdering {

Can you explain why we're not using a simple enum? I personally find this class a bit confusing and not very WebKit like (is_eq() for e.g. does not seem to match WebKit coding style).

[Darin Adler]

Comment on attachment 406755 [details]
Patch

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

>> Source/WebCore/dom/Node.h:676
>> +class PartialOrdering {
> 
> Can you explain why we're not using a simple enum? I personally find this class a bit confusing and not very WebKit like (is_eq() for e.g. does not seem to match WebKit coding style).

Yes, not my design or specific to WebKit.

This is out of C++20, with the name changed from std::partial_ordering to WebCore::PartialOrdering (it’s a small subset, but compatible, of the real std::partial_ordering). My goal is to position us to easily switch to std::partial_ordering once we start using C++20. There will be benefits to using the standard class.

https://en.cppreference.com/w/cpp/utility/compare/partial_ordering
https://en.cppreference.com/w/cpp/utility/compare/named_comparison_functions

I used an enumeration in my first cut, then realized that’s not how std::partial_ordering works, and changed it to match.

[Darin Adler]

Comment on attachment 406755 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1947
>>      if (isNativeTextControl() && is<HTMLTextFormControlElement>(*node))
> 
> The is<HTMLTextFormControlElement>(*node) could be faster if we now leveraged the fact that we already know node is an Element. is<HTMLTextFormControlElement>(*node) does an is<WebCore::Element>(node) check internally when you pass it a Node.

Yes, I’ll fix that.

[Darin Adler]

Comment on attachment 406755 [details]
Patch

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

>>> Source/WebCore/dom/Node.h:676
>>> +class PartialOrdering {
>> 
>> Can you explain why we're not using a simple enum? I personally find this class a bit confusing and not very WebKit like (is_eq() for e.g. does not seem to match WebKit coding style).
> 
> Yes, not my design or specific to WebKit.
> 
> This is out of C++20, with the name changed from std::partial_ordering to WebCore::PartialOrdering (it’s a small subset, but compatible, of the real std::partial_ordering). My goal is to position us to easily switch to std::partial_ordering once we start using C++20. There will be benefits to using the standard class.
> 
> https://en.cppreference.com/w/cpp/utility/compare/partial_ordering
> https://en.cppreference.com/w/cpp/utility/compare/named_comparison_functions
> 
> I used an enumeration in my first cut, then realized that’s not how std::partial_ordering works, and changed it to match.

Maybe I need to add a comment making this clear?

[Darin Adler]

Committed r266026: <https://trac.webkit.org/changeset/266026>

[Radar WebKit Bug Importer]

<rdar://problem/67617503>