Bug 64414 - CompositeEditCommand should not be kept alive for undo and redo
Summary: CompositeEditCommand should not be kept alive for undo and redo
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 64416 73984
Blocks: 74059 74249
  Show dependency treegraph
 
Reported: 2011-07-12 17:32 PDT by Ryosuke Niwa
Modified: 2011-12-10 17:10 PST (History)
7 users (show)

See Also:


Attachments
outdated work in progress patch (6.04 KB, patch)
2011-07-13 16:37 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
cleanup (20.93 KB, patch)
2011-12-08 02:03 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated for ToT (20.90 KB, patch)
2011-12-08 02:11 PST, Ryosuke Niwa
enrica: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-07-12 17:32:36 PDT
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.
Comment 1 Ryosuke Niwa 2011-07-13 16:37:25 PDT
Created attachment 100729 [details]
outdated work in progress patch
Comment 2 Ryosuke Niwa 2011-12-08 02:03:53 PST
Created attachment 118349 [details]
cleanup
Comment 3 Ryosuke Niwa 2011-12-08 02:07:39 PST
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.
Comment 4 Ryosuke Niwa 2011-12-08 02:11:39 PST
Created attachment 118352 [details]
Updated for ToT
Comment 5 Ryosuke Niwa 2011-12-08 09:10:43 PST
Ping reviewers.
Comment 6 Enrica Casucci 2011-12-08 10:08:41 PST
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.
Comment 7 Ryosuke Niwa 2011-12-08 10:43:36 PST
(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 8 Enrica Casucci 2011-12-08 10:59:47 PST
Comment on attachment 118352 [details]
Updated for ToT

thanks for the clarification. R=me with the additional ASSERT.
Comment 9 Ryosuke Niwa 2011-12-08 11:32:22 PST
Committed r102357: <http://trac.webkit.org/changeset/102357>