[webkit-reviews] review granted: [Bug 171787] Drop remaining uses of PassRefPtr in editing code : [Attachment 309318] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 171787: Drop remaining uses of PassRefPtr in editing code
https://bugs.webkit.org/show_bug.cgi?id=171787

Attachment 309318: Patch

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




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

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

Generally the way Range is used in editing code is wrong and a bit dangerous. A
Range is a mutable object, so it’s not OK to just take a reference to one
unless the caller promises to never change the range after passing it. Code
that wants to store a Range without a guaranteed about what the caller will do
with it needs to call cloneRange instead. We should replace the use of Range
with something with more appropriate semantics for what we want to do with it
in editing code. Could make things faster by not having to register/unregister
each Range as we create it, too, if we can do without the automatic Range
adjustment that happens when mutating documents.

> Source/WebCore/editing/ApplyStyleCommand.cpp:60
> -static int toIdentifier(PassRefPtr<CSSValue> value)
> +static int toIdentifier(RefPtr<CSSValue>&& value)

Why a RefPtr? Should just be a raw pointer.

> Source/WebCore/editing/ApplyStyleCommand.cpp:62
> -    return (value && value->isPrimitiveValue()) ?
static_pointer_cast<CSSPrimitiveValue>(value)->valueID() : 0;
> +    return (value && value->isPrimitiveValue()) ?
downcast<CSSPrimitiveValue>(*value).valueID() : 0;

Moving to use downcast<>, would be natural to also use is<>.

    return is<CSSPrimitiveValue>(value) ?
downcast<CSSPrimitiveValue>(*value).valueID() : 0;

> Source/WebCore/editing/CompositeEditCommand.cpp:1721
> -RefPtr<Node> CompositeEditCommand::splitTreeToNode(Node* start, Node* end,
bool shouldSplitAncestor)
> +RefPtr<Node> CompositeEditCommand::splitTreeToNode(Node& start, Node& end,
bool shouldSplitAncestor)
>  {
> -    ASSERT(start);
> -    ASSERT(end);
> -    ASSERT(start != end);
> +    ASSERT(&start != &end);
>  
> -    RefPtr<Node> node;
> -    if (shouldSplitAncestor && end->parentNode())
> -	   end = end->parentNode();
> +    Node* adjustedEnd = &end;
> +    if (shouldSplitAncestor && adjustedEnd->parentNode())
> +	   adjustedEnd = adjustedEnd->parentNode();
>  
> -    RefPtr<Node> endNode = end;
> -    for (node = start; node && node->parentNode() != endNode; node =
node->parentNode()) {
> +    ASSERT(adjustedEnd);
> +    RefPtr<Node> node;
> +    for (node = &start; node && node->parentNode() != adjustedEnd; node =
node->parentNode()) {

If "node" needs to be a RefPtr, I would think that "adjustedEnd" also needs to
be a RefPtr for the same reason.

> Source/WebCore/editing/Editor.cpp:529
> +    auto request =
SpellCheckRequest::create(resolveTextCheckingTypeMask(*nodeToCheck,
TextCheckingTypeSpelling | TextCheckingTypeGrammar), TextCheckingProcessBatch,
rangeToCheck.copyRef(), rangeToCheck.copyRef());
> +    if (request)

Better style to define the request variable inside the if statement.

> Source/WebCore/editing/Editor.cpp:959
> +    EditCommandComposition* composition = command.composition();
>      ASSERT(composition);

If this can’t be null seems like the local variable should be a reference.

    ASSERT(command.composition());
    auto& composition = *command.composition();

> Source/WebCore/editing/Editor.cpp:2054
> +	   RefPtr<Range> misspellingRange =
TextIterator::subrange(spellingSearchRange.ptr(), misspellingOffset,
misspelledWord.length());

Result of subrange is Ref, not RefPtr.

> Source/WebCore/editing/Editor.h:274
> -    void markAndReplaceFor(PassRefPtr<SpellCheckRequest>, const
Vector<TextCheckingResult>&);
> +    void markAndReplaceFor(SpellCheckRequest&, const
Vector<TextCheckingResult>&);

Maybe const& for request? The function does not modify the request.

> Source/WebCore/editing/Editor.h:302
> +    void willWriteSelectionToPasteboard(Range*);

Can this really be null? Seems unlikely to me, although not critical to tackle
this at this time.

> Source/WebCore/editing/InsertListCommand.cpp:64
>      m_listElement = listElement.copyRef();
> -    return listElement.ptr();
> +    return listElement.get();

Another way to write this that does less reference count churn:

    m_listElement = WTFMove(listElement);
    return m_listElement.get();

Which also suggests that perhaps this function doesn’t need a return value.

> Source/WebCore/editing/InsertListCommand.cpp:211
> +	       listNode = &fixOrphanedListChild(*listChildNode);
> +	       listNode = mergeWithNeighboringLists(*listNode);

Maybe this should be a one-liner instead?

    listNode = mergeWithNeighboringLists(fixOrphanedListChild(*listChildNode));

Won’t work, though if we get rid of the return value as I suggested above.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1466
> +    RefPtr<HTMLElement> listElement = &passedListElement;
>  
>      while (listElement->hasOneChild() &&
isListHTMLElement(listElement->firstChild()))
>	   listElement = downcast<HTMLElement>(listElement->firstChild());
> +    ASSERT(listElement);

Seems unnecessary to assert this. Code above clearly can’t set listElement to
null. If you wanted to be even clearer about it you could rewrite the line
above to this:

    listElement = &downcast<HTMLElement>(*listElement->firstChild());

Another way to do this is to factor out this bit of logic into a
deepestSingleChildList function that takes a HTMLElement& and returns a
HTMLElement& and then use it like this:

    static HTMLElement* singleChildList(HTMLElement& parent)
    {
	if (!parent->hasOneChild())
	    return nullptr;
	auto& child = *list->firstChild();
	if (!isListHTMLElement(&child))
	    return nullptr;
	return &child;
    }

    static HTMLElement& deepestSingleChildList(HTMLElement& topLevelList)
    {
	auto* list = &topLevelList;
	while (auto* childList = singleChildList(*list))
	    list = childList;
	return *list;
    }

    ...

    Ref<HTMLElement> listElement = deepestSingleChildList(passedListElement);

    ...

While that is a lot longer, I like it better.

> Source/WebCore/editing/TextCheckingHelper.h:40
> +    TextCheckingParagraph(TextCheckingParagraph&&) = default;
> +    TextCheckingParagraph& operator=(TextCheckingParagraph&&) = default;

Surprised we need to declare these explicitly. I’d expect the compiler to do
the write thing without us specifying this. What happens if we leave these out?