[webkit-reviews] review granted: [Bug 211058] Replace more uses of live ranges with SimpleRange : [Attachment 397645] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 211058: Replace more uses of live ranges with SimpleRange
https://bugs.webkit.org/show_bug.cgi?id=211058

Attachment 397645: Patch

https://bugs.webkit.org/attachment.cgi?id=397645&action=review




--- Comment #6 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 397645
  --> https://bugs.webkit.org/attachment.cgi?id=397645
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.