[webkit-reviews] review denied: [Bug 56874] Repeated copy and paste-in-place operation results in increasingly verbose HTML : [Attachment 86738] Patch4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Enrica Casucci <enrica at apple.com>'s
request for review:
Bug 56874: Repeated copy and paste-in-place operation results in increasingly
verbose HTML
https://bugs.webkit.org/show_bug.cgi?id=56874

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86738&action=review

review- because of the bad HTMLElement cast

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:788
> +    const AtomicString classAttributeValue = static_cast<const
HTMLElement*>(node)->getAttribute(classAttr);

It’s not safe to cast node to HTMLElement* without first checking if it is an
HTMLElement.

The type here should be const AtomicString&, not const AtomicString, to avoid
an extra bit of reference count churn.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:797
> +    const NamedNodeMap* attributesMap = static_cast<const
HTMLElement*>(node)->attributeMap();

This should just be attributeMap, not attributesMap.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:957
> +	       // If we are in the middle of a text node, we need to split it
before we can
> +	       // move the insertion position.
> +	       if (insertionPos.anchorNode()->isTextNode() &&
insertionPos.anchorType() == Position::PositionIsOffsetInAnchor &&
insertionPos.offsetInContainerNode())
> +		  
splitTextNodeContainingElement(static_cast<Text*>(insertionPos.anchorNode()),
insertionPos.offsetInContainerNode());

Does this handle the case where the offset is at the end of a text node? I
think besides checking the offset for 0 we might need to check if it’s the same
as the length of the text.