Summary: | Create documentOrder function, start refactoring to use it instead of Range::compare functions | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||
Component: | DOM | Assignee: | Darin Adler <darin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cdumez, cfleizach, changseok, dmazzoni, esprehn+autocc, ews-watchlist, gyuyoung.kim, jcraig, jdiggs, kangil.han, mifenton, samuel_white, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=215677 | ||||||||
Attachments: |
|
Description
Darin Adler
2020-08-16 14:47:44 PDT
Created attachment 406686 [details]
Patch
Won’t be landing this for another week, but might be ready for review now. 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. Created attachment 406755 [details]
Patch
I’ll be landing this on Saturday, but would appreciate a review sooner. This is passing all tests on EWS now. Would love to get this reviewed so I can land it in a couple days. 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). 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. 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. 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? Committed r266026: <https://trac.webkit.org/changeset/266026> |