[webkit-reviews] review denied: [Bug 42941] Redo fails after text node is split by SplitTextNodeCommand : [Attachment 62534] Added doReapply to SplitTextNodeCommand (asserts parent node is editable)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 42941: Redo fails after text node is split by SplitTextNodeCommand
https://bugs.webkit.org/show_bug.cgi?id=42941

Attachment 62534: Added doReapply to SplitTextNodeCommand (asserts parent node
is editable)
https://bugs.webkit.org/attachment.cgi?id=62534&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
I don’t understand why this patch is needed.

By default, doReapply calls doApply. The existing logic in
SplitTextNodeCommand::doApply looks like it would work fine if you called it
again after doUnapply was called. I trust that there is a failure because you
provide a test case. Could you be more specific about exactly what fails?

I suspect there is a much simpler way to fix this.

> -    Node* parent = m_text2->parentNode();
> -    if (!parent || !parent->isContentEditable())
> -	   return;
> +    ASSERT(m_text2->parentNode() &&
m_text2->parentNode()->isContentEditable());

I don’t think it’s a good idea to change this into an assertion. What’s the
guarantee this function won’t be called incorrectly? In general, the approach
we’ve been using in the editing commands is to have the low level commands
themselves do reality checks. This makes it a a little harder to cause crashes
by using the functions wrong and also makes it safer to use a doApply function
later as a doReapply function, where there has been time for the organization
of the document to possibly have been changed a bit.

I’m not sure why you decided to reverse this design direction for this
particular command.

> +    executeApply();

I think the function name "execute apply" is vague; two verbs in a row is also
hard to parse mentally. The function names for commands, such as doApply and
doReapply, are already quirky and a little hard to understand. Adding a
function named executeApply adds a bit to the confusion.

I won’t suggest another name, because I think there is no need to add functions
here at all.