Bug 171645 – Drop remaining uses of PassRefPtr from CompositeEditCommand

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Drop remaining uses of PassRefPtr from CompositeEditCommand.

[Chris Dumez]

Created attachment 309013 [details]
Patch

[Chris Dumez]

Created attachment 309015 [details]
Patch

[Darin Adler]

Comment on attachment 309015 [details]
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.

[Chris Dumez]

Comment on attachment 309015 [details]
Patch

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

>> Source/WebCore/editing/CompositeEditCommand.cpp:308
>>  }
> 
> 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.

command is not used in this function after the call to apply(). Therefore, I do not think ref'ing command for the duration of this function can be useful. I would agree it would make sense to get rid of this function all together.

>> Source/WebCore/editing/CompositeEditCommand.cpp:654
>> +        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.

ok

>> 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.

It is hard to tell, I think I will protect it to maintain previous behavior.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:654
>>      }
> 
> Maybe should break this up:
> 
>     if (!endBlock)
>         return;
>     if (!endBlock->contains[...]) {
>         removeNode(*endBlock);
>         return;
>     }

ok

>> Source/WebCore/editing/Editor.cpp:3624
>> +            applyCommand(replaceCommand);
> 
> Shorter as a one-liner.

ok

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:227
>> +void ReplacementFragment::removeNodePreservingChildren(Node& node)
> 
> I think the node needs to be protected while removing its children.

ok.

[Chris Dumez]

Created attachment 309138 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 309138 [details]
Patch

Clearing flags on attachment: 309138

Committed r216233: <http://trac.webkit.org/changeset/216233>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 309015 [details]
Patch

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

>>> Source/WebCore/editing/CompositeEditCommand.cpp:308
>>>  }
>> 
>> 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.
> 
> command is not used in this function after the call to apply(). Therefore, I do not think ref'ing command for the duration of this function can be useful. I would agree it would make sense to get rid of this function all together.

I was worrying about code *inside* apply indirectly causing the command to be deref’d. Maybe that’s not a real issue.

[Chris Dumez]

(In reply to Darin Adler from comment #8)
> Comment on attachment 309015 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309015&action=review
> 
> >>> Source/WebCore/editing/CompositeEditCommand.cpp:308
> >>>  }
> >> 
> >> 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.
> > 
> > command is not used in this function after the call to apply(). Therefore, I do not think ref'ing command for the duration of this function can be useful. I would agree it would make sense to get rid of this function all together.
> 
> I was worrying about code *inside* apply indirectly causing the command to
> be deref’d. Maybe that’s not a real issue.

Oh, I undestand now. I'll double check to make sure it is safe.