[webkit-reviews] review granted: [Bug 217886] Optimize Range and StaticRange by having AbstractRange derive from SimpleRange : [Attachment 415915] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 217886: Optimize Range and StaticRange by having AbstractRange derive from
SimpleRange
https://bugs.webkit.org/show_bug.cgi?id=217886

Attachment 415915: Patch

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




--- Comment #20 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 415915
  --> https://bugs.webkit.org/attachment.cgi?id=415915
Patch

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

> Source/WebCore/dom/AbstractRange.cpp:32
> +AbstractRange::AbstractRange(SimpleRange&& range)
> +    : SimpleRange(WTFMove(range))

It seems like we can just inline this now?

> Source/WebCore/dom/AbstractRange.cpp:38
> +    return range;

Ditto.

> Source/WebCore/dom/AbstractRange.cpp:41
> -SimpleRange makeSimpleRange(const Ref<AbstractRange>& range)
> +const SimpleRange& makeSimpleRange(const Ref<AbstractRange>& range)

Ditto.

> Source/WebCore/dom/Range.cpp:131
> +    return setBoundary(start, m_childBeforeStart, end, m_childBeforeEnd,
WTFMove(container), offset);

Hm... the fact SimpleRange doesn't use m_start & m_end makes this code a bit
harder to parse/read.
I wonder if we should make SimpleRange a member of AbstractRange instead so
that we can access these as m_range.start & m_range.end?

> Source/WebCore/dom/Range.cpp:204
> -    auto startOrdering = treeOrder(nodeRange->start,
makeBoundaryPoint(m_start));
> -    auto endOrdering = treeOrder(nodeRange->end, makeBoundaryPoint(m_end));
> +    auto startOrdering = treeOrder(nodeRange->start, start);
> +    auto endOrdering = treeOrder(nodeRange->end, end);

Nice!

> Source/WebCore/dom/Range.cpp:230
> -	   thisPoint = &m_start;
> -	   otherPoint = &sourceRange.m_start;
> -	   break;
> +	   return compareBoundaryPoints(start, sourceRange.start);

This is a much cleaner code too.

> Source/WebCore/dom/Range.cpp:930
> +    if (offset + length >= boundary.offset)
> +	   boundary.offset = offset;
>      else
> -	   boundary.setOffset(boundaryOffset - length);
> +	   boundary.offset -= length;

Hm... I wish there was some kind of validation of these boundary offsets.
Maybe we can add makeBoundaryPoint and have that function validate the offset?

> Source/WebCore/dom/Range.cpp:944
> +	   boundary = { *oldNode.node().previousSibling(), boundary.offset +
offset };

Here again, I wish we can assert the the offset is valid.

> Source/WebCore/dom/Range.cpp:946
> +	   boundary = { *oldNode.node().previousSibling(), offset };

Ditto.

> Source/WebCore/dom/Range.cpp:969
> -		   boundary.set(*oldNode.nextSibling(), boundaryOffset -
splitOffset, 0);
> +		   boundary = { *oldNode.nextSibling(), boundary.offset -
splitOffset };

Ditto.

> Source/WebCore/dom/Range.cpp:971
> -		   boundary.setOffset(splitOffset);
> +		   boundary.offset = splitOffset;

Ditto.

> Source/WebCore/dom/Range.cpp:1052
> +const SimpleRange& makeSimpleRange(const Ref<Range>& range)

It seems like we can inline this now?

> Source/WebCore/dom/Range.cpp:1057
>  Optional<SimpleRange> makeSimpleRange(const RefPtr<Range>& range)

Ditto.

> Source/WebCore/dom/StaticRange.cpp:78
> +const SimpleRange& makeSimpleRange(const Ref<StaticRange>& range)

Same comment about making these inline.

> Source/WebCore/dom/StaticRange.h:-49
> -    Node& startContainer() const final { return
SimpleRange::startContainer(); }

Very nice!