[webkit-reviews] review granted: [Bug 56874] Repeated copy and paste-in-place operation results in increasingly verbose HTML : [Attachment 86545] Patch2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 86545: Patch2
https://bugs.webkit.org/attachment.cgi?id=86545&action=review

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

I’m going to say review+, but I think there is also room for improvement.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:794
> +    if (node->hasTagName(spanTag) && node->renderer() &&
node->renderer()->isInline()) {

I don’t understand why for all those other tags we are assuming they are always
inline, but for span we are checking if a renderer exists and is actually an
inline. Pages can style those other types of tags to be non-inline too; there
is nothing special about span in this respect.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:796
> +	   const HTMLElement* elem = static_cast<const HTMLElement*>(node);
> +	   return elem->hasAttribute(styleAttr);

I don’t think you need a local variable for this. If you did want to keep it,
I’d suggest naming it element, not elem.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:947
> +    // If we are not trying to match the destination style and we are not
> +    // inside a list element we prefer a position that is outside inline
> +    // elements that provide style. This way we can produce a less verbose
> +    // markup.
> +    if (!m_matchStyle && !enclosingList(insertionPos.deprecatedNode())) {

Why is being in a list relevant? Comments need to focus on the question “why”
so the comment should explain.

Why are you using the deprecatedNode function? Isn’t there a more modern way to
write this, avoiding the functions with “deprecated” in their names?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:956
> +	       if (parentNode->childNodes()->length() > 1)

There are more efficient ways to check if something has more than one child.
This will walk the entire list of child nodes to count them all just to check
if it is > 1. You could just check parent->firstChild()->nextSibling() to see
if it is non-zero, with the appropriate null checks.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:957
> +		   splitElement(static_cast<Element*>(parentNode),
parentNode->childNodes()->item(insertionPos.deprecatedEditingOffset()));

Is there a guarantee that the parent is not the document? I just want to make
sure the cast is safe.