[webkit-reviews] review granted: [Bug 190107] Use Position instead of Range in createMarkupInternal : [Attachment 351169] Cleanup

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 190107: Use Position instead of Range in createMarkupInternal
https://bugs.webkit.org/show_bug.cgi?id=190107

Attachment 351169: Cleanup

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




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 351169
  --> https://bugs.webkit.org/attachment.cgi?id=351169
Cleanup

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

This change is definitely a good idea. Using Range internally in WebKit code is
not a great pattern for multiple reasons; creating and destroying Range objects
is quite costly because of they feature where ranges automatically update as a
document is mutated.

On the other hand, maybe some day we will want to use a structure holding two
Position objects for this kind of thing rather than two separate Position
objects.

Functions that take two Position objects need to be prepared for them to be out
of order. The same is not true for functions that take Range objects. I am
concerned that we don’t get this exactly right, although I didn't spot any
obvious problems here.

> Source/WebCore/dom/Position.cpp:253
> +    auto container = makeRefPtr(containerNode());

Seems strange to use a RefPtr here, but not for the result of
computeNodeAfterPosition. Neither needs it.

> Source/WebCore/dom/Position.cpp:256
> +    if (is<CharacterData>(container))

Can be "*character"; might save a null check or might have no effect.

> Source/WebCore/dom/Position.h:106
> +    RefPtr<Node> firstNode() const;

In the old days this would return a Node*, not a RefPtr<Node>, but maybe it’s
good design direction for newly written functions to use RefPtr to make the use
of the function safer.

> Source/WebCore/editing/MarkupAccumulator.cpp:123
>      , m_prefixLevel(0)

Should initialize this in class definition.

> Source/WebCore/editing/markup.cpp:223
> +    StyledMarkupAccumulator(const Position& start, const Position& end,
Vector<Node*>* nodes,
> +	   ResolveURLs, AnnotateForInterchange, MSOListMode, bool
needsPositionStyleConversion, Node* highestNodeToBeSerialized = nullptr);

I think that soon this class should be broken out into its own source file.

> Source/WebCore/editing/markup.cpp:396
> +    if (m_start.isNull() && m_end.isNull())
> +	   return text.data();

This special case is not needed; the code below will do the same. I also don’t
think it’s an important performance optimization.

> Source/WebCore/editing/markup.cpp:398
> +    String textData = text.data();

We don’t really need this local variable.

> Source/WebCore/editing/markup.cpp:400
> +    unsigned end = textData.length();

This could be std::numeric_limits<unsigned>::max() with no effect on the
behavior of the function; substring clamps properly.

> Source/WebCore/editing/markup.cpp:405
> +    ASSERT(start < end);

This is a very late moment to check this invariant; the mistake that made it
untrue will be *very* far from here. It’s possible that m_start and m_end will
both point to the same text node, and the offsets will be out of order with the
start one after the end one. I don’t know where the checks that guarantee that
won’t happen are; I don’t think they are anywhere inside the
StyledMarkupAccumulator class.

I think this is a fragile way to construct the code. One possible solution
would be to make this function robust in the face of such unusual inputs, which
would be very easy to do, just replace "end - start" with zero-clamping
subtraction rather than standard subtraction that will underflow and result in
a large number. Another solution would be to enforce the invariant in the
constructor.

This is a new problem that did not exist when we were using Range, since the
Range constructor does enforce this invariant.

> Source/WebCore/editing/markup.cpp:509
> +    Node* startNode = start.firstNode().get();

This line of code seems dangerous. Why is it OK to just call get on a RefPtr
and then let it fall out of scope? Doing get() here seems inconsistent with the
design direction that inspired us to make the firstNode function return a
RefPtr.

> Source/WebCore/editing/markup.cpp:762
> +    if (start.isNull() || end.isNull() || !comparePositions(start, end))

The two null checks here aren’t needed. The comparePositions function already
returns 0 if either of the two positions is null.

But also, it seems a bit peculiar and a bit expensive to call comparePositions
here and it’s not clear what the check is intended to do. After all
"!comparePositions(a, b)" is roughly the same as "a == b". The expensive
operation is figuring out -1 vs. +1, not 0 vs. non-0. Is the check truly
required?

> Source/WebCore/editing/markup.h:49
> +class VisibleSelection;

Why are we adding this in this patch? Maybe it’s part of the next patch?

> Source/WebCore/editing/markup.h:74
> -WEBCORE_EXPORT String serializePreservingVisualAppearance(const Range&,
Vector<Node*>* = nullptr, AnnotateForInterchange = AnnotateForInterchange::No,
ConvertBlocksToInlines = ConvertBlocksToInlines::No, ResolveURLs =
ResolveURLs::No);
> +WEBCORE_EXPORT String serializePreservingVisualAppearance(const Range&,
Vector<Node*>* = nullptr,
> +    AnnotateForInterchange = AnnotateForInterchange::No,
ConvertBlocksToInlines = ConvertBlocksToInlines::No, ResolveURLs =
ResolveURLs::No);

This change just seems to be adding a line break. Can we omit it? Maybe it
makes more sense in the follow-up patch.