[webkit-reviews] review denied: [Bug 62112] Context-aware HTML paste for Chromium : [Attachment 110925] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 62112: Context-aware HTML paste for Chromium
https://bugs.webkit.org/show_bug.cgi?id=62112

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=110925&action=review


> Source/WebCore/ChangeLog:-975
> -

Please revert this.

> Source/WebCore/editing/markup.cpp:496
> +    return node->hasTagName(addressTag)
> +	   || node->hasTagName(articleTag)
> +	   || node->hasTagName(asideTag)
> +	   || node->hasTagName(audioTag)
> +	   || node->hasTagName(blockquoteTag)
> +	   || node->hasTagName(canvasTag)
> +	   || node->hasTagName(ddTag)
> +	   || node->hasTagName(divTag)
> +	   || node->hasTagName(dlTag)
> +	   || node->hasTagName(fieldsetTag)
> +	   || node->hasTagName(figcaptionTag)
> +	   || node->hasTagName(figureTag)
> +	   || node->hasTagName(footerTag)
> +	   || node->hasTagName(formTag)
> +	   || node->hasTagName(headerTag)
> +	   || node->hasTagName(hgroupTag)
> +	   || node->hasTagName(hrTag)
> +	   || node->hasTagName(noscriptTag)
> +	   || node->hasTagName(olTag)
> +	   || node->hasTagName(outputTag)
> +	   || node->hasTagName(pTag)
> +	   || node->hasTagName(preTag)
> +	   || node->hasTagName(sectionTag)
> +	   || node->hasTagName(tableTag)
> +	   || node->hasTagName(tfootTag)
> +	   || node->hasTagName(ulTag)
> +	   || node->hasTagName(videoTag);

Where did you get this list? We should use the same list of nodes as
ancestorToRetainStructureAndAppearanceForBlock. e.g. this list is missing h1
and including random elements like video and audio. r- because of this code
duplication.

> Source/WebCore/editing/markup.cpp:502
> +    Node* commonAncestorBlock = enclosingNodeOfType(
> +	   firstPositionInOrBeforeNode(commonAncestor),
isHtmlBlockLevelElement);

Nit: It seems like this can fit in one line.

> Source/WebCore/editing/markup.cpp:749
> +    RefPtr<Node> startNode;
> +    RefPtr<Node> endNode;

Come to think of it, these noes should probably be renamed to
nodeBeforePastedContent and nodeAfterPastedContent.

> Source/WebCore/editing/markup.cpp:789
> +	   if (node->childNodeCount())
> +	       static_cast<ContainerNode*>(node.get())->removeAllChildren();
> +	   next = node->traverseNextNode();

Again, next = node->traverseNextSibling(); since you've deleted all children.