Summary: | Replace more uses of live ranges with SimpleRange | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||
Component: | DOM | Assignee: | Darin Adler <darin> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aboxhall, andersca, apinheiro, cdumez, cfleizach, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, glenn, japhet, jcraig, jdiggs, jer.noble, kangil.han, koivisto, mifenton, philipj, samuel_white, sam, sergio, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Darin Adler
2020-04-26 17:37:15 PDT
Created attachment 397639 [details]
Patch
Created attachment 397640 [details]
Patch
Created attachment 397644 [details]
Patch
Created attachment 397645 [details]
Patch
Reducing live ranges is going to take a while, it seems. Might decide to set a breakpoint and prioritize paths that are called during normal webpage loads next. For this patch I just searched for instances of Range::create and then followed up with whatever changes were needed to make them go away. Comment on attachment 397645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397645&action=review > Source/WebCore/dom/Node.cpp:2624 > +RefPtr<Node> commonInclusiveAncestor(Node& a, Node& b) > +{ > + for (auto ancestorA = &a; ancestorA; ancestorA = ancestorA->parentNode()) { > + for (auto ancestorB = &b; ancestorB; ancestorB = ancestorB->parentNode()) { > + if (ancestorA == ancestorB) > + return ancestorA; > + } > + } > + return nullptr; > +} This is a pretty inefficient implementation (I know it is not new in this patch). > Source/WebCore/editing/VisibleSelection.cpp:210 > + if (!scope) > + return WTF::nullopt; I would just return { }; > Source/WebCore/editing/cocoa/DataDetection.h:52 > +enum class DataDetectorTypes : uint32_t { > + None = 0, > + PhoneNumber = 1 << 0, > + Link = 1 << 1, > + Address = 1 << 2, > + CalendarEvent = 1 << 3, > + TrackingNumber = 1 << 4, > + FlightNumber = 1 << 5, > + LookupSuggestion = 1 << 6, > +}; Simon would tell you to line these up. Comment on attachment 397645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397645&action=review >> Source/WebCore/dom/Node.cpp:2624 >> +} > > This is a pretty inefficient implementation (I know it is not new in this patch). Yes definitely. I noticed that, too. Do you have ideas for a better one? I didn’t immediately think of one. >> Source/WebCore/editing/VisibleSelection.cpp:210 >> + return WTF::nullopt; > > I would just return { }; That does seem appealing. Although I have checked in a lot of “return nullopt” in patches recently. Comment on attachment 397645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397645&action=review >>> Source/WebCore/dom/Node.cpp:2624 >>> +} >> >> This is a pretty inefficient implementation (I know it is not new in this patch). > > Yes definitely. I noticed that, too. Do you have ideas for a better one? I didn’t immediately think of one. I looked at https://en.wikipedia.org/wiki/Lowest_common_ancestor and did not immediately find something useful. Maybe one trip each to find the roots and distance to the root, return null if the roots are different, then walk up both at the same time, keeping distance from root constant. Comment on attachment 397645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397645&action=review >>>> Source/WebCore/dom/Node.cpp:2624 >>>> +} >>> >>> This is a pretty inefficient implementation (I know it is not new in this patch). >> >> Yes definitely. I noticed that, too. Do you have ideas for a better one? I didn’t immediately think of one. > > I looked at https://en.wikipedia.org/wiki/Lowest_common_ancestor and did not immediately find something useful. I’m going to fix this in a follow-up patch now that I figured out how. >>> Source/WebCore/editing/VisibleSelection.cpp:210 >>> + return WTF::nullopt; >> >> I would just return { }; > > That does seem appealing. Although I have checked in a lot of “return nullopt” in patches recently. Will do this before landing. >> Source/WebCore/editing/cocoa/DataDetection.h:52 >> +}; > > Simon would tell you to line these up. Not sure how to resolve this minor code style disagreement. Committed r260753: <https://trac.webkit.org/changeset/260753> Comment on attachment 397645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397645&action=review >>>>> Source/WebCore/dom/Node.cpp:2624 >>>>> +} >>>> >>>> This is a pretty inefficient implementation (I know it is not new in this patch). >>> >>> Yes definitely. I noticed that, too. Do you have ideas for a better one? I didn’t immediately think of one. >> >> I looked at https://en.wikipedia.org/wiki/Lowest_common_ancestor and did not immediately find something useful. > > I’m going to fix this in a follow-up patch now that I figured out how. Bug 211078. > That does seem appealing. Although I have checked in a lot of “return
> nullopt” in patches recently.
I know and think both approaches are fine.
Comment on attachment 397645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397645&action=review > Source/WebCore/page/TextIndicator.cpp:200 > - auto* node = iterator.node(); > - if (!is<Text>(node) || !is<RenderText>(node->renderer())) > - continue; > - > - colors.add(node->renderer()->style().color()); > + auto renderer = iterator.node()->renderer(); > + if (is<RenderText>(renderer)) > + colors.add(renderer->style().color()); This change accidentally removed the null check done by is<Text>(node). We don’t need and is<Text> check, but we do need a null check. |