[webkit-reviews] review denied: [Bug 30892] REGRESSION(3.2.3 - 4.0.2): Message composing: when I undo a color change to text, undo/redo behaves strangely : [Attachment 42074] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Enrica Casucci <enrica at apple.com>'s
request for review:
Bug 30892: REGRESSION(3.2.3 - 4.0.2): Message composing: when I undo a color
change to text, undo/redo behaves strangely
https://bugs.webkit.org/show_bug.cgi?id=30892

Attachment 42074: Patch
https://bugs.webkit.org/attachment.cgi?id=42074&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Good approach!

> +void SplitElementCommand::doReapply()
> +{
> +    if (!m_element1)
> +	   return;
> +    
> +    if (m_atChild->parentNode() != m_element2)
> +	   return;
> +    
> +    Vector<RefPtr<Node> > children;
> +    for (Node* node = m_element2->firstChild(); node != m_atChild; node =
node->nextSibling())
> +	   children.append(node);
> +    
> +    ExceptionCode ec = 0;
> +    
> +    Node* parent = m_element2->parentNode();
> +    if (!parent)
> +	   return;
> +    parent->insertBefore(m_element1.get(), m_element2.get(), ec);
> +    if (ec)
> +	   return;
> +    
> +    size_t size = children.size();
> +    for (size_t i = 0; i < size; ++i)
> +	   m_element1->appendChild(children[i], ec);
> +}

This needs to share more code with doApply. It's not good to have all that code
copied and pasted. I suggest moving most of doApply into a new function and
have both doApply and doReapply call it.

Same comment for WrapContentsInDummySpanCommand.

review- because of the copied and pasted code