[webkit-reviews] review denied: [Bug 93643] Preserve styling elements in DeleteSelectionCommand : [Attachment 158440] proposed patch to fix test failures
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 14 16:40:43 PDT 2012
Darin Adler <darin at apple.com> has denied Alice Cheng <alice_cheng at apple.com>'s
request for review:
Bug 93643: Preserve styling elements in DeleteSelectionCommand
https://bugs.webkit.org/show_bug.cgi?id=93643
Attachment 158440: proposed patch to fix test failures
https://bugs.webkit.org/attachment.cgi?id=158440&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=158440&action=review
> Source/WebCore/editing/AppendNodeCommand.cpp:46
> - ASSERT(m_parent->rendererIsEditable() || !m_parent->attached());
> + ASSERT(m_parent->rendererIsEditable() || !m_parent->attached() ||
(m_parent->hasTagName(HTMLNames::headTag) &&
((HTMLElement*)m_parent.get())->contentEditable() == "true"));
This logic is long enough that I think it should go into a helper function now,
rather than being repeated three times.
The style of that cast is wrong, but you won’t need a typecast at all.
Something more like this:
ASSERT(m_parent->rendererIsEditable() || !m_parent->attached() ||
(m_parent->hasTagName(HTMLNames::headTag) && m_parent->isEditable()));
But also, you need a “why” comment. Why is the head element a special case?
> Source/WebCore/editing/DeleteSelectionCommand.cpp:425
> + if (!head || head->contentEditable() != "true")
> + return;
The right way to do this is just:
if (!head || !head->isContentEditable())
return;
You should use the contentEditable function that returns a string.
> Source/WebCore/editing/DeleteSelectionCommand.cpp:426
> + for (Node* node = range->firstNode(); node && node !=
range->pastLastNode(); node = node->traverseNextNode()) {
It’s not safe to point to a node with a raw pointer while editing a document.
Mutation events could run and cause the node to be deleted. So you need the
node to be a RefPtr<Node>.
> Source/WebCore/editing/DeleteSelectionCommand.cpp:430
> + RefPtr<Node> clone = node->cloneNode(true);
> + clone->setParentOrHostNode(0);
> + appendNode(clone, head);
You should not have to clone the node. Instead you should do a removeNode
followed by an appendNode to undoably move the node.
However, doing that also requires changing the way you walk from each node to
the next. Before removing a node you need to call traverseNextSibling to
advance to a node that won’t be removed.
> Source/WebCore/editing/DeleteSelectionCommand.cpp:443
> + // Move styling elements to prevent style loss
Sentence style comment should have a period in it.
More information about the webkit-reviews
mailing list