Bug 64414 – CompositeEditCommand should not be kept alive for undo and redo

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

As discussed on webkit-dev, we should make CompositeEditCommand a temporary object that exits only to execute a command.  To reduce the memory footage and to support persistent positions, we should not keep it for undo and redo purposes.

We can instead generate a EditCommandComposition that outlives CompositeEditCommand to implement undo and redo.

[Ryosuke Niwa]

Created attachment 100729 [details]
outdated work in progress patch

[Ryosuke Niwa]

Created attachment 118349 [details]
cleanup

[Ryosuke Niwa]

My current plan:
1. Land this patch
2. Change the argument type of registerCommandForUndo, registerCommandForRedo, and clearUndoRedoOperations from EditCommand* to EditCommandComposition* (perhaps a better name like DOMTransaction).
3. Make CompositeEditCommand not inherit from EditCommand.

[Ryosuke Niwa]

Created attachment 118352 [details]
Updated for ToT

[Ryosuke Niwa]

Ping reviewers.

[Enrica Casucci]

Comment on attachment 118352 [details]
Updated for ToT

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

> Source/WebCore/editing/CompositeEditCommand.cpp:138
> +    RefPtr<EditCommand> command = prpCommand;

Why do you need this assignment? Do you need to keep a reference while executing the code below? Can apply() change something?

> Source/WebCore/editing/CompositeEditCommand.cpp:150
> +    RefPtr<CompositeEditCommand> command = prpCommand;

Ditto. (Here I don't see the need for the reference at all).

> Source/WebCore/editing/CompositeEditCommand.h:154
> +{

Should we at least assert the command is not null?

> Source/WebCore/editing/CompositeEditCommand.h:159
> +inline CompositeEditCommand* toCompositeEditCommand(EditCommand* command)

Ditto.

> Source/WebCore/editing/EditCommand.h:104
> +{

I would add an ASSERT that command is not null.

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 118352 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118352&action=review
> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:138
> > +    RefPtr<EditCommand> command = prpCommand;
> 
> Why do you need this assignment? Do you need to keep a reference while executing the code below? Can apply() change something?

prpCommand is a pass ref ptr so calling composition()->append or m_command.append will clear prpCommand :( I could call get() but that seemed ugly.

> > Source/WebCore/editing/CompositeEditCommand.cpp:150
> > +    RefPtr<CompositeEditCommand> command = prpCommand;
> 
> Ditto. (Here I don't see the need for the reference at all).

Ditto.

> > Source/WebCore/editing/CompositeEditCommand.h:154
> > +{
> 
> Should we at least assert the command is not null?
>
> > Source/WebCore/editing/CompositeEditCommand.h:159
> > +inline CompositeEditCommand* toCompositeEditCommand(EditCommand* command)
> 
> Ditto.
> 
> > Source/WebCore/editing/EditCommand.h:104
> > +{
> 
> I would add an ASSERT that command is not null.

Sure. Will do.

[Enrica Casucci]

Comment on attachment 118352 [details]
Updated for ToT

thanks for the clarification. R=me with the additional ASSERT.

[Ryosuke Niwa]

Committed r102357: <http://trac.webkit.org/changeset/102357>