[webkit-reviews] review requested: [Bug 42941] Redo fails after text node is split by SplitTextNodeCommand : [Attachment 62546] Added a test and fix per Darin's comment.

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has asked  for review:
Bug 42941: Redo fails after text node is split by SplitTextNodeCommand
https://bugs.webkit.org/show_bug.cgi?id=42941

Attachment 62546: Added a test and fix per Darin's comment.
https://bugs.webkit.org/attachment.cgi?id=62546&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
(In reply to comment #8)
> In the past we had serious problems with crashes when we try to undo a
command after changes to the document. I changed all the simple command undo
and redo code to be defensive; after that change those crashes are almost
completely a thing of the past. We should not go back to the old way.
> 
> I agree that the simple command steps should not be "smart". They should know
only enough to do a mechanical operation, and not have some higher level view
of how this operation fits into a larger comment.
> 
> Generally speaking I am not fond of functions that can be "called wrong";
ones that have to do a lot of assertions on their arguments. Overall they are
harder to use than functions that do something well defined in all cases, make
few assumptions, and hence have to make few assertions.

I see. Thanks for the detailed explanation.

> But in undo and redo, in doUnappy and doReapply, we’ll need sufficient checks
to make sure the code does not lead to a crash or problem if a careless or
malicious JavaScript programmer changes the document between the initial
command execution, undo, and redo.

Agreed.

> > I followed the pattern for SplitElementCommand.
> 
> Yes, I don’t like the name there either.

Renamed but let me know if you have a better one.


> - With the patch, the redo code path will crash if m_text2 no longer has a
parent node. This can arise when JavaScript programmers change the document
between the undo and redo operations. So the code we removed from doApply does
need to be kept around for doReapply.

Fixed with a test. But AppendChildNode is adding back "hello" and I can't spot
where it's coming from. It should probably be addressed in another bug though.

> - With the patch, the redo code path will now operate on the nodes even if
they are now part of non-editable content. That’s a policy change that is
probably not an improvement. The code we removed from doApply probably does
need to be kept around for doReapply

Fixed.

> - Since we’re going to keep the text node around, we may need to do some
reality checks on it and not do the redo command in certain cases. For example,
if the text node has a parent we may want to do nothing. This can arise when
JavaScript programmers change the document between the undo and redo
operations. One of the benefits of the old approach was that changes to that
text node could not interfere with future undo/redo.

Are you talking about the case where m_text1 gets a parent?

(In reply to comment #10)
> (From update of attachment 62518 [details])
> WebCore/editing/SplitTextNodeCommand.cpp:99
>  +	  int ec = 0;
> We normally use the type ExceptionCode rather than just "int".

Oops, fixed.