[webkit-reviews] review requested: [Bug 7579] TinyMCE: Implement execCommand(insertImage, ...) : [Attachment 6931] patch

[bugzilla-request-daemon at opendarwin.org]

Justin Garcia <justin.garcia at apple.com> has asked  for review:
Bug 7579: TinyMCE: Implement execCommand(insertImage, ...)
http://bugzilla.opendarwin.org/show_bug.cgi?id=7579

Attachment 6931: patch
http://bugzilla.opendarwin.org/attachment.cgi?id=6931&action=edit

------- Additional Comments from Justin Garcia <justin.garcia at apple.com>
> I don't think enabledPaste is a good name for the function any more if it's
> being used for other insertion. Are you sure that's the right function to
use?

I changed this and now use enabledAnySelection.  The enabledXXX functions need
work, I filed 7650.

> I think ReplaceSelectionCommand::prune would read better as a for loop.

I disagree, I left the while loop in.

> shouldPruneNode() will return true for a rendered leaf node (because it has a

> no rendered children).  r-

True, but it will never be passed a leaf because 1) prune is only passed
containers and 2) prune only ascends straight upward.

> Also, "prune" is not an appropriate name because a) it does not necessarily
remove any nodes

I don't think prune implies that nodes will necessarily be removed, only that
they will be removed if they are superfluous.

> b) it may remove more than just the passed node.

To make it clear what may be pruned I made the function into
removeNodeAndPruneAncestors.  This also frees us from worrying that we'll prune
leaves.