Bug 211058 – Replace more uses of live ranges with SimpleRange

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

[Darin Adler]

Replace more uses of live ranges with SimpleRange

[Darin Adler]

Created attachment 397639 [details]
Patch

[Darin Adler]

Created attachment 397640 [details]
Patch

[Darin Adler]

Created attachment 397644 [details]
Patch

[Darin Adler]

Created attachment 397645 [details]
Patch

[Darin Adler]

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.

[Antti Koivisto]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

Committed r260753: <https://trac.webkit.org/changeset/260753>

[Radar WebKit Bug Importer]

<rdar://problem/62452499>

[Darin Adler]

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.

[Antti Koivisto]

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

[Darin Adler]

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.