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.
Created attachment 100729 [details] outdated work in progress patch
Created attachment 118349 [details] cleanup
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.
Created attachment 118352 [details] Updated for ToT
Ping reviewers.
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.
(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.
Comment on attachment 118352 [details] Updated for ToT thanks for the clarification. R=me with the additional ASSERT.
Committed r102357: <http://trac.webkit.org/changeset/102357>