[webkit-reviews] review granted: [Bug 171645] Drop remaining uses of PassRefPtr from CompositeEditCommand : [Attachment 309015] 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 171645: Drop remaining uses of PassRefPtr from CompositeEditCommand
https://bugs.webkit.org/show_bug.cgi?id=171645

Attachment 309015: Patch

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




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

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

> Source/WebCore/editing/CompositeEditCommand.cpp:308
> -void applyCommand(PassRefPtr<CompositeEditCommand> command)
> +void applyCommand(CompositeEditCommand& command)
>  {
> -    command->apply();
> +    command.apply();
>  }

This gets rid of the protection that references the command for the duration of
apply. Is that OK?

If we don’t need protection, then why do we need this function if this is
literally its implementation? Seems like we should dump it and change call
sites to call apply directly.

> Source/WebCore/editing/CompositeEditCommand.cpp:654
> +    if (RefPtr<Node> highestNodeToRemove =
highestNodeToRemoveInPruning(node))
> +	   removeNode(*highestNodeToRemove);

Seems strange to get a RefPtr here instead of a raw pointer just to call
removeNode. Seems that removeNode itself needs to protect the node, but not the
caller.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:379
> +void DeleteSelectionCommand::removeNode(Node& node,
ShouldAssumeContentIsAlwaysEditable shouldAssumeContentIsAlwaysEditable)

Is this function guaranteed to do the right thing without protecting "node"? I
couldn’t tell if so just by reading the code quickly.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:654
>      if (!endBlock ||
!endBlock->contains(startOfParagraphToMove.deepEquivalent().deprecatedNode())
|| !startOfParagraphToMove.deepEquivalent().deprecatedNode()) {
> -	   removeNode(enclosingBlock(m_downstreamEnd.deprecatedNode()));
> +	   if (endBlock)
> +	       removeNode(*endBlock);
>	   return;
>      }

Maybe should break this up:

    if (!endBlock)
	return;
    if (!endBlock->contains[...]) {
	removeNode(*endBlock);
	return;
    }

> Source/WebCore/editing/Editor.cpp:3624
> +	       auto replaceCommand =
ReplaceRangeWithTextCommand::create(range.get(),
acceptedCandidate.replacement);
> +	       applyCommand(replaceCommand);

Shorter as a one-liner.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:227
> +void ReplacementFragment::removeNodePreservingChildren(Node& node)

I think the node needs to be protected while removing its children.